On Tue, 2019-08-27 at 08:41 -0400, Brian Foster wrote:
> On Fri, Aug 23, 2019 at 08:59:43AM +0800, Ian Kent wrote:
> > Make xfs_parse_param() take arguments of the fs context operation
> > .parse_param() in preperation for switching to use the file system
> > mount context for mount.
> >
> > The function fc_parse() only uses the file system context (fc here)
> > when calling log macros warnf() and invalf() which in turn check
> > only the fc->log field to determine if the message should be saved
> > to a context buffer (for later retrival by userspace) or logged
> > using printk().
> >
> > Also the temporary function match_kstrtoint() is now unused, remove
> > it.
> >
> > Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
> > ---
>
> I see Eric had some feedback on this patch already. Some additional
> notes (which may overlap)...
>
> > fs/xfs/xfs_super.c | 187 ++++++++++++++++++++++++++++++--------
> > --------------
> > 1 file changed, 108 insertions(+), 79 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 3ae29938dd89..754d2ccfd960 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> ...
> > @@ -275,33 +271,38 @@ xfs_parse_param(
> > mp->m_flags |= XFS_MOUNT_NOUUID;
> > break;
> > case Opt_ikeep:
> > - mp->m_flags |= XFS_MOUNT_IKEEP;
> > - break;
> > - case Opt_noikeep:
> > - mp->m_flags &= ~XFS_MOUNT_IKEEP;
> > + if (result.negated)
> > + mp->m_flags &= ~XFS_MOUNT_IKEEP;
> > + else
> > + mp->m_flags |= XFS_MOUNT_IKEEP;
> > break;
> > case Opt_largeio:
> > - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
> > - break;
> > - case Opt_nolargeio:
> > - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
> > + if (result.negated)
> > + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
> > + else
> > + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
> > break;
> > case Opt_attr2:
> > - mp->m_flags |= XFS_MOUNT_ATTR2;
> > - break;
> > - case Opt_noattr2:
> > - mp->m_flags &= ~XFS_MOUNT_ATTR2;
> > - mp->m_flags |= XFS_MOUNT_NOATTR2;
> > + if (!result.negated) {
> > + mp->m_flags |= XFS_MOUNT_ATTR2;
> > + } else {
> > + mp->m_flags &= ~XFS_MOUNT_ATTR2;
> > + mp->m_flags |= XFS_MOUNT_NOATTR2;
> > + }
>
> Eric's comments aside, it would be nice to have some consistency
> between
> the various result.negated checks (i.e. 'if (negated)' vs 'if
> (!negated)').
Right, that's my "the smallest block must always be first 'ism"
cutting in there. I always feel that a larger block before a
smaller block makes the later (partially) invisible in reading
the code. But the fact is that seeing this annoys me so I always
see the hanging code so what I'm saying is kind-off nonsence!
I'll try and overcome my 'ism and make this consistent, ;)
>
> > break;
> > case Opt_filestreams:
> > mp->m_flags |= XFS_MOUNT_FILESTREAMS;
> > break;
> > - case Opt_noquota:
> > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
> > - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
> > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
> > - break;
> > case Opt_quota:
> > + if (!result.negated) {
> > + mp->m_qflags |= (XFS_UQUOTA_ACCT |
> > XFS_UQUOTA_ACTIVE |
> > + XFS_UQUOTA_ENFD);
> > + } else {
> > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
> > + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
> > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
> > + }
> > + break;
> > case Opt_uquota:
> > case Opt_usrquota:
> > mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
> ...
> > @@ -367,10 +368,10 @@ xfs_parseargs(
> > {
> > const struct super_block *sb = mp->m_super;
> > char *p;
> > - substring_t args[MAX_OPT_ARGS];
> > - int dsunit = 0;
> > - int dswidth = 0;
> > - uint8_t iosizelog = 0;
> > +
> > + struct fs_context fc;
> > + struct xfs_fs_context context;
> > + struct xfs_fs_context *ctx = &context;
>
> I don't really see the point for having a separate pointer variable
> based on the code so far. Why not just do:
>
> struct xfs_fs_context ctx = {0,};
>
> ... and pass by reference where necessary?
Yep, it was a stupid mistake to begin with.
I'm using only the context variable now.
>
> >
> > /*
> > * set up the mount name first so all the errors will refer to
> > the
> > @@ -406,17 +407,41 @@ xfs_parseargs(
> > if (!options)
> > goto done;
> >
> > + memset(&fc, 0, sizeof(fc));
> > + memset(&ctx, 0, sizeof(ctx));
> > + fc.fs_private = ctx;
> > + fc.s_fs_info = mp;
> > +
> > while ((p = strsep(&options, ",")) != NULL) {
> > - int token;
> > - int ret;
> > + struct fs_parameter param;
> > + char *value;
> > + int ret;
> >
> > if (!*p)
> > continue;
> >
> > - token = match_token(p, tokens, args);
> > - ret = xfs_parse_param(token, p, args, mp,
> > - &dsunit, &dswidth, &iosizelog);
> > - if (ret)
> > + param.key = p;
> > + param.type = fs_value_is_string;
> > + param.size = 0;
> > +
> > + value = strchr(p, '=');
> > + if (value) {
> > + if (value == p)
> > + continue;
>
> What's the purpose of the above check? Why do we skip the param as
> opposed to return an error or something?
Well, that's a good question, there's a thought in the back
of my mind that if the char isn't found the whole string
is returned but that's not the way strchr(3) works so I was
thinking of something different.
Maybe I'm not the only one to think this way because it looks
like the VFS code does the same thing (and that's where this
came from altough I think it was slightly different when I
did that).
But I'm probably miss-reading the VFS code ...
I'll need to look closer at it.
>
> > + *value++ = 0;
> > + param.size = strlen(value);
> > + if (param.size > 0) {
> > + param.string = kmemdup_nul(value,
> > + param.size,
> > + GFP_KERNEL);
> > + if (!param.string)
> > + return -ENOMEM;
> > + }
> > + }
> > +
> > + ret = xfs_parse_param(&fc, ¶m);
> > + kfree(param.string);
> > + if (ret < 0)
> > return ret;
> > }
> >
> ...
> > @@ -1914,6 +1939,10 @@ static const struct super_operations
> > xfs_super_operations = {
> > .free_cached_objects = xfs_fs_free_cached_objects,
> > };
> >
> > +static const struct fs_context_operations xfs_context_ops = {
> > + .parse_param = xfs_parse_param,
> > +};
> > +
>
> It's probably better to introduce this in the first patch where it's
> used.
Either or, I thought that building up the operations structure
as we go gave insight into the context of where the change is
headed.
But if doing this irritates your sensibilities I can change
it as long as no-one else has strong preferences against it.
>
> Brian
>
> > static struct file_system_type xfs_fs_type = {
> > .owner = THIS_MODULE,
> > .name = "xfs",
> >
