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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
