String options always have parameters, hence the check for optional parameter will never trigger.
Check for param type being a flag first (flag is the only type that does not have a parameter) and report "Missing value" if the parameter is mandatory.
Tested with gfs2's "quota" option, which is currently the only user of fs_param_v_optional.
Signed-off-by: Miklos Szeredi mszeredi@redhat.com Cc: Andrew Price anprice@redhat.com Cc: David Howells dhowells@redhat.com Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers") Cc: stable@vger.kernel.org # v5.4 --- fs/fs_parser.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/fs_parser.c b/fs/fs_parser.c index d1930adce68d..5d8833d71b37 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -127,13 +127,15 @@ int fs_parse(struct fs_context *fc, case fs_param_is_u64: case fs_param_is_enum: case fs_param_is_string: - if (param->type != fs_value_is_string) - goto bad_value; - if (!result->has_value) { + if (param->type == fs_value_is_flag) { if (p->flags & fs_param_v_optional) goto okay; - goto bad_value; + + return invalf(fc, "%s: Missing value for '%s'", + desc->name, param->key); } + if (param->type != fs_value_is_string) + goto bad_value; /* Fall through */ default: break;
On 28/11/2019 15:59, Miklos Szeredi wrote:
String options always have parameters, hence the check for optional parameter will never trigger.
Check for param type being a flag first (flag is the only type that does not have a parameter) and report "Missing value" if the parameter is mandatory.
Tested with gfs2's "quota" option, which is currently the only user of fs_param_v_optional.
It's not clear to me what the bug is here. My tests with the quota option are giving expected results. Perhaps I missed a case?
Andy
Signed-off-by: Miklos Szeredi mszeredi@redhat.com Cc: Andrew Price anprice@redhat.com Cc: David Howells dhowells@redhat.com Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers") Cc: stable@vger.kernel.org # v5.4
fs/fs_parser.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/fs_parser.c b/fs/fs_parser.c index d1930adce68d..5d8833d71b37 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -127,13 +127,15 @@ int fs_parse(struct fs_context *fc, case fs_param_is_u64: case fs_param_is_enum: case fs_param_is_string:
if (param->type != fs_value_is_string)goto bad_value;if (!result->has_value) {
if (param->type == fs_value_is_flag) { if (p->flags & fs_param_v_optional) goto okay;
goto bad_value;
return invalf(fc, "%s: Missing value for '%s'",desc->name, param->key);if (param->type != fs_value_is_string)goto bad_value;
On Fri, Nov 29, 2019 at 12:31 PM Andrew Price anprice@redhat.com wrote:
On 28/11/2019 15:59, Miklos Szeredi wrote:
String options always have parameters, hence the check for optional parameter will never trigger.
Check for param type being a flag first (flag is the only type that does not have a parameter) and report "Missing value" if the parameter is mandatory.
Tested with gfs2's "quota" option, which is currently the only user of fs_param_v_optional.
It's not clear to me what the bug is here. My tests with the quota option are giving expected results. Perhaps I missed a case?
fsopen-test-2: fsconfig(3, FSCONFIG_SET_FLAG, "quota", NULL, 0): Invalid argument fsopen-test-2: context log: <e gfs2: Bad value for 'quota'>
kernel: 5.4.0-08836-g81b6b96475ac
Thanks, Miklos
On 29/11/2019 14:43, Miklos Szeredi wrote:
On Fri, Nov 29, 2019 at 12:31 PM Andrew Price anprice@redhat.com wrote:
On 28/11/2019 15:59, Miklos Szeredi wrote:
String options always have parameters, hence the check for optional parameter will never trigger.
Check for param type being a flag first (flag is the only type that does not have a parameter) and report "Missing value" if the parameter is mandatory.
Tested with gfs2's "quota" option, which is currently the only user of fs_param_v_optional.
It's not clear to me what the bug is here. My tests with the quota option are giving expected results. Perhaps I missed a case?
fsopen-test-2: fsconfig(3, FSCONFIG_SET_FLAG, "quota", NULL, 0): Invalid argument fsopen-test-2: context log: <e gfs2: Bad value for 'quota'>
kernel: 5.4.0-08836-g81b6b96475ac
Ah right, gotcha. My tests were relying on the same codepaths being used from the legacy/monolithic parsing code.
Reviewed-by: Andrew Price anprice@redhat.com
Andy
On Thu, Nov 28, 2019 at 04:59:30PM +0100, Miklos Szeredi wrote:
String options always have parameters, hence the check for optional parameter will never trigger.
What do you mean, always have parameters? Granted, for fsconfig(2) it's (currently) true, but I see at least two other pathways that do not impose such requirement - vfs_parse_fs_string() and rbd_parse_options().
You seem to deal with the former later in the patchset, but I don't see anything for the latter...
On Mon, Dec 16, 2019 at 11:28:45PM +0000, Al Viro wrote:
On Thu, Nov 28, 2019 at 04:59:30PM +0100, Miklos Szeredi wrote:
String options always have parameters, hence the check for optional parameter will never trigger.
What do you mean, always have parameters? Granted, for fsconfig(2) it's (currently) true, but I see at least two other pathways that do not impose such requirement - vfs_parse_fs_string() and rbd_parse_options().
You seem to deal with the former later in the patchset, but I don't see anything for the latter...
FWIW, I strongly dislike fs_param_v_optional. I mean, look at the gfs2 usecase: quota ->uint_64 = 0 ->negated = false quota=off ->uint_32 = 1 ->negated = false quota=account ->uint_32 = 2 ->negated = false quota=on ->uint_32 = 3 ->negated = false noquota ->boolean = false ->negated = true with gfs2 postprocessing for that thing being if (result.negated) args->ar_quota = GFS2_QUOTA_OFF; else if (result.int_32 > 0) args->ar_quota = opt_quota_values[result.int_32]; else args->ar_quota = GFS2_QUOTA_ON; break; and that relies upon having enum opt_quota members associated with off/account/on starting from 1. I mean, WTF? What we really want is quota GFS2_QUOTA_ON quota=on GFS2_QUOTA_ON quota=account GFS2_QUOTA_ACCOUNT quota=off GFS2_QUOTA_OFF noquota GFS2_QUOTA_OFF
I certainly agree that flag/NULL string is ugly; do we even want to keep fs_value_is_flag? It's internal-only, so we can bloody well turn it into fs_value_is_string and ->string is NULL... And sure, ->has_value is redundant - if nothing else, it would make a lot more sense as static inline bool param_has_value(const struct fs_parameter *param) { return !!param->string; } But I really wonder if we should keep breeding kludges. Look at the use cases, including the yet-to-be-merged ones. 1) GFS2: see above 2) ceph: fsc/nofsc/fsc=... 3) ext4: init_itable/noinit_itable/init_itable=<number> 4) nfs: fsc/nofsc/fsc=...
All of that is trivially handled by splitting the opt=... and opt cases. We have two such in the tree and two more in posted patchsets. Plus one more that ext4 patchset breaks, AFAICS (barrier). Out of several hundreds. Everything else either requires = in all cases or rejects it in all cases.
So how about a flag for "takes no arguments", set automatically by fsparam_flag()/fsparam_flag_no(), with fs_lookup_key() taking an extra "comes with argument" flag and filtering according to it? Rules: foo => "foo", true foo= => "foo", false foo=bar => "foo", false And to hell with the "optional" flag; for gfs2 we'd end up with fsparam_flag_no("quota", Opt_quota_flag), // quota|noquota fsparam_flag_enum("quota", Opt_quota, gfs2_param_quota), // quota={on|account|off} Postprocessing won't be any harder, really - we could bloody well do case Opt_quota_flag: result.int_32 = result.negated ? GFS2_QUOTA_OFF : GFS2_QUOTA_ON; /* fallthru */ case Opt_quota: args->ar_quota = result.int_32; break; with gfs2_param_quota having the right values in it, instead of that intermediate enum.
All ->has_value checks go away that way, AFAICS. With minimal impact on yet-to-be-merged series...
On Tue, Dec 17, 2019 at 01:18:13AM +0000, Al Viro wrote:
So how about a flag for "takes no arguments", set automatically by fsparam_flag()/fsparam_flag_no(), with fs_lookup_key() taking an extra "comes with argument" flag and filtering according to it? Rules: foo => "foo", true foo= => "foo", false foo=bar => "foo", false And to hell with the "optional" flag; for gfs2 we'd end up with fsparam_flag_no("quota", Opt_quota_flag), // quota|noquota fsparam_flag_enum("quota", Opt_quota, gfs2_param_quota), // quota={on|account|off} Postprocessing won't be any harder, really - we could bloody well do case Opt_quota_flag: result.int_32 = result.negated ? GFS2_QUOTA_OFF : GFS2_QUOTA_ON; /* fallthru */ case Opt_quota: args->ar_quota = result.int_32; break; with gfs2_param_quota having the right values in it, instead of that intermediate enum.
All ->has_value checks go away that way, AFAICS. With minimal impact on yet-to-be-merged series...
FWIW, we have the following types right now: fs_param_is_flag no argument fs_param_is_bool no argument or string argument (1 instance in mainline[*]) fs_param_is_u32 string argument [**] fs_param_is_u32_octal string argument fs_param_is_u32_hex string argument fs_param_is_s32 string argument fs_param_is_u64 string argument fs_param_is_enum string argument; gfs2 has the quota/noquota/quota=... mess fs_param_is_string string argument; ceph (and nfs) have fsc/nofsc/fsc=... fs_param_is_fd unused at the moment; eventually - string argument. fs_param_is_blob fsconfig-only fs_param_is_blockdev fsconfig-only fs_param_is_path fsconfig-only
[*] no_disconnect in gadgetfs; buggered, since it used to require a numeric argument. Now it quietly treats no_disconnect as no_disconnect=1 and rejects e.g. no_disconnect=2. There are two more in ext4, also buggered as far as I can tell. [**] ext4 has init_itable/init_itable=%u/noinit_itable
The total over all filesystems with conversions posted or already in mainline:
* no_disconnect [drivers/usb/gadget/function/f_fs.c, broken] * init_itable/init_itable=%u/noinit_itable [ext4] * fsc/fsc=%s/nofsc [ceph] * fsc/fsc=%s/nofsc [nfs] * quota/quota=on/quota=off/quota=account/noquota [gfs2] * barrier/barrier=%u/nobarrier [ext4] * auto_da_alloc/auto_da_alloc=%u/noauto_da_alloc [ext4]
It's rare. So much that I don't believe that fs_param_v_optional has any reason to exist. Let's split those entries and be done with that.
Hmm... Deciding whether we have an argument-bearing or no-argument case is somewhat inconvenient for fsconfig-generated calls - check for NULL param->string is, strictly speaking, in nasal daemon territory for types other than flag and string... OK, sold - let's check for fs_value_is_flag. So I'm combining your patch with #9/12 (to avoid bisect hazard) + correction for rbd. And putting that in the very beginning of fs_parser reorg series, with elimination of fs_param_v_optional closer to the end.
linux-stable-mirror@lists.linaro.org
-
Al Viro -
Andrew Price -
Miklos Szeredi