On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu <cgxu...@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.