On Wed, Aug 11, 2021 at 12:18 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Aug 11, 2021 at 10:11:56AM -0700, Dan Williams wrote:
> > On Wed, Aug 11, 2021 at 9:59 AM Andy Shevchenko
> > <[email protected]> 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 <[email protected]>
> > >
> > > 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.
You want the flexibility to make a uuid_t not a 16-byte value? Isn't
that no longer a uuid_t? However, if the answer is yes, then I agree
it can not be used in these "on-disk" structures. I would expect
uuid_t size to be as reliable as any other Linux kernel specific type
that implies a size, and I would nak a patch that tried to change
uuid_t the way you describe.
That is, if I'm understanding your concern correctly...
>
> > > > > __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?
Documentation on these superblocks formats? Some are in EFI, some are
Linux specific.
> How do we handle such types in kernel that covers a lot of code?
I'm not following?
