Re: [PATCH 10/23] libnvdimm/labels: Add uuid helpers

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 11, 2021 at 10:11:56AM -0700, Dan Williams wrote:
> On Wed, Aug 11, 2021 at 9:59 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Aug 11, 2021 at 11:05:55AM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 09, 2021 at 03:28:40PM -0700, Dan Williams wrote:
> > > > In preparation for CXL labels that move the uuid to a different offset
> > > > in the label, add nsl_{ref,get,validate}_uuid(). These helpers use the
> > > > proper uuid_t type. That type definition predated the libnvdimm
> > > > subsystem, so now is as a good a time as any to convert all the uuid
> > > > handling in the subsystem to uuid_t to match the helpers.
> > > >
> > > > As for the whitespace changes, all new code is clang-format compliant.
> > >
> > > Thanks, looks good to me!
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Sorry, I'm in doubt this Rb stays. See below.
> >
> > ...
> >
> > > >  struct btt_sb {
> > > >     u8 signature[BTT_SIG_LEN];
> > > > -   u8 uuid[16];
> > > > -   u8 parent_uuid[16];
> > > > +   uuid_t uuid;
> > > > +   uuid_t parent_uuid;
> >
> > uuid_t type is internal to the kernel. This seems to be an ABI?
> 
> No, it's not a user ABI, this is an on-disk metadata structure. uuid_t
> is approprirate.

So, changing size of the structure is forbidden after this change, right?
I don't like this. It means we always stuck with this type to be like this and
no change will be allowed.

> > > >     __le32 flags;
> > > >     __le16 version_major;
> > > >     __le16 version_minor;
> >
> > ...
> >
> > > >  struct nd_namespace_label {
> > > > -   u8 uuid[NSLABEL_UUID_LEN];
> > > > +   uuid_t uuid;
> >
> > So seems this.
> >
> > > >     u8 name[NSLABEL_NAME_LEN];
> > > >     __le32 flags;
> > > >     __le16 nlabel;
> >
> > ...
> >
> > I'm not familiar with FS stuff, but looks to me like unwanted changes.
> > In such cases you have to use export/import APIs. otherwise you make the type
> > carved in stone without even knowing that it's part of an ABI or some hardware
> > / firmware interfaces.
> 
> Can you clarify the concern? Carving the intent that these 16-bytes
> are meant to be treated as UUID in stone is deliberate.

It's a bit surprise to me. Do we have any documentation on that?
How do we handle such types in kernel that covers a lot of code?

-- 
With Best Regards,
Andy Shevchenko