Re: [PATCH v2 2/7] overlayfs: Convert to super_set_uuid()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 6, 2024 at 11:49 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> > We don't want to be settingc sb->s_uuid directly anymore, as there's a
> > length field that also has to be set, and this conversion was not
> > completely trivial.
> >
> > Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> > Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> > Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> > Cc: linux-unionfs@xxxxxxxxxxxxxxx
> > ---
> >  fs/overlayfs/util.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 0217094c23ea..f1f0ee9a9dff 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> >                        const struct path *upperpath)
> >  {
> >       bool set = false;
> > +     uuid_t uuid;
> >       int res;
> >
> >       /* Try to load existing persistent uuid */
> > -     res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> > +     res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
> >                               UUID_SIZE);
> >       if (res == UUID_SIZE)
> > -             return true;
> > +             goto success;
> >
> >       if (res != -ENODATA)
> >               goto fail;
> > @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> >       }
> >
> >       /* Generate overlay instance uuid */
> > -     uuid_gen(&sb->s_uuid);
> > +     uuid_gen(&uuid);
> >
> >       /* Try to store persistent uuid */
> >       set = true;
> > -     res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> > +     res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
> >                          UUID_SIZE);
> >       if (res == 0)
> > -             return true;
> > +             goto success;
>
> This is a bit weird. Normally the success case is in line, and we
> jump out of line for the fail case. I think this is more better:
>
>         if (res)
>                 goto fail;
> success:
>         super_set_uuid(sb, uuid.b, sizeof(uuid));
>         return true;
> >
> >  fail:
> >       memset(sb->s_uuid.b, 0, UUID_SIZE);
>
> And then the fail case follows naturally.
>

I agree. Please use the label name
set_uuid:

Also the memset() in fail: case is not needed anymore, because
with your change we do not dirty sb->s_uuid in this function.

Thanks,
Amir.





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux