On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> Christian Brauner <[email protected]> writes:
>
> > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> >> NULL are handled as 'flag' types. However, parameters that have the
> >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> >>
> >> Signed-off-by: Luis Henriques <[email protected]>
> >> ---
> >> fs/fs_parser.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> index edb3712dcfa5..53f6cb98a3e0 100644
> >> --- a/fs/fs_parser.c
> >> +++ b/fs/fs_parser.c
> >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> >> /* Try to turn the type we were given into the type desired by the
> >> * parameter and give an error if we can't.
> >> */
> >> - if (is_flag(p)) {
> >> + if (is_flag(p) ||
> >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
> >> if (param->type != fs_value_is_flag)
> >> return inval_plog(log, "Unexpected value for '%s'",
> >> param->key);
> >
> > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > param->string is guaranteed to not be NULL. So really this is only
> > about:
> >
> > FSCONFIG_SET_FD
> > FSCONFIG_SET_BINARY
> > FSCONFIG_SET_PATH
> > FSCONFIG_SET_PATH_EMPTY
> >
> > and those values being used without a value. What filesystem does this?
> > I don't see any.
> >
> > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > from every helper that isn't fs_param_is_string() until we actually have
> > a filesystem that wants to use any of the above as flags. Will lose a
> > lot of code that isn't currently used.
>
> Right, I find it quite confusing and I may be fixing the issue in the
> wrong place. What I'm seeing with ext4 when I mount a filesystem using
> the option '-o usrjquota' is that fs_parse() will get:
>
> * p->type is set to fs_param_is_string
> ('p' is a struct fs_parameter_spec, ->type is a function)
> * param->type is set to fs_value_is_flag
> ('param' is a struct fs_parameter, ->type is an enum)
>
> This is because ext4 will use the __fsparam macro to set define a
> fs_param_spec as a fs_param_is_string but will also set the
> fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> as a flag. That's why param->string will be NULL in this case.
So I'm a bit confused here. Valid variants of these quota options are like
"usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
quota file name). The variant "usrjquota" should ideally be rejected
because it doesn't make a good sense and only adds to confusion. Now as far
as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
shows) this is what is happening so what is exactly the problem you're
trying to fix?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> > Christian Brauner <[email protected]> writes:
> >
> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> > >> NULL are handled as 'flag' types. However, parameters that have the
> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> > >>
> > >> Signed-off-by: Luis Henriques <[email protected]>
> > >> ---
> > >> fs/fs_parser.c | 3 ++-
> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> > >> --- a/fs/fs_parser.c
> > >> +++ b/fs/fs_parser.c
> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> > >> /* Try to turn the type we were given into the type desired by the
> > >> * parameter and give an error if we can't.
> > >> */
> > >> - if (is_flag(p)) {
> > >> + if (is_flag(p) ||
> > >> + (!param->string && (p->flags & fs_param_can_be_empty))) {
> > >> if (param->type != fs_value_is_flag)
> > >> return inval_plog(log, "Unexpected value for '%s'",
> > >> param->key);
> > >
> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > > param->string is guaranteed to not be NULL. So really this is only
> > > about:
> > >
> > > FSCONFIG_SET_FD
> > > FSCONFIG_SET_BINARY
> > > FSCONFIG_SET_PATH
> > > FSCONFIG_SET_PATH_EMPTY
> > >
> > > and those values being used without a value. What filesystem does this?
> > > I don't see any.
> > >
> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > > from every helper that isn't fs_param_is_string() until we actually have
> > > a filesystem that wants to use any of the above as flags. Will lose a
> > > lot of code that isn't currently used.
> >
> > Right, I find it quite confusing and I may be fixing the issue in the
> > wrong place. What I'm seeing with ext4 when I mount a filesystem using
> > the option '-o usrjquota' is that fs_parse() will get:
> >
> > * p->type is set to fs_param_is_string
> > ('p' is a struct fs_parameter_spec, ->type is a function)
> > * param->type is set to fs_value_is_flag
> > ('param' is a struct fs_parameter, ->type is an enum)
> >
> > This is because ext4 will use the __fsparam macro to set define a
> > fs_param_spec as a fs_param_is_string but will also set the
> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> > as a flag. That's why param->string will be NULL in this case.
>
> So I'm a bit confused here. Valid variants of these quota options are like
> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> quota file name). The variant "usrjquota" should ideally be rejected
> because it doesn't make a good sense and only adds to confusion. Now as far
> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> shows) this is what is happening so what is exactly the problem you're
> trying to fix?
mount(8) has no way of easily knowing that for something like
mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
indistinguishable from a flag because it's specified without an
argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
we should require mount(8) to know what mount options are strings or no.
I've ran into this issue before myself when using the mount api
programatically.