* [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers
@ 2019-07-16 15:04 Andy Shevchenko
2019-07-16 15:04 ` [PATCH v2 2/3] Btrfs: Switch to use new generic UUID API Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andy Shevchenko @ 2019-07-16 15:04 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs,
Christoph Hellwig
Cc: Andy Shevchenko, David Sterba
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(+)
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];
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 2/3] Btrfs: Switch to use new generic UUID API 2019-07-16 15:04 [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Andy Shevchenko @ 2019-07-16 15:04 ` Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 3/3] uuid: Remove no more needed macro Andy Shevchenko 2019-07-16 15:11 ` [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Christoph Hellwig 2 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2019-07-16 15:04 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs, Christoph Hellwig Cc: Andy Shevchenko 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. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- 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(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 299e11e6c554..1fa396c0478e 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -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); diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 41a2bd2e0c56..9a3dd918b8dd 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -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; diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 1af069a9a0c7..9520d19234ee 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -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, diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 818f7ec8bb0e..f9238d213079 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -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); diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index 47733fb55df7..7bc7a504fdc8 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -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); } } diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 69b59bf75882..b681e2d523f0 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -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 diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 3b8ae1a8f02d..40313f895ee1 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -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); diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index a13ddba1ebc3..90072f53ad9c 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -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, -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] uuid: Remove no more needed macro 2019-07-16 15:04 [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 2/3] Btrfs: Switch to use new generic UUID API Andy Shevchenko @ 2019-07-16 15:04 ` Andy Shevchenko 2019-07-16 15:11 ` [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Christoph Hellwig 2 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2019-07-16 15:04 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs, Christoph Hellwig Cc: Andy Shevchenko uuid_le_gen() is no used anymore, remove it for good. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/uuid.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/uuid.h b/include/linux/uuid.h index b8e431d65222..7efa86a1b588 100644 --- a/include/linux/uuid.h +++ b/include/linux/uuid.h @@ -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) -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-16 15:04 [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 2/3] Btrfs: Switch to use new generic UUID API Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 3/3] uuid: Remove no more needed macro Andy Shevchenko @ 2019-07-16 15:11 ` Christoph Hellwig 2019-07-16 15:22 ` Andy Shevchenko 2 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2019-07-16 15:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs, Christoph Hellwig, David Sterba 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-16 15:11 ` [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Christoph Hellwig @ 2019-07-16 15:22 ` Andy Shevchenko 2019-07-17 15:37 ` David Sterba 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2019-07-16 15:22 UTC (permalink / raw) To: Christoph Hellwig Cc: Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs, David Sterba 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). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-16 15:22 ` Andy Shevchenko @ 2019-07-17 15:37 ` David Sterba 2019-07-17 15:53 ` Andy Shevchenko 2019-07-18 5:39 ` Christoph Hellwig 0 siblings, 2 replies; 9+ messages in thread From: David Sterba @ 2019-07-17 15:37 UTC (permalink / raw) To: Andy Shevchenko Cc: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-17 15:37 ` David Sterba @ 2019-07-17 15:53 ` Andy Shevchenko 2019-07-18 5:39 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2019-07-17 15:53 UTC (permalink / raw) To: dsterba, Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs 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? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-17 15:37 ` David Sterba 2019-07-17 15:53 ` Andy Shevchenko @ 2019-07-18 5:39 ` Christoph Hellwig 2019-07-18 17:52 ` David Sterba 1 sibling, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2019-07-18 5:39 UTC (permalink / raw) To: dsterba, Andy Shevchenko, Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers 2019-07-18 5:39 ` Christoph Hellwig @ 2019-07-18 17:52 ` David Sterba 0 siblings, 0 replies; 9+ messages in thread From: David Sterba @ 2019-07-18 17:52 UTC (permalink / raw) To: Christoph Hellwig Cc: dsterba, Andy Shevchenko, Chris Mason, Josef Bacik, David Sterba, Lu Fengqi, linux-btrfs 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-18 17:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-16 15:04 [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 2/3] Btrfs: Switch to use new generic UUID API Andy Shevchenko 2019-07-16 15:04 ` [PATCH v2 3/3] uuid: Remove no more needed macro Andy Shevchenko 2019-07-16 15:11 ` [PATCH v2 1/3] uuid: Add inline helpers to operate on raw buffers Christoph Hellwig 2019-07-16 15:22 ` Andy Shevchenko 2019-07-17 15:37 ` David Sterba 2019-07-17 15:53 ` Andy Shevchenko 2019-07-18 5:39 ` Christoph Hellwig 2019-07-18 17:52 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox