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.
