On Thu, 2019-09-26 at 15:06 +0800, Ian Kent wrote:
> 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?
Actually, forget about this.
That "e", "w" and "i" will attract inconsistent log formatting
comments.
Probably simplest to just add an xfs log macro to log to the
kernel log and also use the mount-api log macro if context ->log
is non-null.
>
> Ian
>
