Re: [PATCH v2 2/2] kernel-trace: Less calls for iput() in create_trace_uprobe() after error detection

[Steven Rostedt]

On Sun, 16 Nov 2014 20:22:22 +0100
SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:

> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Sun, 16 Nov 2014 19:49:39 +0100
> 
> The iput() function was called in three cases by the create_trace_uprobe()
> function during error handling even if the passed variable contained still
> a null pointer. This implementation detail could be improved by the
> introduction of another jump label.

The first patch is fine, and the only reason is to save the few bytes
that the branch check might take. It's in a path that is unlikely to be
hit so it is not a performance issue at all.

This patch is useless. I rather not apply any patch than to create
another jump that skips over the freeing of iput() just because we know
inode is null. That's why we had the if (inode) in the first place.

So Nack on this patch and I'll contemplate applying the first one. I
probably will as it seems rather harmless.

Thanks,

-- Steve


> 
> Suggested-by: Julia Lawall <julia.lawall@xxxxxxx>
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  kernel/trace/trace_uprobe.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index ec002c0..a0288f2 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -434,19 +434,24 @@ static int create_trace_uprobe(int argc, char **argv)
>  	arg = strchr(argv[1], ':');
>  	if (!arg) {
>  		ret = -EINVAL;
> -		goto fail_address_parse;
> +		goto fail_address_parse2;
>  	}
>  
>  	*arg++ = '\0';
>  	filename = argv[1];
>  	ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>  	if (ret)
> -		goto fail_address_parse;
> +		goto fail_address_parse2;
>  
>  	inode = igrab(path.dentry->d_inode);
>  	path_put(&path);
>  
> -	if (!inode || !S_ISREG(inode->i_mode)) {
> +	if (!inode) {
> +		ret = -EINVAL;
> +		goto fail_address_parse2;
> +	}
> +
> +	if (!S_ISREG(inode->i_mode)) {
>  		ret = -EINVAL;
>  		goto fail_address_parse;
>  	}
> @@ -554,6 +559,7 @@ error:
>  fail_address_parse:
>  	iput(inode);
>  
> +fail_address_parse2:
>  	pr_info("Failed to parse address or file.\n");
>  
>  	return ret;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/