Adrian Bunk wrote:
> I considered it more odd to read
>
>
> rc = cgroupstats_build(stats, file->f_dentry);
> if (rc < 0)
> goto err;
>
> fput_light(file, fput_needed);
> return send_reply(rep_skb, info->snd_pid);
> err:
> fput_light(file, fput_needed);
> nlmsg_free(rep_skb);
>
I agree, but sometimes the alternative can be worse to read and
understand
>
> But that's just personal taste.
>
> Anyway, duplicating the same fput_light() call two or three times isn't
> nice.
>
> Originally I had the bigger patch below and considered it too big for
> only this fix, but now I think it might be appropriate.
>
> If you ignore whitespace when diff'ing, then all it does is:
>
> <-- snip -->
>
> @@ -398,7 +398,9 @@ static int cgroupstats_user_cmd(struct sk_buff *skb,
> struct genl_info *info)
>
> fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]);
> file = fget_light(fd, &fput_needed);
> - if (file) {
> + if (!file)
> + return 0;
> +
> size = nla_total_size(sizeof(struct cgroupstats));
>
> rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb,
> @@ -412,17 +414,15 @@ static int cgroupstats_user_cmd(struct sk_buff *skb,
> struct genl_info *info)
> memset(stats, 0, sizeof(*stats));
>
> rc = cgroupstats_build(stats, file->f_dentry);
> - if (rc < 0)
> + if (rc < 0) {
> + nlmsg_free(rep_skb);
> goto err;
> -
> - fput_light(file, fput_needed);
> - return send_reply(rep_skb, info->snd_pid);
> }
>
> + rc = send_reply(rep_skb, info->snd_pid);
> +
> err:
> - if (file)
> fput_light(file, fput_needed);
> - nlmsg_free(rep_skb);
> return rc;
> }
>
> <-- snip -->
>
>
> Is this patch OK for you?
>
>
> cu
> Adrian
>
>
> <-- snip -->
>
>
> We'd better not nlmsg_free on a pointer containing an undefined value
> (and without having anything allocated).
>
> Spotted by the Coverity checker.
>
> Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
>
> ---
>
> kernel/taskstats.c | 36 ++++++++++++++++++------------------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> eedd297ac36b5d92fc38b937677ba40dc6df1cd0
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 354e74b..07e86a8 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -398,31 +398,31 @@ static int cgroupstats_user_cmd(struct sk_buff *skb,
> struct genl_info *info)
>
> fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]);
> file = fget_light(fd, &fput_needed);
> - if (file) {
> - size = nla_total_size(sizeof(struct cgroupstats));
> + if (!file)
> + return 0;
>
> - rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb,
> - size);
> - if (rc < 0)
> - goto err;
> + size = nla_total_size(sizeof(struct cgroupstats));
>
> - na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS,
> - sizeof(struct cgroupstats));
> - stats = nla_data(na);
> - memset(stats, 0, sizeof(*stats));
> + rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb,
> + size);
> + if (rc < 0)
> + goto err;
>
> - rc = cgroupstats_build(stats, file->f_dentry);
> - if (rc < 0)
> - goto err;
> + na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS,
> + sizeof(struct cgroupstats));
> + stats = nla_data(na);
> + memset(stats, 0, sizeof(*stats));
>
> - fput_light(file, fput_needed);
> - return send_reply(rep_skb, info->snd_pid);
> + rc = cgroupstats_build(stats, file->f_dentry);
> + if (rc < 0) {
> + nlmsg_free(rep_skb);
> + goto err;
> }
>
> + rc = send_reply(rep_skb, info->snd_pid);
> +
> err:
> - if (file)
> - fput_light(file, fput_needed);
> - nlmsg_free(rep_skb);
> + fput_light(file, fput_needed);
> return rc;
> }
>
>
Looks good to me!
Acked-by: Balbir Singh <[EMAIL PROTECTED]>
Yet to be tested, I'll try and test it soon.
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
