On Thu, 2019-09-26 at 05:14 +0100, Al Viro wrote:
> On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
>
> > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > + if (opt < 0) {
> > + /*
> > + * If fs_parse() returns -ENOPARAM and the parameter
> > + * is "source" the VFS needs to handle this option
> > + * in order to boot otherwise use the default case
> > + * below to handle invalid options.
> > + */
> > + if (opt != -ENOPARAM ||
> > + strcmp(param->key, "source") == 0)
> > + return opt;
>
> Just return opt; here and be done with that. The comment is bloody
> misleading - for one thing, "in order to boot" is really "in order to
> mount anything", and the only reason for the kludge is that the
> default for "source" (in vfs_parse_fs_param(), triggered in case
> when -ENOPARAM had been returned by ->parse_param()) won't get
> triggered
> if you insist on reporting _all_ unknown options on your own.
>
> > + }
> > default:
> > - xfs_warn(mp, "unknown mount option [%s].", p);
> > + xfs_warn(mp, "unknown mount option [%s].", param->key);
> > return -EINVAL;
>
> ... here, instead of letting the same vfs_parse_fs_param() handle
> the warning.
>
> Or you could add Opt_source for handling that, with equivalent of
> that
> fallback (namely,
> if (param->type != fs_value_is_string)
> return invalf(fc, "VFS: Non-string source");
> if (fc->source)
> return invalf(fc, "VFS: Multiple sources");
> fc->source = param->string;
> param->string = NULL;
> return 0;
> ) done in your ->parse_param().
Either of those makes sense to me.
The only other thing relevant to either case is messages not going
to the kernel log if fsconfig() is being used which could make problem
resolution more difficult.
Any objection to changing logfc() to always log to the kernel log
and save messages to the context if fc->log is non-null rather than
the either or behaviour we have now?
Ian
