lkml.org 
[lkml]   [2018]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible
From
Date
Thanks for your review, I’ll fix the issue in v3.


> 在 2018年5月3日,上午6:32,Andrew Morton <akpm@linux-foundation.org> 写道:
>
> On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu <cgxu519@gmx.com> wrote:
>
>> Currently when detecting invalid options in option parsing,
>> some options(e.g. msize) just set errno and allow to continuously
>> validate other options so that it can detect invalid options
>> as much as possible and give proper error messages together.
>>
>> This patch applies same rule to option 'trans' and 'version'
>> when detecting EINVAL.
>>
>> ...
>
> A few issues:
>
>> --- a/net/9p/client.c
>> +++ b/net/9p/client.c
>> @@ -199,7 +199,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> s);
>> ret = -EINVAL;
>> kfree(s);
>> - goto free_and_return;
>> + continue;
>> }
>> kfree(s);
>> break;
>
> Here we can just do
>
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -198,8 +198,6 @@ static int parse_opts(char *opts, struct
> pr_info("Could not find request transport: %s\n",
> s);
> ret = -EINVAL;
> - kfree(s);
> - continue;
> }
> kfree(s);
> break;
>
>> @@ -217,7 +217,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> ret = get_protocol_version(s);
>
> And here's the patch introduces a bug: if `ret' has value -EINVAL from
> an earlier parsing error, this code will overwrite that error code with
> zero.
>
> So you'll need to introduce a new temporary variable here. And I
> suggest that for future-safety, we permit all error returns from
> get_protocol_version(), not just -EINVAL. So something like:
>
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -214,13 +214,12 @@ static int parse_opts(char *opts, struct
> "problem allocating copy of version arg\n");
> goto free_and_return;
> }
> - ret = get_protocol_version(s);
> - if (ret == -EINVAL) {
> - kfree(s);
> - continue;
> - }
> + pv = get_protocol_version(s);
> + if (pv >= 0)
> + clnt->proto_version = pv;
> + else
> + ret = pv;
> kfree(s);
> - clnt->proto_version = ret;
> break;
> default:
> continue;
> _
>
>
> Similar changes can be made to patch 2/2.
>

\
 
 \ /
  Last update: 2018-05-03 09:44    [W:0.181 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site