* [PATCH 1/2] net/9p: detecting invalid options as much as possible @ 2018-04-16 7:48 Chengguang Xu 2018-04-16 7:48 ` [PATCH 2/2] fs/9p: " Chengguang Xu 0 siblings, 1 reply; 4+ messages in thread From: Chengguang Xu @ 2018-04-16 7:48 UTC (permalink / raw) To: ericvh, rminnich, lucho; +Cc: v9fs-developer, linux-kernel, Chengguang Xu Currently when detecting invalid options in option parsing, some options(e.g. msize) just set errno and allow to continusly validate other options so that it can detect invalid options as much as possible and give proper error messages together. This patch apply this policy to all options and the case of memory allocation error in option parsing. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- net/9p/client.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/net/9p/client.c b/net/9p/client.c index 21e6df1..066f136 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -186,10 +186,11 @@ static int parse_opts(char *opts, struct p9_client *clnt) case Opt_trans: s = match_strdup(&args[0]); if (!s) { - ret = -ENOMEM; + if (!ret) + ret = -ENOMEM; p9_debug(P9_DEBUG_ERROR, "problem allocating copy of trans arg\n"); - goto free_and_return; + continue; } v9fs_put_trans(clnt->trans_mod); @@ -199,7 +200,7 @@ static int parse_opts(char *opts, struct p9_client *clnt) s); ret = -EINVAL; kfree(s); - goto free_and_return; + continue; } kfree(s); break; @@ -209,25 +210,28 @@ static int parse_opts(char *opts, struct p9_client *clnt) case Opt_version: s = match_strdup(&args[0]); if (!s) { - ret = -ENOMEM; + if (!ret) + ret = -ENOMEM; p9_debug(P9_DEBUG_ERROR, "problem allocating copy of version arg\n"); - goto free_and_return; + continue; } ret = get_protocol_version(s); if (ret == -EINVAL) { kfree(s); - goto free_and_return; + continue; } kfree(s); clnt->proto_version = ret; break; default: + p9_debug(P9_DEBUG_ERROR, + "unrecognized option \"%s\" or missing value\n", + p); continue; } } -free_and_return: v9fs_put_trans(clnt->trans_mod); kfree(tmp_options); return ret; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] fs/9p: detecting invalid options as much as possible 2018-04-16 7:48 [PATCH 1/2] net/9p: detecting invalid options as much as possible Chengguang Xu @ 2018-04-16 7:48 ` Chengguang Xu 2018-04-16 7:56 ` [V9fs-developer] " Dominique Martinet 0 siblings, 1 reply; 4+ messages in thread From: Chengguang Xu @ 2018-04-16 7:48 UTC (permalink / raw) To: ericvh, rminnich, lucho; +Cc: v9fs-developer, linux-kernel, Chengguang Xu Currently when detecting invalid options in option parsing, some options(e.g. debug) just set errno and allow to continusly validate other options so that it can detect invalid options as much as possible and give proper error messages together. This patch apply this policy to all options and the case of memory allocation error in option parsing. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- fs/9p/v9fs.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c index e622f0f..29ba937 100644 --- a/fs/9p/v9fs.c +++ b/fs/9p/v9fs.c @@ -192,10 +192,9 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) return 0; tmp_options = kstrdup(opts, GFP_KERNEL); - if (!tmp_options) { - ret = -ENOMEM; - goto fail_option_alloc; - } + if (!tmp_options) + return -ENOMEM; + options = tmp_options; while ((p = strsep(&options, ",")) != NULL) { @@ -263,18 +262,16 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) case Opt_uname: kfree(v9ses->uname); v9ses->uname = match_strdup(&args[0]); - if (!v9ses->uname) { - ret = -ENOMEM; - goto free_and_return; - } + if (!v9ses->uname) + if (!ret) + ret = -ENOMEM; break; case Opt_remotename: kfree(v9ses->aname); v9ses->aname = match_strdup(&args[0]); - if (!v9ses->aname) { - ret = -ENOMEM; - goto free_and_return; - } + if (!v9ses->aname) + if (!ret) + ret = -ENOMEM; break; case Opt_nodevmap: v9ses->nodev = 1; @@ -292,24 +289,24 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) #ifdef CONFIG_9P_FSCACHE kfree(v9ses->cachetag); v9ses->cachetag = match_strdup(&args[0]); - if (!v9ses->cachetag) { - ret = -ENOMEM; - goto free_and_return; - } + if (!v9ses->cachetag) + if (!ret) + ret = -ENOMEM; #endif break; case Opt_cache: s = match_strdup(&args[0]); if (!s) { - ret = -ENOMEM; + if (!ret) + ret = -ENOMEM; p9_debug(P9_DEBUG_ERROR, "problem allocating copy of cache arg\n"); - goto free_and_return; + continue; } ret = get_cache_mode(s); if (ret == -EINVAL) { kfree(s); - goto free_and_return; + continue; } v9ses->cache = ret; @@ -319,10 +316,11 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) case Opt_access: s = match_strdup(&args[0]); if (!s) { - ret = -ENOMEM; + if (!ret) + ret = -ENOMEM; p9_debug(P9_DEBUG_ERROR, "problem allocating copy of access arg\n"); - goto free_and_return; + continue; } v9ses->flags &= ~V9FS_ACCESS_MASK; @@ -341,14 +339,14 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) pr_info("Unknown access argument %s\n", s); kfree(s); - goto free_and_return; + continue; } v9ses->uid = make_kuid(current_user_ns(), uid); if (!uid_valid(v9ses->uid)) { ret = -EINVAL; pr_info("Uknown uid %s\n", s); kfree(s); - goto free_and_return; + continue; } } @@ -365,13 +363,14 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) break; default: + p9_debug(P9_DEBUG_ERROR, + "unrecognized mount option \"%s\" or missing value\n", + p); continue; } } -free_and_return: kfree(tmp_options); -fail_option_alloc: return ret; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible 2018-04-16 7:48 ` [PATCH 2/2] fs/9p: " Chengguang Xu @ 2018-04-16 7:56 ` Dominique Martinet 2018-04-16 11:37 ` cgxu519 0 siblings, 1 reply; 4+ messages in thread From: Dominique Martinet @ 2018-04-16 7:56 UTC (permalink / raw) To: Chengguang Xu; +Cc: ericvh, rminnich, lucho, v9fs-developer, linux-kernel Chengguang Xu wrote on Mon, Apr 16, 2018: > default: > + p9_debug(P9_DEBUG_ERROR, > + "unrecognized mount option \"%s\" or missing value\n", > + p); > continue; The problem with that is that the same options are passed to the vfs, net and transport init later on - none of which know the full subset of valid options to tell what option has been recognized or not. Unless we do some backward-incompatible breakage of passing all the net/transport options in its own option (e.g. net=foo:bar:moo) there unfortunately is no nice way of fixing this now. (I don't mind the rest of the patches, although I'd say if we hit ENOMEM there is likely trouble going on so I'm not so sure about hiding it if there also is an EINVAL, but I don't really care) -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible 2018-04-16 7:56 ` [V9fs-developer] " Dominique Martinet @ 2018-04-16 11:37 ` cgxu519 0 siblings, 0 replies; 4+ messages in thread From: cgxu519 @ 2018-04-16 11:37 UTC (permalink / raw) To: Dominique Martinet Cc: cgxu519, ericvh, rminnich, lucho, v9fs-developer, linux-kernel Hi Dominique, Thanks for your quick reply and comment. 在 2018年4月16日，下午3:56，Dominique Martinet <asmadeus@codewreck.org> 写道： > > Chengguang Xu wrote on Mon, Apr 16, 2018: >> default: >> + p9_debug(P9_DEBUG_ERROR, >> + "unrecognized mount option \"%s\" or missing value\n", >> + p); >> continue; > > The problem with that is that the same options are passed to the vfs, > net and transport init later on - none of which know the full subset of > valid options to tell what option has been recognized or not. > > Unless we do some backward-incompatible breakage of passing all the > net/transport options in its own option (e.g. net=foo:bar:moo) there > unfortunately is no nice way of fixing this now. Yes, I agree with you. > > > (I don't mind the rest of the patches, although I'd say if we hit ENOMEM > there is likely trouble going on so I'm not so sure about hiding it if > there also is an EINVAL, but I don't really care) The initial motivation of hiding ENOMEM here is when reproducing the error， the error code may change by system condition(more accurately memory condition), after this patch the error code will be persistent. However, as you pointed out, when we hit ENOMEM there would be a serious trouble, so in real life maybe we cannot benefit from it. What do you think? Thanks, Chengguang. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-16 11:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-16 7:48 [PATCH 1/2] net/9p: detecting invalid options as much as possible Chengguang Xu 2018-04-16 7:48 ` [PATCH 2/2] fs/9p: " Chengguang Xu 2018-04-16 7:56 ` [V9fs-developer] " Dominique Martinet 2018-04-16 11:37 ` cgxu519
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.