[PATCH 0/2] Minor macro and type cleanups

[PATCH 0/2] Minor macro and type cleanups

[David Sterba]

Get rid of unsigned char, and use sizeof_filed().

David Sterba (2):
  btrfs: replace unsigned char with u8 for type casts
  btrfs: use helper sizeof_field in struct accessors

 fs/btrfs/accessors.h     | 23 +++++++++++------------
 fs/btrfs/delayed-inode.c |  2 +-
 fs/btrfs/file-item.c     |  8 +++-----
 fs/btrfs/send.c          |  2 +-
 4 files changed, 16 insertions(+), 19 deletions(-)

-- 
2.40.0

[PATCH 1/2] btrfs: replace unsigned char with u8 for type casts

[David Sterba]

There's no reason to use 'unsigned char' when we can simply use u8
for byte buffers like for checksums or send buffers. The char could be
signed or unsigned depending on the compiler settings, per [1] it's not
simple.

[1] https://lore.kernel.org/lkml/CAHk-=wgz3Uba8w7kdXhsqR1qvfemYL+OFQdefJnkeqXG8qZ_pA@mail.gmail.com/

Checksum buffer item already uses u8 so this unifies the types and in
btrfs_readdir_delayed_dir_index() the unsigned char has been probably
inherited from fs_ftype_to_dtype() bit it's not strictly necessary

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/delayed-inode.c | 2 +-
 fs/btrfs/file-item.c     | 8 +++-----
 fs/btrfs/send.c          | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 6b457b010cbc..fd8a9916bf64 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1733,7 +1733,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
 	char *name;
 	int name_len;
 	int over = 0;
-	unsigned char d_type;
+	u8 d_type;
 
 	if (list_empty(ins_list))
 		return 0;
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 696bf695d8eb..acd09cfaf62c 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -245,8 +245,7 @@ btrfs_lookup_csum(struct btrfs_trans_handle *trans,
 		}
 	}
 	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_csum_item);
-	item = (struct btrfs_csum_item *)((unsigned char *)item +
-					  csum_offset * csum_size);
+	item = (struct btrfs_csum_item *)((u8 *)item + csum_offset * csum_size);
 	return item;
 fail:
 	if (ret > 0)
@@ -1223,10 +1222,9 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 	leaf = path->nodes[0];
 csum:
 	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_csum_item);
-	item_end = (struct btrfs_csum_item *)((unsigned char *)item +
+	item_end = (struct btrfs_csum_item *)((u8 *)item +
 				      btrfs_item_size(leaf, path->slots[0]));
-	item = (struct btrfs_csum_item *)((unsigned char *)item +
-					  csum_offset * csum_size);
+	item = (struct btrfs_csum_item *)((u8 *)item + csum_offset * csum_size);
 found:
 	ins_size = (u32)(sums->len - total_bytes) >> fs_info->sectorsize_bits;
 	ins_size *= csum_size;
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 8bfd44750efe..dffdf6c54726 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -796,7 +796,7 @@ static int send_cmd(struct send_ctx *sctx)
 	put_unaligned_le32(sctx->send_size - sizeof(*hdr), &hdr->len);
 	put_unaligned_le32(0, &hdr->crc);
 
-	crc = btrfs_crc32c(0, (unsigned char *)sctx->send_buf, sctx->send_size);
+	crc = btrfs_crc32c(0, (u8 *)sctx->send_buf, sctx->send_size);
 	put_unaligned_le32(crc, &hdr->crc);
 
 	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
-- 
2.40.0

Re: [PATCH 1/2] btrfs: replace unsigned char with u8 for type casts

[Christoph Hellwig]

On Thu, Jul 13, 2023 at 04:10:24PM +0200, David Sterba wrote:
> There's no reason to use 'unsigned char' when we can simply use u8
> for byte buffers like for checksums or send buffers. The char could be
> signed or unsigned depending on the compiler settings, per [1] it's not
> simple.
> 
> [1] https://lore.kernel.org/lkml/CAHk-=wgz3Uba8w7kdXhsqR1qvfemYL+OFQdefJnkeqXG8qZ_pA@mail.gmail.com/

I don't understand how this link is related.  Plain char is indeed
a mess because it could be signed or not.  unsigned char vs u8 is
just plain preference as the latter is always an alias to the former.

u8 tends to look nice to my eyes, but this is a simple cleanup / style
change and not relatded to the above.

> 
> Checksum buffer item already uses u8 so this unifies the types and in
> btrfs_readdir_delayed_dir_index() the unsigned char has been probably
> inherited from fs_ftype_to_dtype() bit it's not strictly necessary

That one isn't used by a cast anyway..

>  	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_csum_item);
> +	item = (struct btrfs_csum_item *)((u8 *)item + csum_offset * csum_size);

This still looks a bit odd to me, because btrfs_csum_item returns the
pointer as the last argument, and the next thing we'd do here is to cast
that away.  If you don't mind using the gcc extention of void pointer
arithmetics that we use all over the kernel this could become:

	item = btrfs_item_ptr(leaf, path->slots[0], void) +
		csum_offset * csum_size;

which I find quite a bit readable.

>  	leaf = path->nodes[0];
>  csum:
>  	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_csum_item);
> -	item_end = (struct btrfs_csum_item *)((unsigned char *)item +
> +	item_end = (struct btrfs_csum_item *)((u8 *)item +
>  				      btrfs_item_size(leaf, path->slots[0]));
> -	item = (struct btrfs_csum_item *)((unsigned char *)item +
> -					  csum_offset * csum_size);
> +	item = (struct btrfs_csum_item *)((u8 *)item + csum_offset * csum_size);

And something similar here.  No point in asking btrfs_item_ptr
to cast to a a btrfs_csum_item, when you then instantly cast it to u8
(or void per the above) for pointer arithmetic.

[PATCH 2/2] btrfs: use helper sizeof_field in struct accessors

[David Sterba]

There's a helper for obtaining size of a struct member, we can use it
instead of open coding the pointer magic.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/accessors.h | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
index ceadfc5d6c66..8cfc8214109c 100644
--- a/fs/btrfs/accessors.h
+++ b/fs/btrfs/accessors.h
@@ -3,6 +3,8 @@
 #ifndef BTRFS_ACCESSORS_H
 #define BTRFS_ACCESSORS_H
 
+#include <linux/stddef.h>
+
 struct btrfs_map_token {
 	struct extent_buffer *eb;
 	char *kaddr;
@@ -34,13 +36,13 @@ static inline void put_unaligned_le8(u8 val, void *p)
 	read_extent_buffer(eb, (char *)(result),			\
 			   ((unsigned long)(ptr)) +			\
 			    offsetof(type, member),			\
-			   sizeof(((type *)0)->member)))
+			    sizeof_field(type, member)))
 
 #define write_eb_member(eb, ptr, type, member, result) (\
 	write_extent_buffer(eb, (char *)(result),			\
 			   ((unsigned long)(ptr)) +			\
 			    offsetof(type, member),			\
-			   sizeof(((type *)0)->member)))
+			    sizeof_field(type, member)))
 
 #define DECLARE_BTRFS_SETGET_BITS(bits)					\
 u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
@@ -62,25 +64,25 @@ DECLARE_BTRFS_SETGET_BITS(64)
 static inline u##bits btrfs_##name(const struct extent_buffer *eb,	\
 				   const type *s)			\
 {									\
-	static_assert(sizeof(u##bits) == sizeof(((type *)0))->member);	\
+	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
 	return btrfs_get_##bits(eb, s, offsetof(type, member));		\
 }									\
 static inline void btrfs_set_##name(const struct extent_buffer *eb, type *s, \
 				    u##bits val)			\
 {									\
-	static_assert(sizeof(u##bits) == sizeof(((type *)0))->member);	\
+	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
 	btrfs_set_##bits(eb, s, offsetof(type, member), val);		\
 }									\
 static inline u##bits btrfs_token_##name(struct btrfs_map_token *token,	\
 					 const type *s)			\
 {									\
-	static_assert(sizeof(u##bits) == sizeof(((type *)0))->member);	\
+	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
 	return btrfs_get_token_##bits(token, s, offsetof(type, member));\
 }									\
 static inline void btrfs_set_token_##name(struct btrfs_map_token *token,\
 					  type *s, u##bits val)		\
 {									\
-	static_assert(sizeof(u##bits) == sizeof(((type *)0))->member);	\
+	static_assert(sizeof(u##bits) == sizeof_field(type, member));	\
 	btrfs_set_token_##bits(token, s, offsetof(type, member), val);	\
 }
 
@@ -111,17 +113,14 @@ static inline void btrfs_set_##name(type *s, u##bits val)		\
 static inline u64 btrfs_device_total_bytes(const struct extent_buffer *eb,
 					   struct btrfs_dev_item *s)
 {
-	static_assert(sizeof(u64) ==
-		      sizeof(((struct btrfs_dev_item *)0))->total_bytes);
-	return btrfs_get_64(eb, s, offsetof(struct btrfs_dev_item,
-					    total_bytes));
+	static_assert(sizeof(u64) == sizeof_field(struct btrfs_dev_item, total_bytes));
+	return btrfs_get_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes));
 }
 static inline void btrfs_set_device_total_bytes(const struct extent_buffer *eb,
 						struct btrfs_dev_item *s,
 						u64 val)
 {
-	static_assert(sizeof(u64) ==
-		      sizeof(((struct btrfs_dev_item *)0))->total_bytes);
+	static_assert(sizeof(u64) == sizeof_field(struct btrfs_dev_item, total_bytes));
 	WARN_ON(!IS_ALIGNED(val, eb->fs_info->sectorsize));
 	btrfs_set_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes), val);
 }
-- 
2.40.0

Re: [PATCH 2/2] btrfs: use helper sizeof_field in struct accessors

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 0/2] Minor macro and type cleanups

[Johannes Thumshirn]