| Message ID | 1493904383-2187-3-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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).
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.
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.
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.
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.
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
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.
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
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")
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(-)