On Tue, Aug 01, 2023 at 05:46:07PM +0200, Christoph Hellwig wrote:
> > + /* require the new mount api */
> > + if (exclusive && (fc->ops == &legacy_fs_context_ops))
>
> No need for the inner braces.
My love for operator precendence isn't very strong and having also
studied math has left me with an obsessive relationsip with brackets...
>
> > + return -EOPNOTSUPP;
> > +
> > fc->phase = FS_CONTEXT_CREATING;
> > + fc->exclusive = exclusive;
> >
> > ret = vfs_get_tree(fc);
> > - if (ret)
> > + if (ret) {
> > + fc->exclusive = false;
>
> What's the point in clearing the flag on error?
I originally thought that when the caller did:
fsconfig(fd_fs, FSCONFIG_CMD_CREATE_EXCL)
and this failed but the caller immediately followed this up with another
call to:
fsconfig(fd_fs, FSCONFIG_CMD_CREATE)
then leaving fc->exclusive set might turn FSCONFIG_CMD_CREATE into an
exclusive create on accident leaving the caller confused. But then I
remembered that this code explicitly fails the whole fs_context forever
after a failed superblock creation because after vfs_get_tree() it is
unclear what state the fs_context is in. And I apparently forgot to
remove that after I remembered that.
>
> > + case FSCONFIG_CMD_CREATE_EXCL:
> > + fallthrough;
> > case FSCONFIG_CMD_CREATE:
> > - ret = vfs_cmd_create(fc);
> > + ret = vfs_cmd_create(fc, cmd == FSCONFIG_CMD_CREATE_EXCL);
> > if (ret)
> > break;
> > return 0;
>
> Nitpick, but I always find it cleaner to do something like:
>
> case FSCONFIG_CMD_CREATE_EXCL:
> ret = vfs_cmd_create(fc, true)
> break;
> case FSCONFIG_CMD_CREATE:
> ret = vfs_cmd_create(fc, false);
>
> but that might just be preference.
Fine by me.
