Thanks for your review, I’ll fix the issue in v3.
> 在 2018年5月3日，上午6:32，Andrew Morton <[email protected]> 写道： > > On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu <[email protected]> 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. >
