[RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Amir Goldstein]

We need a helper for VFS to check if struct super_block field s_uuid
was filled by the filesystem, before consumers can use it.

The libnvdimm code already has a private helper to check if uuid is null
and the helper name is not using a private namespace prefix, which
prevents us from using the same helper name as a common function.

Hoist the libnvdimm helper as an inline helper to linux/uuid.h.

Suggested-by: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 drivers/nvdimm/btt_devs.c | 8 +-------
 include/linux/uuid.h      | 7 +++++++
 2 files changed, 8 insertions(+), 7 deletions(-)

Christoph,

Following your suggestion to provide a helper for checking if filesystem
had filled sb->s_uuid, here is a patch to add that helper.

I considered hoisting xfs's uuid_is_nil() helper and uuid_t to uuid.h, but
that would have been more painful, so just moved this simple helper instead.
We can continue debating which implementation is better, but that would be
futile...

If in the future xfs uuid table code is going to be moved to VFS, we should
probably move variants of the xfs/uuid.c functions to lib/uuid.c if those
variants don't already exist.

CC'ing the maintainers of clearcache and EVM/IMA to see if this helper
should be added (as sanity?) to their code before accessing sb->s_uuid.

I truely hope that EVM/IMA signatures do not expect xfs/ubifs to keep
exporting null s_uuid, because if they do, then starting to export s_uuid
for xfs/ubifs may require a new config/mkfs/mount option and that would
be a shame.

Cheers,
Amir.

diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 97dd292..d0fcda2 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/uuid.h>
 #include "nd-core.h"
 #include "btt.h"
 #include "nd.h"
@@ -222,13 +223,6 @@ struct device *nd_btt_create(struct nd_region *nd_region)
 	return dev;
 }
 
-static bool uuid_is_null(u8 *uuid)
-{
-	static const u8 null_uuid[16];
-
-	return (memcmp(uuid, null_uuid, 16) == 0);
-}
-
 /**
  * nd_btt_arena_is_valid - check if the metadata layout is valid
  * @nd_btt:	device with BTT geometry and backing device info
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index 4dff73a..d3f8656 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -58,6 +58,13 @@ static inline int uuid_be_cmp(const uuid_be u1, const uuid_be u2)
 	return memcmp(&u1, &u2, sizeof(uuid_be));
 }
 
+static inline bool uuid_is_null(u8 *uuid)
+{
+	static const u8 null_uuid[16];
+
+	return (memcmp(uuid, null_uuid, 16) == 0);
+}
+
 void generate_random_uuid(unsigned char uuid[16]);
 
 extern void uuid_le_gen(uuid_le *u);
-- 
2.7.4

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Dan Williams]

On Tue, May 2, 2017 at 1:19 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> We need a helper for VFS to check if struct super_block field s_uuid
> was filled by the filesystem, before consumers can use it.
>
> The libnvdimm code already has a private helper to check if uuid is null
> and the helper name is not using a private namespace prefix, which
> prevents us from using the same helper name as a common function.
>
> Hoist the libnvdimm helper as an inline helper to linux/uuid.h.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Acked-by: Dan Williams <dan.j.williams@intel.com>

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Andy Shevchenko]

On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote:
> We need a helper for VFS to check if struct super_block field s_uuid
> was filled by the filesystem, before consumers can use it.
> 
> The libnvdimm code already has a private helper to check if uuid is
> null
> and the helper name is not using a private namespace prefix, which
> prevents us from using the same helper name as a common function.
> 

Hmm...
Have you checked my branch here:
https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid
?

Probably not.
I have number of patches to make UUID API used kernel wide.

...including helpers for null UUID check:
https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc0f0b
62cce2b6eb7fdb?at=topic/uuid

> Hoist the libnvdimm helper as an inline helper to linux/uuid.h.
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  drivers/nvdimm/btt_devs.c | 8 +-------
>  include/linux/uuid.h      | 7 +++++++
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> Christoph,
> 
> Following your suggestion to provide a helper for checking if
> filesystem
> had filled sb->s_uuid, here is a patch to add that helper.
> 
> I considered hoisting xfs's uuid_is_nil() helper and uuid_t to uuid.h,
> but
> that would have been more painful, so just moved this simple helper
> instead.
> We can continue debating which implementation is better, but that
> would be
> futile...
> 
> If in the future xfs uuid table code is going to be moved to VFS, we
> should
> probably move variants of the xfs/uuid.c functions to lib/uuid.c if
> those
> variants don't already exist.
> 
> CC'ing the maintainers of clearcache and EVM/IMA to see if this helper
> should be added (as sanity?) to their code before accessing sb-
> >s_uuid.
> 
> I truely hope that EVM/IMA signatures do not expect xfs/ubifs to keep
> exporting null s_uuid, because if they do, then starting to export
> s_uuid
> for xfs/ubifs may require a new config/mkfs/mount option and that
> would
> be a shame.
> 
> Cheers,
> Amir.
> 
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 97dd292..d0fcda2 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> +#include <linux/uuid.h>
>  #include "nd-core.h"
>  #include "btt.h"
>  #include "nd.h"
> @@ -222,13 +223,6 @@ struct device *nd_btt_create(struct nd_region
> *nd_region)
>  	return dev;
>  }
>  
> -static bool uuid_is_null(u8 *uuid)
> -{
> -	static const u8 null_uuid[16];
> -
> -	return (memcmp(uuid, null_uuid, 16) == 0);
> -}
> -
>  /**
>   * nd_btt_arena_is_valid - check if the metadata layout is valid
>   * @nd_btt:	device with BTT geometry and backing device info
> diff --git a/include/linux/uuid.h b/include/linux/uuid.h
> index 4dff73a..d3f8656 100644
> --- a/include/linux/uuid.h
> +++ b/include/linux/uuid.h
> @@ -58,6 +58,13 @@ static inline int uuid_be_cmp(const uuid_be u1,
> const uuid_be u2)
>  	return memcmp(&u1, &u2, sizeof(uuid_be));
>  }
>  
> +static inline bool uuid_is_null(u8 *uuid)
> +{
> +	static const u8 null_uuid[16];
> +
> +	return (memcmp(uuid, null_uuid, 16) == 0);
> +}
> +
>  void generate_random_uuid(unsigned char uuid[16]);
>  
>  extern void uuid_le_gen(uuid_le *u);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Amir Goldstein]

On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote:
>> We need a helper for VFS to check if struct super_block field s_uuid
>> was filled by the filesystem, before consumers can use it.
>>
>> The libnvdimm code already has a private helper to check if uuid is
>> null
>> and the helper name is not using a private namespace prefix, which
>> prevents us from using the same helper name as a common function.
>>
>
> Hmm...
> Have you checked my branch here:
> https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid
> ?
>

Hi Andy,

No I have not seen your patches. When do you plan to post them?

> Probably not.
> I have number of patches to make UUID API used kernel wide.
>

That looks nice, but I see you did not get to converting many other
users that represent uuid as u8[16], like libnvdimm, bluetooth,
various filesystems and users of sb->s_uuid (IMA).

> ...including helpers for null UUID check:
> https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc0f0b
> 62cce2b6eb7fdb?at=topic/uuid
>

I wouldn't mind using is_null_uuid_le(sb->s_uuid) myself and that is how
I implemented the test in my current patch set, but I can hear the voices of
those saying that there should be a 'natural' helper uuid_is_null(u8 *) for the
users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid).

Also, I suggest naming your helpers uuid_{le|be}_is_null() to maintain
the uuid_* naming convention with other helpers you added to uuid.h.

Cheers,
Amir.

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Andy Shevchenko]

On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote:
> On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote:
> > > We need a helper for VFS to check if struct super_block field
> > > s_uuid
> > > was filled by the filesystem, before consumers can use it.
> > > 
> > > The libnvdimm code already has a private helper to check if uuid
> > > is
> > > null
> > > and the helper name is not using a private namespace prefix, which
> > > prevents us from using the same helper name as a common function.
> > > 
> > 
> > Hmm...
> > Have you checked my branch here:
> > https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid
> > ?
> > 
> 
> Hi Andy,
> 
> No I have not seen your patches. When do you plan to post them?

Some of them had been sent in different series recently and ~year ago.
Some need to be polished and tested first.

So, somewhere in the future for sure.

> > Probably not.
> > I have number of patches to make UUID API used kernel wide.
> > 
> 
> That looks nice, but I see you did not get to converting many other
> users that represent uuid as u8[16], like libnvdimm, bluetooth,
> various filesystems and users of sb->s_uuid (IMA).

Yes, my main goal is to make ACPI glue layer to use UUID API.

I didn't touch and I won't touch filesystems since it's a huge area
which I have no knowledge of.

For the rest I might be interested to do, but have no time.

> 
> > ...including helpers for null UUID check:
> > https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc
> > 0f0b
> > 62cce2b6eb7fdb?at=topic/uuid
> > 
> 
> I wouldn't mind using is_null_uuid_le(sb->s_uuid) myself and that is
> how
> I implemented the test in my current patch set, but I can hear the
> voices of
> those saying that there should be a 'natural' helper uuid_is_null(u8
> *) for the
> users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid).

u8 * doesn't represent UUID as a type.
Perhaps we need to reflect this in the name of the function somehow.

> Also, I suggest naming your helpers uuid_{le|be}_is_null() to maintain
> the uuid_* naming convention with other helpers you added to uuid.h.

I'm open to a name that satisfies most of us. So, no objections here.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Amir Goldstein]

On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote:
>> On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko
[...]

>> I can hear the voices of
>> those saying that there should be a 'natural' helper uuid_is_null(u8
>> *) for the
>> users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid).
>
> u8 * doesn't represent UUID as a type.
> Perhaps we need to reflect this in the name of the function somehow.
>

OK, since you have no time nor the intention to convert filesystem code
to use uuid_le (not should you), I see 2 options for filesystems/VFS:

1. Use the simple u8* libnvdimm helper proposed by this patch
2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h lib/uuid.c

If option #2 is preferred, I think we should conform to libuuid's helper
name uuid_is_null() (instead of freebsd's uuid_is_nil()) and maybe
convert libnvdimm and bluetooth to use the xfs helpers as well.

Christoph,

Since you got me started on this helper, do you have a preference?

Amir.

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Andy Shevchenko]

On Wed, 2017-05-03 at 15:42 +0300, Amir Goldstein wrote:
> On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote:
> > > On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko

> I can hear the voices of
> > > those saying that there should be a 'natural' helper
> > > uuid_is_null(u8
> > > *) for the
> > > users that represent uuid as u8[16] (i.e. filesystems and sb-
> > > >s_uuid).
> > 
> > u8 * doesn't represent UUID as a type.
> > Perhaps we need to reflect this in the name of the function somehow.
> > 
> 
> OK, since you have no time nor the intention to convert filesystem
> code
> to use uuid_le 

uuid_be I suppose. I noticed core developers doesn't support endianess
feature of UUID: https://lkml.org/lkml/2016/5/25/724

That's also why I don't want to touch filesystem stuff.

> (not should you), I see 2 options for filesystems/VFS:
> 
> 1. Use the simple u8* libnvdimm helper proposed by this patch
> 2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h
> lib/uuid.c
> 
> If option #2 is preferred, I think we should conform to libuuid's
> helper
> name uuid_is_null() (instead of freebsd's uuid_is_nil()) 

I think option #2 is preferred and we need actually to submit some
generic uuid_{be|le}_cmp() helpers.

For now it's blocked by Gcc bug.

We may remove const from the prototypes for now and add them later when
the bug will be fixed.

> and maybe
> convert libnvdimm and bluetooth to use the xfs helpers as well.
> 
> Christoph,
> 
> Since you got me started on this helper, do you have a preference?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Andy Shevchenko]

On Wed, 2017-05-03 at 19:35 +0300, Andy Shevchenko wrote:
> On Wed, 2017-05-03 at 15:42 +0300, Amir Goldstein wrote:
> > On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote:
> > > > On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko

> > I can hear the voices of
> > > > those saying that there should be a 'natural' helper
> > > > uuid_is_null(u8
> > > > *) for the
> > > > users that represent uuid as u8[16] (i.e. filesystems and sb-
> > > > > s_uuid).
> > > 
> > > u8 * doesn't represent UUID as a type.
> > > Perhaps we need to reflect this in the name of the function
> > > somehow.
> > > 
> > 
> > OK, since you have no time nor the intention to convert filesystem
> > code
> > to use uuid_le 
> 
> uuid_be I suppose. I noticed core developers doesn't support endianess
> feature of UUID: https://lkml.org/lkml/2016/5/25/724
> 
> That's also why I don't want to touch filesystem stuff.
> 
> > (not should you), I see 2 options for filesystems/VFS:
> > 
> > 1. Use the simple u8* libnvdimm helper proposed by this patch
> > 2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h
> > lib/uuid.c
> > 
> > If option #2 is preferred, I think we should conform to libuuid's
> > helper
> > name uuid_is_null() (instead of freebsd's uuid_is_nil()) 

I renamed as proposed and updated my branch.

By the way, notice that uuid_v1 type (which had been moved to uuid.h
already) is the same what xfs uses as uuid_t.

So, helpers against it we can prefix with uuid_v1_[foo_bar()].

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Amir Goldstein]

On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

>
> By the way, notice that uuid_v1 type (which had been moved to uuid.h
> already) is the same what xfs uses as uuid_t.
>
> So, helpers against it we can prefix with uuid_v1_[foo_bar()].
>

I really don't see why we can't use the exact same helpers
found in fs/xfs/uuid.c and uuid_t as well for that matter.

They are mostly similar to the libuuid API that filesystems
tools use in userspace and they don't collide with your
uuid_{le|be} helpers.

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Andy Shevchenko]

On Wed, 2017-05-03 at 21:34 +0300, Amir Goldstein wrote:
> On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > 
> > By the way, notice that uuid_v1 type (which had been moved to uuid.h
> > already) is the same what xfs uses as uuid_t.
> > 
> > So, helpers against it we can prefix with uuid_v1_[foo_bar()].
> > 
> 
> I really don't see why we can't use the exact same helpers
> found in fs/xfs/uuid.c and uuid_t as well for that matter.

Does it make sense to have two types that defines the same?

> They are mostly similar to the libuuid API that filesystems
> tools use in userspace and they don't collide with your
> uuid_{le|be} helpers.

Okay, if you are insisting just move them to the header, we may simplify
it later, though I think that there is no makes sense to do any effort
that will be neglected in the future (so, just movement is fine, but
using it elsewhere I prefer to have 1 type and const for parameters).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC][PATCH] linux/uuid.h: hoist uuid_is_null() helper from libnvdimm

[Amir Goldstein]

On Thu, May 4, 2017 at 11:21 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2017-05-03 at 21:34 +0300, Amir Goldstein wrote:
>> On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> >
>> > By the way, notice that uuid_v1 type (which had been moved to uuid.h
>> > already) is the same what xfs uses as uuid_t.
>> >
>> > So, helpers against it we can prefix with uuid_v1_[foo_bar()].
>> >
>>
>> I really don't see why we can't use the exact same helpers
>> found in fs/xfs/uuid.c and uuid_t as well for that matter.
>
> Does it make sense to have two types that defines the same?
>
>> They are mostly similar to the libuuid API that filesystems
>> tools use in userspace and they don't collide with your
>> uuid_{le|be} helpers.
>
> Okay, if you are insisting just move them to the header, we may simplify
> it later, though I think that there is no makes sense to do any effort
> that will be neglected in the future (so, just movement is fine, but
> using it elsewhere I prefer to have 1 type and const for parameters).
>

Preparing to post a series with

typedef struct uuid_v1 uuid_t;

I think you'll like it... stay tuned.