Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible

[cgxu...@gmx.com]

Thanks for your review, I’ll fix the issue in v3.


> 在 2018年5月3日，上午6:32，Andrew Morton  写道：
> 
> On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu  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.
> 

Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible

[cgxu...@gmx.com]

Thanks for your review, I’ll fix the issue in v3.


> 在 2018年5月3日，上午6:32，Andrew Morton  写道：
> 
> On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu  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.
> 

Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible

[Andrew Morton]

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

Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible

[Andrew Morton]

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