| Message ID | 20190716150418.84018-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] uuid: Add inline helpers to operate on raw buffers | expand |
On Tue, Jul 16, 2019 at 06:04:16PM +0300, Andy Shevchenko wrote: > +static inline void guid_copy_from_raw(guid_t *dst, const __u8 *src) > +{ > + memcpy(dst, (const guid_t *)src, sizeof(guid_t)); > +} > + > +static inline void guid_copy_to_raw(__u8 *dst, const guid_t *src) > +{ > + memcpy((guid_t *)dst, src, sizeof(guid_t)); > +} Maybe import_guid/export_guid is a better name? Either way, I don't think we need the casts, and they probably want kerneldoc comments describing their use. Same for the uuid side. > +static inline void guid_gen_raw(__u8 *guid) > +{ > + guid_gen((guid_t *)guid); > +} > + > +static inline void uuid_gen_raw(__u8 *uuid) > +{ > + uuid_gen((uuid_t *)uuid); > +} I hate this raw naming. If people really want to use the generators on u8 fields a cast seems more descriptive then hiding it.
On Tue, Jul 16, 2019 at 05:11:33PM +0200, Christoph Hellwig wrote: > On Tue, Jul 16, 2019 at 06:04:16PM +0300, Andy Shevchenko wrote: > > +static inline void guid_copy_from_raw(guid_t *dst, const __u8 *src) > > +{ > > + memcpy(dst, (const guid_t *)src, sizeof(guid_t)); > > +} > > + > > +static inline void guid_copy_to_raw(__u8 *dst, const guid_t *src) > > +{ > > + memcpy((guid_t *)dst, src, sizeof(guid_t)); > > +} > > Maybe import_guid/export_guid is a better name? Yes, sounds good to me. > Either way, I don't think we need the casts, and they probably want > kerneldoc comments describing their use. > > Same for the uuid side. Got it. > > +static inline void guid_gen_raw(__u8 *guid) > > +{ > > + guid_gen((guid_t *)guid); > > +} > > + > > +static inline void uuid_gen_raw(__u8 *uuid) > > +{ > > + uuid_gen((uuid_t *)uuid); > > +} > > I hate this raw naming. If people really want to use the generators on > u8 fields a cast seems more descriptive then hiding it. This entire patch because of BTRFS maintainers, they didn't want the explicit casts. Maybe something has been changed, I dunno. Perhaps, you can sell them the point somehow. (everybody else is using a cast in the kernel).
On Tue, Jul 16, 2019 at 06:22:22PM +0300, Andy Shevchenko wrote: > On Tue, Jul 16, 2019 at 05:11:33PM +0200, Christoph Hellwig wrote: > > On Tue, Jul 16, 2019 at 06:04:16PM +0300, Andy Shevchenko wrote: > > > +static inline void guid_copy_from_raw(guid_t *dst, const __u8 *src) > > > +{ > > > + memcpy(dst, (const guid_t *)src, sizeof(guid_t)); > > > +} > > > + > > > +static inline void guid_copy_to_raw(__u8 *dst, const guid_t *src) > > > +{ > > > + memcpy((guid_t *)dst, src, sizeof(guid_t)); > > > +} > > > > Maybe import_guid/export_guid is a better name? > > Yes, sounds good to me. > > > Either way, I don't think we need the casts, and they probably want > > kerneldoc comments describing their use. > > > > Same for the uuid side. > > Got it. > > > > +static inline void guid_gen_raw(__u8 *guid) > > > +{ > > > + guid_gen((guid_t *)guid); > > > +} > > > + > > > +static inline void uuid_gen_raw(__u8 *uuid) > > > +{ > > > + uuid_gen((uuid_t *)uuid); > > > +} > > > > I hate this raw naming. If people really want to use the generators on > > u8 fields a cast seems more descriptive then hiding it. > > This entire patch because of BTRFS maintainers, they didn't want the explicit > casts. Maybe something has been changed, I dunno. No change on our side. The uuids are u8 in the on-disk structures, that will stay. The uuid functions use a different type so the casts have to be added, that's clear. The question is if it's up to the API to provide functions that take u8, or btrfs code to put typecasts everywhere or carry own wrappers that do that. I tend to avoid the explicit typecasts for widely used functions because it's easy to forget them, and it overrides the type checks (that could be caught by compiler but also not). Specifically for uuid, the endianness might matter, so that we use the raw buffers makes things more explicit.
On Wed, Jul 17, 2019 at 05:37:06PM +0200, David Sterba wrote: > On Tue, Jul 16, 2019 at 06:22:22PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 16, 2019 at 05:11:33PM +0200, Christoph Hellwig wrote: > > > On Tue, Jul 16, 2019 at 06:04:16PM +0300, Andy Shevchenko wrote: > > > I hate this raw naming. If people really want to use the generators on > > > u8 fields a cast seems more descriptive then hiding it. > > > > This entire patch because of BTRFS maintainers, they didn't want the explicit > > casts. Maybe something has been changed, I dunno. > > No change on our side. The uuids are u8 in the on-disk structures, that > will stay. The uuid functions use a different type so the casts have to > be added, that's clear. The question is if it's up to the API to provide > functions that take u8, or btrfs code to put typecasts everywhere or > carry own wrappers that do that. > > I tend to avoid the explicit typecasts for widely used functions because > it's easy to forget them, and it overrides the type checks (that could > be caught by compiler but also not). > > Specifically for uuid, the endianness might matter, so that we use the > raw buffers makes things more explicit. Thank you for the information. Can you review v3 of the series where I attempted to satisfy everybody, Chris in his wish to not do ugly raw generators and Btrfs by providing minimum needed API helpers?
On Wed, Jul 17, 2019 at 05:37:06PM +0200, David Sterba wrote: > > This entire patch because of BTRFS maintainers, they didn't want the explicit > > casts. Maybe something has been changed, I dunno. > > No change on our side. The uuids are u8 in the on-disk structures, that > will stay. The uuid functions use a different type so the casts have to > be added, that's clear. The question is if it's up to the API to provide > functions that take u8, or btrfs code to put typecasts everywhere or > carry own wrappers that do that. So why do you insist on the u8 for the on-disk format? uuid_t is defined in RFC4122 as a stable format, and one of the two origins of our uuid_t infrastructure is the XFS code, where it is used for the on-disk format. What is different in btrfs? > Specifically for uuid, the endianness might matter, so that we use the > raw buffers makes things more explicit. u8 arrays hide the endianess, while the RFC4122 UUID is very clearly defined as having big endian fields where they are bigger than a byte.
On Thu, Jul 18, 2019 at 07:39:51AM +0200, Christoph Hellwig wrote: > On Wed, Jul 17, 2019 at 05:37:06PM +0200, David Sterba wrote: > > > This entire patch because of BTRFS maintainers, they didn't want the explicit > > > casts. Maybe something has been changed, I dunno. > > > > No change on our side. The uuids are u8 in the on-disk structures, that > > will stay. The uuid functions use a different type so the casts have to > > be added, that's clear. The question is if it's up to the API to provide > > functions that take u8, or btrfs code to put typecasts everywhere or > > carry own wrappers that do that. > > So why do you insist on the u8 for the on-disk format? uuid_t is > defined in RFC4122 as a stable format, and one of the two origins of > our uuid_t infrastructure is the XFS code, where it is used for the > on-disk format. What is different in btrfs? As I replied to v1 (https://lore.kernel.org/linux-btrfs/20190121181841.GJ2900@twin.jikos.cz/) I like the simple types in the on-disk structure definitions and that the amount of bytes occupied by the member is obvious. Use of guid_t would need to include linux/uuid.h in the UAPI btrfs headers (that now only include linux/types.h and linux/ioctl.h). This pollutes the namespace as there's also the user-space uuid library that provides the same type, so I can't say that's totally safe. > > Specifically for uuid, the endianness might matter, so that we use the > > raw buffers makes things more explicit. > > u8 arrays hide the endianess, while the RFC4122 UUID is very clearly > defined as having big endian fields where they are bigger than a byte. So we'll use the proper accessors for the raw buffer that's by definition of btrfs format in little endian order, like cpu_to_le and similar. I really try to see what the uuid/guid types would bring but so far see only problems we don't have and the remaining reason is a matter of style/preference/consistency. If you are concerned about uuid API cleanness then we can add the helpers to btrfs. I offered that as an option before, but pushing a typedef to on-disk structures does not feel right given what we have now.
diff --git a/include/linux/uuid.h b/include/linux/uuid.h index 0c631e2a73b6..b8e431d65222 100644 --- a/include/linux/uuid.h +++ b/include/linux/uuid.h @@ -43,11 +43,26 @@ static inline void guid_copy(guid_t *dst, const guid_t *src) memcpy(dst, src, sizeof(guid_t)); } +static inline void guid_copy_from_raw(guid_t *dst, const __u8 *src) +{ + memcpy(dst, (const guid_t *)src, sizeof(guid_t)); +} + +static inline void guid_copy_to_raw(__u8 *dst, const guid_t *src) +{ + memcpy((guid_t *)dst, src, sizeof(guid_t)); +} + static inline bool guid_is_null(const guid_t *guid) { return guid_equal(guid, &guid_null); } +static inline bool guid_is_null_raw(const __u8 *guid) +{ + return guid_equal((const guid_t *)guid, &guid_null); +} + static inline bool uuid_equal(const uuid_t *u1, const uuid_t *u2) { return memcmp(u1, u2, sizeof(uuid_t)) == 0; @@ -58,16 +73,41 @@ static inline void uuid_copy(uuid_t *dst, const uuid_t *src) memcpy(dst, src, sizeof(uuid_t)); } +static inline void uuid_copy_from_raw(uuid_t *dst, const __u8 *src) +{ + memcpy(dst, (const uuid_t *)src, sizeof(uuid_t)); +} + +static inline void uuid_copy_to_raw(__u8 *dst, const uuid_t *src) +{ + memcpy((uuid_t *)dst, src, sizeof(uuid_t)); +} + static inline bool uuid_is_null(const uuid_t *uuid) { return uuid_equal(uuid, &uuid_null); } +static inline bool uuid_is_null_raw(const __u8 *uuid) +{ + return uuid_equal((const uuid_t *)uuid, &uuid_null); +} + void generate_random_uuid(unsigned char uuid[16]); extern void guid_gen(guid_t *u); extern void uuid_gen(uuid_t *u); +static inline void guid_gen_raw(__u8 *guid) +{ + guid_gen((guid_t *)guid); +} + +static inline void uuid_gen_raw(__u8 *uuid) +{ + uuid_gen((uuid_t *)uuid); +} + bool __must_check uuid_is_valid(const char *uuid); extern const u8 guid_index[16];
Sometimes we may need to copy UUID from or to the raw buffer, which is provided outside of kernel and can't be declared as UUID type. With current API this operation will require an explicit casting to one of UUID types and length, that is always a constant derived as sizeof of the certain UUID type. Provide a helpful set of inline helpers to minimize developer's effort in the cases when raw buffers are involved. Suggested-by: David Sterba <dsterba@suse.cz> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/uuid.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)