[PATCH v2 0/8] hoist uuid helpers from xfs to linux/uuid.h

[PATCH v2 0/8] hoist uuid helpers from xfs to linux/uuid.h

[Amir Goldstein]

Darick,

I moved some code around to be able to hoist all the generic
xfs uuid helpers to linux/uuid.h, so that other filesystems
and uuid related code in the kernel can use them.

Let me know what you think of the outcome.

I did not find any uuid related regressions with xfs.
Specifically, the following xfstests exercise these changes:
xfs/045 xfs/073 xfs/077 (multi uuid mount)
generic/079 xfs/238 xfs/269 xfs/062 (fshandle)

Some of these helpers are quite similar to thier uuid_{le|be}
variants in linux/uuid.h, but at the moment I don't see xfs
or any filesystem starting to use the uuid_{le|be} helpers,
so we probably have to live with this duplicity for a while
longer.

Amir.

Amir Goldstein (8):
  xfs: use uuid_copy() helper to abstract uuid_t
  xfs: re-define uuid_t as common struct uuid_v1
  xfs: dismiss xfs_uu_t
  xfs: namespace the helper uuid_getnodeuniq()
  md: namespace private helper names
  linux/uuid.h: hoist helpers uuid_equal() and uuid_copy() from xfs
  linux/uuid.h: hoist uuid_is_null() helper from libnvdimm
  xfs: use the common helper uuid_is_null()

 drivers/md/md.c           |  8 ++++----
 drivers/nvdimm/btt_devs.c | 10 ++--------
 fs/xfs/uuid.c             | 42 ++++++------------------------------------
 fs/xfs/uuid.h             | 14 ++------------
 fs/xfs/xfs_inode_item.c   |  8 ++------
 fs/xfs/xfs_log_recover.c  |  6 +++---
 fs/xfs/xfs_mount.c        | 10 +++++-----
 include/linux/uuid.h      | 21 +++++++++++++++++++--
 lib/uuid.c                |  3 +++
 9 files changed, 46 insertions(+), 76 deletions(-)

-- 
2.7.4

[PATCH v2 1/8] xfs: use uuid_copy() helper to abstract uuid_t

[Amir Goldstein]

uuid_t definition is about to change.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_inode_item.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index d90e781..9967bd7 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -829,9 +829,7 @@ xfs_inode_item_format_convert(
 		in_f->ilf_dsize = in_f32->ilf_dsize;
 		in_f->ilf_ino = in_f32->ilf_ino;
 		/* copy biggest field of ilf_u */
-		memcpy(in_f->ilf_u.ilfu_uuid.__u_bits,
-		       in_f32->ilf_u.ilfu_uuid.__u_bits,
-		       sizeof(uuid_t));
+		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f32->ilf_u.ilfu_uuid);
 		in_f->ilf_blkno = in_f32->ilf_blkno;
 		in_f->ilf_len = in_f32->ilf_len;
 		in_f->ilf_boffset = in_f32->ilf_boffset;
@@ -846,9 +844,7 @@ xfs_inode_item_format_convert(
 		in_f->ilf_dsize = in_f64->ilf_dsize;
 		in_f->ilf_ino = in_f64->ilf_ino;
 		/* copy biggest field of ilf_u */
-		memcpy(in_f->ilf_u.ilfu_uuid.__u_bits,
-		       in_f64->ilf_u.ilfu_uuid.__u_bits,
-		       sizeof(uuid_t));
+		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f64->ilf_u.ilfu_uuid);
 		in_f->ilf_blkno = in_f64->ilf_blkno;
 		in_f->ilf_len = in_f64->ilf_len;
 		in_f->ilf_boffset = in_f64->ilf_boffset;
-- 
2.7.4

[PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Amir Goldstein]

xfs was defining a non namespaced type named uuid_t and for no good
reason. xfs code doesn't care about the internals of uuid_t struct -
it only cares about its size.

Re-define uuid_t as the common struct uuid_v1 in include/linux/uuid.h
and get rid of the xfs private definition.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/uuid.h        | 4 +---
 include/linux/uuid.h | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index 104db0f..0e3ecd0 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -18,9 +18,7 @@
 #ifndef __XFS_SUPPORT_UUID_H__
 #define __XFS_SUPPORT_UUID_H__
 
-typedef struct {
-	unsigned char	__u_bits[16];
-} uuid_t;
+#include <linux/uuid.h>
 
 extern int uuid_is_nil(uuid_t *uuid);
 extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2);
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index 4dff73a..02253f0 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -26,7 +26,7 @@
  *     time
  * - the clock sequence is a 14-bit counter to avoid duplicate times
  */
-struct uuid_v1 {
+typedef struct uuid_v1 {
 	__be32		time_low;			/* low part of timestamp */
 	__be16		time_mid;			/* mid part of timestamp */
 	__be16		time_hi_and_version;		/* high part of timestamp and version  */
@@ -40,7 +40,7 @@ struct uuid_v1 {
 #define UUID_VARIANT_STD	0x80
 	u8		clock_seq_low;			/* clock seq low */
 	u8		node[6];			/* spatially unique node ID (MAC addr) */
-};
+} uuid_t;
 
 /*
  * The length of a UUID string ("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee")
-- 
2.7.4

[PATCH v2 3/8] xfs: dismiss xfs_uu_t

[Amir Goldstein]

uuid_t (a.k.a struct uuid_v1) already provides the fields
needed by uuid_getnodeuniq(), so use them directly.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/uuid.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index b83f76b..7bb4fb8 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -17,15 +17,6 @@
  */
 #include <xfs.h>
 
-/* IRIX interpretation of an uuid_t */
-typedef struct {
-	__be32	uu_timelow;
-	__be16	uu_timemid;
-	__be16	uu_timehi;
-	__be16	uu_clockseq;
-	__be16	uu_node[3];
-} xfs_uu_t;
-
 /*
  * uuid_getnodeuniq - obtain the node unique fields of a UUID.
  *
@@ -35,11 +26,10 @@ typedef struct {
 void
 uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
 {
-	xfs_uu_t *uup = (xfs_uu_t *)uuid;
-
-	fsid[0] = (be16_to_cpu(uup->uu_clockseq) << 16) |
-		   be16_to_cpu(uup->uu_timemid);
-	fsid[1] = be32_to_cpu(uup->uu_timelow);
+	fsid[0] = (uuid->clock_seq_hi_and_reserved << 24) |
+		  (uuid->clock_seq_low << 16) |
+		  be16_to_cpu(uuid->time_mid);
+	fsid[1] = be32_to_cpu(uuid->time_low);
 }
 
 int
-- 
2.7.4

[PATCH v2 4/8] xfs: namespace the helper uuid_getnodeuniq()

[Amir Goldstein]

uuid_getnodeuniq(), as the comment says, 'is not in any way
a standard or condoned UUID function'.

Therefore, prefix the helper name with xfs_ to differentiate it
from the rest of the common uuid_ helpers.

The uuid_ helpers are about to be hoisted to lib/uuid.c.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/uuid.c      | 4 ++--
 fs/xfs/uuid.h      | 2 +-
 fs/xfs/xfs_mount.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index 7bb4fb8..16958e7 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -18,13 +18,13 @@
 #include <xfs.h>
 
 /*
- * uuid_getnodeuniq - obtain the node unique fields of a UUID.
+ * xfs_uuid_getnodeuniq - obtain the node unique fields of a UUID.
  *
  * This is not in any way a standard or condoned UUID function;
  * it just something that's needed for user-level file handles.
  */
 void
-uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
+xfs_uuid_getnodeuniq(uuid_t *uuid, int fsid[2])
 {
 	fsid[0] = (uuid->clock_seq_hi_and_reserved << 24) |
 		  (uuid->clock_seq_low << 16) |
diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index 0e3ecd0..b40dee7 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -22,7 +22,7 @@
 
 extern int uuid_is_nil(uuid_t *uuid);
 extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2);
-extern void uuid_getnodeuniq(uuid_t *uuid, int fsid [2]);
+extern void xfs_uuid_getnodeuniq(uuid_t *uuid, int fsid[2]);
 
 static inline void
 uuid_copy(uuid_t *dst, uuid_t *src)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 4c0d8d7..f4fdce7 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -794,7 +794,7 @@ xfs_mountfs(
 	 *  Copies the low order bits of the timestamp and the randomly
 	 *  set "sequence" number out of a UUID.
 	 */
-	uuid_getnodeuniq(&sbp->sb_uuid, mp->m_fixedfsid);
+	xfs_uuid_getnodeuniq(&sbp->sb_uuid, mp->m_fixedfsid);
 
 	mp->m_dmevmask = 0;	/* not persistent; set after each mount */
 
-- 
2.7.4

[PATCH v2 5/8] md: namespace private helper names

[Amir Goldstein]

The md private helper uuid_equal() collides with a generic helper
of the same name.

Rename the md private helper to md_uuid_equal() and do the same for
md_sb_equal().

Cc: Shaohua Li <shli@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 drivers/md/md.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 548d1b8..e3a5163 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -821,7 +821,7 @@ static int read_disk_sb(struct md_rdev *rdev, int size)
 	return -EINVAL;
 }
 
-static int uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
+static int md_uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 {
 	return	sb1->set_uuid0 == sb2->set_uuid0 &&
 		sb1->set_uuid1 == sb2->set_uuid1 &&
@@ -829,7 +829,7 @@ static int uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 		sb1->set_uuid3 == sb2->set_uuid3;
 }
 
-static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
+static int md_sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 {
 	int ret;
 	mdp_super_t *tmp1, *tmp2;
@@ -1021,12 +1021,12 @@ static int super_90_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor
 	} else {
 		__u64 ev1, ev2;
 		mdp_super_t *refsb = page_address(refdev->sb_page);
-		if (!uuid_equal(refsb, sb)) {
+		if (!md_uuid_equal(refsb, sb)) {
 			pr_warn("md: %s has different UUID to %s\n",
 				b, bdevname(refdev->bdev,b2));
 			goto abort;
 		}
-		if (!sb_equal(refsb, sb)) {
+		if (!md_sb_equal(refsb, sb)) {
 			pr_warn("md: %s has same UUID but different superblock to %s\n",
 				b, bdevname(refdev->bdev, b2));
 			goto abort;
-- 
2.7.4

[PATCH v2 6/8] linux/uuid.h: hoist helpers uuid_equal() and uuid_copy() from xfs

[Amir Goldstein]

These helper are used to compare and copy two uuid_t type objects.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/uuid.c        |  6 ------
 fs/xfs/uuid.h        |  7 -------
 include/linux/uuid.h | 10 ++++++++++
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index 16958e7..d41fa68 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -45,9 +45,3 @@ uuid_is_nil(uuid_t *uuid)
 		if (*cp++) return 0;	/* not nil */
 	return 1;	/* is nil */
 }
-
-int
-uuid_equal(uuid_t *uuid1, uuid_t *uuid2)
-{
-	return memcmp(uuid1, uuid2, sizeof(uuid_t)) ? 0 : 1;
-}
diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index b40dee7..4911905 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -21,13 +21,6 @@
 #include <linux/uuid.h>
 
 extern int uuid_is_nil(uuid_t *uuid);
-extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2);
 extern void xfs_uuid_getnodeuniq(uuid_t *uuid, int fsid[2]);
 
-static inline void
-uuid_copy(uuid_t *dst, uuid_t *src)
-{
-	memcpy(dst, src, sizeof(uuid_t));
-}
-
 #endif	/* __XFS_SUPPORT_UUID_H__ */
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index 02253f0..a1dd9cc 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -58,6 +58,16 @@ 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_equal(const uuid_t *u1, const uuid_t *u2)
+{
+	return (memcmp(u1, u2, sizeof(uuid_t)) == 0);
+}
+
+static inline void uuid_copy(uuid_t *dst, const uuid_t *src)
+{
+	memcpy(dst, src, sizeof(uuid_t));
+}
+
 void generate_random_uuid(unsigned char uuid[16]);
 
 extern void uuid_le_gen(uuid_le *u);
-- 
2.7.4

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

[Amir Goldstein]

Hoist the libnvdimm helper as an inline helper to linux/uuid.h
using an auxiliary const variable uuid_null in lib/uuid.c.

The common helper uses the new abstract type uuid_t* instead of
u8*.

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 | 10 ++--------
 include/linux/uuid.h      |  7 +++++++
 lib/uuid.c                |  3 +++
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 97dd292..f52c66d 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
@@ -249,7 +243,7 @@ bool nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super)
 	if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
 		return false;
 
-	if (!uuid_is_null(super->parent_uuid))
+	if (!uuid_is_null((uuid_t *)super->parent_uuid))
 		if (memcmp(super->parent_uuid, parent_uuid, 16) != 0)
 			return false;
 
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index a1dd9cc..a617ccf 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -68,6 +68,13 @@ static inline void uuid_copy(uuid_t *dst, const uuid_t *src)
 	memcpy(dst, src, sizeof(uuid_t));
 }
 
+extern const uuid_t uuid_null;
+
+static inline bool uuid_is_null(uuid_t *uuid)
+{
+	return uuid_equal(uuid, &uuid_null);
+}
+
 void generate_random_uuid(unsigned char uuid[16]);
 
 extern void uuid_le_gen(uuid_le *u);
diff --git a/lib/uuid.c b/lib/uuid.c
index 37687af..17d0360 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -21,6 +21,9 @@
 #include <linux/uuid.h>
 #include <linux/random.h>
 
+const uuid_t uuid_null;
+EXPORT_SYMBOL(uuid_null);
+
 const u8 uuid_le_index[16] = {3,2,1,0,5,4,7,6,8,9,10,11,12,13,14,15};
 EXPORT_SYMBOL(uuid_le_index);
 const u8 uuid_be_index[16] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
-- 
2.7.4

[PATCH v2 8/8] xfs: use the common helper uuid_is_null()

[Amir Goldstein]

Use the common helper uuid_is_null() and remove the xfs specific
helper uuid_is_nil().

The common helper does not check for the NULL pointer value as
xfs helper did, but xfs code never calls the helper with a pointer
that can be NULL.

Conform comments and warning strings to use the term 'null uuid'
instead of 'nil uuid', because this is the terminology used by
lib/uuid.c and its users. It is also the terminology used in
userspace by libuuid and xfsprogs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/uuid.c            | 14 --------------
 fs/xfs/uuid.h            |  1 -
 fs/xfs/xfs_log_recover.c |  6 +++---
 fs/xfs/xfs_mount.c       |  8 ++++----
 4 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index d41fa68..a516805 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -31,17 +31,3 @@ xfs_uuid_getnodeuniq(uuid_t *uuid, int fsid[2])
 		  be16_to_cpu(uuid->time_mid);
 	fsid[1] = be32_to_cpu(uuid->time_low);
 }
-
-int
-uuid_is_nil(uuid_t *uuid)
-{
-	int	i;
-	char	*cp = (char *)uuid;
-
-	if (uuid == NULL)
-		return 0;
-	/* implied check of version number here... */
-	for (i = 0; i < sizeof *uuid; i++)
-		if (*cp++) return 0;	/* not nil */
-	return 1;	/* is nil */
-}
diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index 4911905..12f15d1 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -20,7 +20,6 @@
 
 #include <linux/uuid.h>
 
-extern int uuid_is_nil(uuid_t *uuid);
 extern void xfs_uuid_getnodeuniq(uuid_t *uuid, int fsid[2]);
 
 #endif	/* __XFS_SUPPORT_UUID_H__ */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 4a98762..004d0a9 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -352,13 +352,13 @@ xlog_header_check_mount(
 {
 	ASSERT(head->h_magicno == cpu_to_be32(XLOG_HEADER_MAGIC_NUM));
 
-	if (uuid_is_nil(&head->h_fs_uuid)) {
+	if (uuid_is_null(&head->h_fs_uuid)) {
 		/*
 		 * IRIX doesn't write the h_fs_uuid or h_fmt fields. If
-		 * h_fs_uuid is nil, we assume this log was last mounted
+		 * h_fs_uuid is null, we assume this log was last mounted
 		 * by IRIX and continue.
 		 */
-		xfs_warn(mp, "nil uuid in log - IRIX style log");
+		xfs_warn(mp, "null uuid in log - IRIX style log");
 	} else if (unlikely(!uuid_equal(&mp->m_sb.sb_uuid, &head->h_fs_uuid))) {
 		xfs_warn(mp, "log has mismatched uuid - can't recover");
 		xlog_header_check_dump(mp, head);
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index f4fdce7..9096948 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -80,14 +80,14 @@ xfs_uuid_mount(
 	if (mp->m_flags & XFS_MOUNT_NOUUID)
 		return 0;
 
-	if (uuid_is_nil(uuid)) {
-		xfs_warn(mp, "Filesystem has nil UUID - can't mount");
+	if (uuid_is_null(uuid)) {
+		xfs_warn(mp, "Filesystem has null UUID - can't mount");
 		return -EINVAL;
 	}
 
 	mutex_lock(&xfs_uuid_table_mutex);
 	for (i = 0, hole = -1; i < xfs_uuid_table_size; i++) {
-		if (uuid_is_nil(&xfs_uuid_table[i])) {
+		if (uuid_is_null(&xfs_uuid_table[i])) {
 			hole = i;
 			continue;
 		}
@@ -124,7 +124,7 @@ xfs_uuid_unmount(
 
 	mutex_lock(&xfs_uuid_table_mutex);
 	for (i = 0; i < xfs_uuid_table_size; i++) {
-		if (uuid_is_nil(&xfs_uuid_table[i]))
+		if (uuid_is_null(&xfs_uuid_table[i]))
 			continue;
 		if (!uuid_equal(uuid, &xfs_uuid_table[i]))
 			continue;
-- 
2.7.4

Re: [PATCH v2 4/8] xfs: namespace the helper uuid_getnodeuniq()

[Christoph Hellwig]

On Thu, May 04, 2017 at 04:26:19PM +0300, Amir Goldstein wrote:
> uuid_getnodeuniq(), as the comment says, 'is not in any way
> a standard or condoned UUID function'.
> 
> Therefore, prefix the helper name with xfs_ to differentiate it
> from the rest of the common uuid_ helpers.

I'd just kill it off and open code it in the only caller.

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

[Christoph Hellwig]

> -	if (!uuid_is_null(super->parent_uuid))
> +	if (!uuid_is_null((uuid_t *)super->parent_uuid))

No strange casts please, change parent_uuid to be an actual uuid_t
instead.

Re: [PATCH v2 1/8] xfs: use uuid_copy() helper to abstract uuid_t

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Christoph Hellwig]

On Thu, May 04, 2017 at 04:26:17PM +0300, Amir Goldstein wrote:
> xfs was defining a non namespaced type named uuid_t and for no good
> reason. xfs code doesn't care about the internals of uuid_t struct -
> it only cares about its size.
> 
> Re-define uuid_t as the common struct uuid_v1 in include/linux/uuid.h
> and get rid of the xfs private definition.

I'm not sure this really is a good idea.  uuid_v1 currently is only
used by afs.  I'd much rather switch both afs and xfs to use the
uuid_be type (which might as well grow the standard uuid_t name while
we're at it), and use accessors that do the byte-array access for
the very few places that care about the interpretation.

There is some more fallou from this, e.g. generate_random_uuid
should also take a uuid_be (and maybe renamed to uuid_generate_random).

Re: [PATCH v2 3/8] xfs: dismiss xfs_uu_t

[Christoph Hellwig]

On Thu, May 04, 2017 at 04:26:18PM +0300, Amir Goldstein wrote:
> uuid_t (a.k.a struct uuid_v1) already provides the fields
> needed by uuid_getnodeuniq(), so use them directly.

As mentioned below I'd much rather see get_unaligned_be* here on
the plain uuid_be type.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Amir Goldstein]

On Thu, May 4, 2017 at 4:34 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, May 04, 2017 at 04:26:17PM +0300, Amir Goldstein wrote:
>> xfs was defining a non namespaced type named uuid_t and for no good
>> reason. xfs code doesn't care about the internals of uuid_t struct -
>> it only cares about its size.
>>
>> Re-define uuid_t as the common struct uuid_v1 in include/linux/uuid.h
>> and get rid of the xfs private definition.
>
> I'm not sure this really is a good idea.  uuid_v1 currently is only
> used by afs.  I'd much rather switch both afs and xfs to use the
> uuid_be type (which might as well grow the standard uuid_t name while
> we're at it), and use accessors that do the byte-array access for
> the very few places that care about the interpretation.
>

I did consider defining uuid_t as uuid_be.
most of the patch set would have remained the same and
xfs_uuid_getnodeuniq() would use struct uuid_v1 explicitly
instead of implicitly.

Bare in mind that we do need to make small steps, so
I wouldn't mix killing uuid_v1 and actors with this review.

Amir.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Christoph Hellwig]

On Thu, May 04, 2017 at 04:57:51PM +0300, Amir Goldstein wrote:
> I did consider defining uuid_t as uuid_be.
> most of the patch set would have remained the same and
> xfs_uuid_getnodeuniq() would use struct uuid_v1 explicitly
> instead of implicitly.

At least don't add new users of uuid_v1.  Moving that stuff into
uuid.[ch] was a major mistake, and I wish review would have caught
it back then.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Amir Goldstein]

On Thu, May 4, 2017 at 4:59 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, May 04, 2017 at 04:57:51PM +0300, Amir Goldstein wrote:
>> I did consider defining uuid_t as uuid_be.
>> most of the patch set would have remained the same and
>> xfs_uuid_getnodeuniq() would use struct uuid_v1 explicitly
>> instead of implicitly.
>
> At least don't add new users of uuid_v1.  Moving that stuff into
> uuid.[ch] was a major mistake, and I wish review would have caught
> it back then.

Fine. I can keep xfs_uu_t for now.
It's not really a part of the effort to move the generic helpers to uuid.h.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Christoph Hellwig]

On Thu, May 04, 2017 at 05:00:34PM +0300, Amir Goldstein wrote:
> On Thu, May 4, 2017 at 4:59 PM, Christoph Hellwig <hch@lst.de> wrote:
> > On Thu, May 04, 2017 at 04:57:51PM +0300, Amir Goldstein wrote:
> >> I did consider defining uuid_t as uuid_be.
> >> most of the patch set would have remained the same and
> >> xfs_uuid_getnodeuniq() would use struct uuid_v1 explicitly
> >> instead of implicitly.
> >
> > At least don't add new users of uuid_v1.  Moving that stuff into
> > uuid.[ch] was a major mistake, and I wish review would have caught
> > it back then.
> 
> Fine. I can keep xfs_uu_t for now.

Please kill it, but instead of using uuid_v1 just use get_unaligned_be*
access to uuid_be.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[David Howells]

Christoph Hellwig <hch@lst.de> wrote:

> I'm not sure this really is a good idea.  uuid_v1 currently is only
> used by afs.  I'd much rather switch both afs and xfs to use the
> uuid_be type (which might as well grow the standard uuid_t name while
> we're at it), and use accessors that do the byte-array access for
> the very few places that care about the interpretation.

Leave struct uuid_v1 as is please.  The AFS protocol XDR encodes the fields as
delineated in the struct:

		r->time_low			= b[0];
		r->time_mid			= htons(ntohl(b[1]));
		r->time_hi_and_version		= htons(ntohl(b[2]));
		r->clock_seq_hi_and_reserved 	= ntohl(b[3]);
		r->clock_seq_low		= ntohl(b[4]);

		for (loop = 0; loop < 6; loop++)
			r->node[loop] = ntohl(b[loop + 5]);

Yeah, I know it's crazy to do it like this on the wire rather than just encode
it as a 16-byte blob, but that's what someone defined it as...  Trying to use
the uuid_be struct instead just makes things more messy.

David

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[Christoph Hellwig]

On Thu, May 04, 2017 at 03:16:10PM +0100, David Howells wrote:
> Leave struct uuid_v1 as is please.  The AFS protocol XDR encodes the fields as
> delineated in the struct:

No.  Use direct decoding of the fields (maybe using helpers) for the
two places in the whole kernel (afs and xfs) that care about they layout,
instead of needing a secondary structure and the related infrastructure.

The uuid_be (really should be uuid_t) and uuid_le (really should be guid_t)
is bad enough.

Re: [PATCH v2 2/8] xfs: re-define uuid_t as common struct uuid_v1

[David Howells]

Christoph Hellwig <hch@lst.de> wrote:

> > Leave struct uuid_v1 as is please.  The AFS protocol XDR encodes the
> > fields as delineated in the struct:
> 
> No.  Use direct decoding of the fields

No.  Structured types exist in C for a reason.  It makes the code easier to
read.  If it really gets your goat, I can move the uuid_v1 struct definition
into AFS code.

David