Re: [PATCH] net: sched: One function call less in em_meta_change() after error detection

[Lino Sanfilippo]

Hi,

On 31.01.2015 17:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Sat, 31 Jan 2015 17:18:48 +0100
> 
> The meta_delete() function could be called in four cases by the
> em_meta_change() function during error handling even if the passed
> variable "meta" contained still a null pointer.
> 
> * This implementation detail could be improved by adjustments for jump labels.
> 
> * Let us return immediately after the first failed function call according to
>   the current Linux coding style convention.
> 
> * Let us delete also unnecessary checks for the variables "err" and
>   "meta" there.
> 
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  net/sched/em_meta.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
> index b5294ce..2aa67b1 100644
> --- a/net/sched/em_meta.c
> +++ b/net/sched/em_meta.c
> @@ -866,23 +866,23 @@ static int em_meta_change(struct net *net, void *data, int len,
>  
>  	err = nla_parse(tb, TCA_EM_META_MAX, data, len, meta_policy);
>  	if (err < 0)
> -		goto errout;
> +		return err;
>  
>  	err = -EINVAL;
>  	if (tb[TCA_EM_META_HDR] == NULL)
> -		goto errout;
> +		goto exit;
>  	hdr = nla_data(tb[TCA_EM_META_HDR]);
>  
>  	if (TCF_META_TYPE(hdr->left.kind) != TCF_META_TYPE(hdr->right.kind) ||
>  	    TCF_META_TYPE(hdr->left.kind) > TCF_META_TYPE_MAX ||
>  	    TCF_META_ID(hdr->left.kind) > TCF_META_ID_MAX ||
>  	    TCF_META_ID(hdr->right.kind) > TCF_META_ID_MAX)
> -		goto errout;
> +		goto exit;
>  
>  	meta = kzalloc(sizeof(*meta), GFP_KERNEL);
>  	if (meta == NULL) {
>  		err = -ENOMEM;
> -		goto errout;
> +		goto exit;
>  	}
>  
>  	memcpy(&meta->lvalue.hdr, &hdr->left, sizeof(hdr->left));
> @@ -891,20 +891,20 @@ static int em_meta_change(struct net *net, void *data, int len,
>  	if (!meta_is_supported(&meta->lvalue) ||
>  	    !meta_is_supported(&meta->rvalue)) {
>  		err = -EOPNOTSUPP;
> -		goto errout;
> +		goto delete_meta;
>  	}
>  
>  	if (meta_change_data(&meta->lvalue, tb[TCA_EM_META_LVALUE]) < 0 ||
>  	    meta_change_data(&meta->rvalue, tb[TCA_EM_META_RVALUE]) < 0)
> -		goto errout;
> +		goto delete_meta;
>  
>  	m->datalen = sizeof(*meta);
>  	m->data = (unsigned long) meta;
>  
> -	err = 0;
> -errout:
> -	if (err && meta)
> -		meta_delete(meta);
> +	return 0;
> +delete_meta:
> +	meta_delete(meta);
> +exit:
>  	return err;
>  }

Why do you use that exit label if it does nothing more than returning
the error value? Also if nla_parse fails you dont use it but return the
error directly. While using a label which is used only to return an
error may be a matter of taste, its at least inconsistent to do both in
a function, use such a label in one case and return immediately in
another, isnt it?

Regards,
Lino


--
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/