uuid: Add inline helpers to operate on raw buffers
Subsystems (2)
Andy Shevchenko
reviewerPATCH v2 1/3 uuid: Add inline helpers to operate on raw buffers
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.
--- include/linux/uuid.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
include/linux/uuid.h +40 -0
@@ -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 ];
Signature · 1 line
2.20.1
Andy Shevchenko
reviewerPATCH v2 2/3 Btrfs: Switch to use new generic UUID API
There are new types and helpers that are supposed to be used in new code.
As a preparation to get rid of legacy types and API functions do the conversion here.
--- fs/btrfs/ctree.h | 1 - fs/btrfs/disk-io.c | 6 +++--- fs/btrfs/inode.c | 2 +- fs/btrfs/ioctl.c | 25 +++++++------------------ fs/btrfs/root-tree.c | 4 +--- fs/btrfs/send.c | 6 +++--- fs/btrfs/transaction.c | 9 ++++----- fs/btrfs/volumes.c | 8 ++++---- 8 files changed, 23 insertions(+), 38 deletions(-)
fs/btrfs/ctree.h +0 -1
@@ -3207,7 +3207,6 @@ long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);long btrfs_compat_ioctl (struct file *file,unsigned int cmd,unsigned long arg);int btrfs_ioctl_get_supported_features (void __user *arg);void btrfs_sync_inode_flags_to_i_flags (struct inode *inode); -int btrfs_is_empty_uuid (u8 *uuid);int btrfs_defrag_file (struct inode *inode,struct file *file,struct btrfs_ioctl_defrag_range_args *range,u64 newer_than,unsigned long max_pages);
fs/btrfs/disk-io.c +3 -3
@@ -1259,7 +1259,6 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,struct btrfs_key key;unsigned int nofs_flag;int ret =0 ; -uuid_le uuid =NULL_UUID_LE ;/* * We're holding a transaction handle, so use a NOFS memory allocation
@@ -1299,8 +1298,9 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,btrfs_set_root_last_snapshot (&root->root_item,0 );btrfs_set_root_dirid (&root->root_item,0 );if (is_fstree (objectid)) -uuid_le_gen (&uuid); -memcpy (root->root_item.uuid, uuid.b,BTRFS_UUID_SIZE ); +guid_gen_raw (root->root_item.uuid); +else +guid_copy_to_raw (root->root_item.uuid, &guid_null); root->root_item.drop_level =0 ; key.objectid = objectid;
fs/btrfs/inode.c +1 -1
@@ -4410,7 +4410,7 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry) err = ret; goto out_end_trans; } -if (!btrfs_is_empty_uuid (dest->root_item.received_uuid)) { +if (!guid_is_null_raw (dest->root_item.received_uuid)) { ret = btrfs_uuid_tree_remove(trans, dest->root_item.received_uuid, BTRFS_UUID_KEY_RECEIVED_SUBVOL,
fs/btrfs/ioctl.c +7 -18
@@ -540,17 +540,6 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)return 0 ; } -int btrfs_is_empty_uuid (u8 *uuid) -{ -int i; - -for (i =0 ; i <BTRFS_UUID_SIZE ; i++) { -if (uuid[i]) -return 0 ; - } -return 1 ; -} - static noinline int create_subvol(struct inode *dir, struct dentry *dentry, const char *name, int namelen,
@@ -573,7 +562,6 @@ static noinline int create_subvol(struct inode *dir,u64 objectid;u64 new_dirid =BTRFS_FIRST_FREE_OBJECTID ;u64 index =0 ; -uuid_le new_uuid; root_item =kzalloc (sizeof (*root_item),GFP_KERNEL );if (!root_item)
@@ -643,8 +631,7 @@ static noinline int create_subvol(struct inode *dir,btrfs_set_root_generation_v2 (root_item,btrfs_root_generation (root_item)); -uuid_le_gen (&new_uuid); -memcpy (root_item->uuid, new_uuid.b,BTRFS_UUID_SIZE ); +guid_gen_raw (root_item->uuid);btrfs_set_stack_timespec_sec (&root_item->otime, cur_time.tv_sec);btrfs_set_stack_timespec_nsec (&root_item->otime, cur_time.tv_nsec); root_item->ctime = root_item->otime;
@@ -3170,14 +3157,16 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info, {struct btrfs_ioctl_dev_info_args *di_args;struct btrfs_device *dev; +u8 *s_uuid;int ret =0 ; -char *s_uuid =NULL ; di_args =memdup_user (arg,sizeof (*di_args));if (IS_ERR (di_args))return PTR_ERR (di_args); -if (!btrfs_is_empty_uuid (di_args->uuid)) +if (guid_is_null_raw (di_args->uuid)) + s_uuid =NULL ; +else s_uuid = di_args->uuid;rcu_read_lock ();
@@ -5156,7 +5145,7 @@ static long _btrfs_ioctl_set_received_subvol(struct file *file, received_uuid_changed =memcmp (root_item->received_uuid, sa->uuid,BTRFS_UUID_SIZE );if (received_uuid_changed && - !btrfs_is_empty_uuid (root_item->received_uuid)) { + !guid_is_null_raw (root_item->received_uuid)) { ret =btrfs_uuid_tree_remove (trans, root_item->received_uuid,BTRFS_UUID_KEY_RECEIVED_SUBVOL , root->root_key.objectid);
@@ -5180,7 +5169,7 @@ static long _btrfs_ioctl_set_received_subvol(struct file *file,btrfs_end_transaction (trans); goto out; } -if (received_uuid_changed && !btrfs_is_empty_uuid (sa->uuid)) { +if (received_uuid_changed && !guid_is_null_raw (sa->uuid)) { ret =btrfs_uuid_tree_add (trans, sa->uuid,BTRFS_UUID_KEY_RECEIVED_SUBVOL , root->root_key.objectid);
fs/btrfs/root-tree.c +1 -3
@@ -22,7 +22,6 @@static void btrfs_read_root_item (struct extent_buffer *eb,int slot,struct btrfs_root_item *item) { -uuid_le uuid;u32 len;int need_reset =0 ;
@@ -44,8 +43,7 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,sizeof (*item) - offsetof(struct btrfs_root_item , generation_v2)); -uuid_le_gen (&uuid); -memcpy (item->uuid, uuid.b,BTRFS_UUID_SIZE ); +guid_gen_raw (item->uuid); } }
fs/btrfs/send.c +3 -3
@@ -2361,7 +2361,7 @@ static int send_subvol_begin(struct send_ctx *sctx)TLV_PUT_STRING (sctx,BTRFS_SEND_A_PATH , name, namelen); -if (!btrfs_is_empty_uuid (sctx->send_root->root_item.received_uuid)) +if (!guid_is_null_raw (sctx->send_root->root_item.received_uuid))TLV_PUT_UUID (sctx,BTRFS_SEND_A_UUID , sctx->send_root->root_item.received_uuid);else
@@ -2371,7 +2371,7 @@ static int send_subvol_begin(struct send_ctx *sctx)TLV_PUT_U64 (sctx,BTRFS_SEND_A_CTRANSID ,le64_to_cpu (sctx->send_root->root_item.ctransid));if (parent_root) { -if (!btrfs_is_empty_uuid (parent_root->root_item.received_uuid)) +if (!guid_is_null_raw (parent_root->root_item.received_uuid))TLV_PUT_UUID (sctx,BTRFS_SEND_A_CLONE_UUID , parent_root->root_item.received_uuid);else
@@ -4930,7 +4930,7 @@ static int send_clone(struct send_ctx *sctx,* subvolume and then use that as the parent and try to receive on a * different host. */ -if (!btrfs_is_empty_uuid (clone_root->root->root_item.received_uuid)) +if (!guid_is_null_raw (clone_root->root->root_item.received_uuid))TLV_PUT_UUID (sctx,BTRFS_SEND_A_CLONE_UUID , clone_root->root->root_item.received_uuid);else
fs/btrfs/transaction.c +4 -5
@@ -1395,7 +1395,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,u64 index =0 ;u64 objectid;u64 root_flags; -uuid_le new_uuid;ASSERT (pending->path); path = pending->path;
@@ -1488,8 +1487,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,btrfs_set_root_generation_v2 (new_root_item, trans->transid); -uuid_le_gen (&new_uuid); -memcpy (new_root_item->uuid, new_uuid.b,BTRFS_UUID_SIZE ); +guid_gen_raw (new_root_item->uuid);memcpy (new_root_item->parent_uuid, root->root_item.uuid,BTRFS_UUID_SIZE );if (!(root_flags &BTRFS_ROOT_SUBVOL_RDONLY )) {
@@ -1600,13 +1598,14 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,btrfs_abort_transaction (trans, ret); goto fail; } - ret =btrfs_uuid_tree_add (trans, new_uuid.b,BTRFS_UUID_KEY_SUBVOL , + ret =btrfs_uuid_tree_add (trans, new_root_item->uuid, +BTRFS_UUID_KEY_SUBVOL , objectid);if (ret) {btrfs_abort_transaction (trans, ret); goto fail; } -if (!btrfs_is_empty_uuid (new_root_item->received_uuid)) { +if (!guid_is_null_raw (new_root_item->received_uuid)) { ret =btrfs_uuid_tree_add (trans, new_root_item->received_uuid,BTRFS_UUID_KEY_RECEIVED_SUBVOL , objectid);
fs/btrfs/volumes.c +4 -4
@@ -4469,8 +4469,8 @@ static int btrfs_uuid_scan_kthread(void *data)if (btrfs_root_refs (&root_item) ==0 ) goto skip; -if (!btrfs_is_empty_uuid (root_item.uuid) || - !btrfs_is_empty_uuid (root_item.received_uuid)) { +if (!guid_is_null_raw (root_item.uuid) || + !guid_is_null_raw (root_item.received_uuid)) {if (trans) goto update_tree;
@@ -4489,7 +4489,7 @@ static int btrfs_uuid_scan_kthread(void *data) goto skip; } update_tree: -if (!btrfs_is_empty_uuid (root_item.uuid)) { +if (!guid_is_null_raw (root_item.uuid)) { ret =btrfs_uuid_tree_add (trans, root_item.uuid,BTRFS_UUID_KEY_SUBVOL , key.objectid);
@@ -4500,7 +4500,7 @@ static int btrfs_uuid_scan_kthread(void *data) } } -if (!btrfs_is_empty_uuid (root_item.received_uuid)) { +if (!guid_is_null_raw (root_item.received_uuid)) { ret = btrfs_uuid_tree_add(trans, root_item.received_uuid, BTRFS_UUID_KEY_RECEIVED_SUBVOL,
Signature · 1 line
2.20.1
Andy Shevchenko
reviewerPATCH v2 3/3 uuid: Remove no more needed macro
In reply to Andy Shevchenkouuid_le_gen() is no used anymore, remove it for good.
--- include/linux/uuid.h | 1 - 1 file changed, 1 deletion(-)
include/linux/uuid.h +0 -1
@@ -117,7 +117,6 @@ int guid_parse(const char *uuid, guid_t *u);int uuid_parse (const char *uuid,uuid_t *u);/* backwards compatibility, don't use in new code */ -#define uuid_le_gen (u) guid_gen(u)#define uuid_le_to_bin (guid, u) guid_parse(guid, u)static inline int uuid_le_cmp (const guid_t u1,const guid_t u2)
Signature · 1 line
2.20.1
include/linux/uuid.h static inline void guid_copy(guid_t *dst, const guid_t *src) · line 54
Quoted 9 lines · Andy Shevchenko
include/linux/uuid.h +9
-0+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.
include/linux/uuid.h static inline void uuid_copy(uuid_t *dst, const uuid_t *src) · line 109
Quoted 9 lines
include/linux/uuid.h +9
-0+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.
Andy Shevchenko
reviewerQuoted 12 lines · Christoph Hellwig
include/linux/uuid.h static inline void guid_copy(guid_t *dst, const guid_t *src) · line 54
Quoted 9 lines · Andy Shevchenko
include/linux/uuid.h +9
-0+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.
Quoted 4 lines
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.
Quoted 4 lines
Quoted 4 lines
include/linux/uuid.h +4
-0+static inline void guid_gen_raw (__u8 *guid) +{ +guid_gen ((guid_t *)guid); +}
Quoted 8 lines
include/linux/uuid.h static inline void uuid_copy(uuid_t *dst, const uuid_t *src) · line 109
Quoted 5 lines
include/linux/uuid.h +5
-0+ +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).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 36 lines · Andy Shevchenko
Quoted 12 lines · Christoph Hellwig
include/linux/uuid.h static inline void guid_copy(guid_t *dst, const guid_t *src) · line 54
Quoted 9 lines · Andy Shevchenko
include/linux/uuid.h +9
-0+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.
Quoted 4 lines
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.
Quoted 4 lines
Quoted 4 lines
include/linux/uuid.h +4
-0+static inline void guid_gen_raw (__u8 *guid) +{ +guid_gen ((guid_t *)guid); +}Quoted 8 lines
include/linux/uuid.h static inline void uuid_copy(uuid_t *dst, const uuid_t *src) · line 109
Quoted 5 lines
include/linux/uuid.h +5
-0+ +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.
Quoted 2 lines
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.
Andy Shevchenko
reviewerQuoted 16 lines · David Sterba
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · Christoph Hellwig
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.
Quoted 6 lines
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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 8 lines · David Sterba
Quoted 2 lines
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?
Quoted 2 lines
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.
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · David Sterba
Quoted 2 lines
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.
Quoted 4 lines
So why do you insist on the u8 for the on-disk format?
uuid_tis defined in RFC4122 as a stable format, and one of the two origins of ouruuid_tinfrastructure 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/[email protected]/)
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.
Quoted 5 lines
Quoted 2 lines
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.