Re: [PATCH v2][next] container_of: add container_first() macro

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 29/01/25 16:24, Greg KH wrote:
> On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
>> This is like container_of_const() but it contains an assert to
>> ensure that it's using the first member in the structure.
> 
> But why?  If you "know" it's the first member, just do a normal cast.
> If you don't, then you probably shouldn't be caring about this anyway,
> right?

This is more about the cases where the member _must_ be first in the
structure. See below for an example related to -Wflex-array-member-not-at-end

> 
>>
>> Co-developed-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>
>> I will be using this in my -Wflex-array-member-not-at-end patches. :)
> 
> Confused, I'd like to see some users first please.

When addressing the -Wflex-array-member-not-at-end warnings, the common
scenario is when we have to separate the flexible-array member from the
rest of the members in the flexible structure, as shown below [1]:

struct bplus_header
{
         struct_group_tagged(bplus_header_fixed, __hdr,
                 u8 flags;               /* bit 0 - high bit of first free entry offset
                                            bit 5 - we're pointed to by an fnode,
                                            the data btree or some ea or the
                                            main ea bootage pointer ea_secno
                                            bit 6 - suggest binary search (unused)
                                            bit 7 - 1 -> (internal) tree of anodes
                                            0 -> (leaf) list of extents */
                 u8 fill[3];
                 u8 n_free_nodes;        /* free nodes in following array */
                 u8 n_used_nodes;        /* used nodes in following array */
                 __le16 first_free;      /* offset from start of header to
                                            first free node in array */
         );
         union {
                 /* (internal) 2-word entries giving subtree pointers */
                 DECLARE_FLEX_ARRAY(struct bplus_internal_node, internal);
                 /* (external) 3-word entries giving sector runs */
                 DECLARE_FLEX_ARRAY(struct bplus_leaf_node, external);
         } u;
};

struct_group_tagged() creates a new type: `struct bplus_header_fixed`, we
then use the newly created type to change the type of the middle objects
causing the warnings, and with that the warnings are gone:

@@ -453,7 +455,7 @@ struct fnode
    __le16 flags;				/* bit 1 set -> ea_secno is an anode */
  					/* bit 8 set -> directory.  first & only extent
  					   points to dnode. */
-  struct bplus_header btree;		/* b+ tree, 8 extents or 12 subtrees */
+  struct bplus_header_fixed btree;	/* b+ tree, 8 extents or 12 subtrees */
    union {
      struct bplus_leaf_node external[8];
      struct bplus_internal_node internal[12];
@@ -495,7 +497,7 @@ struct anode
    __le32 self;				/* pointer to this anode */
    __le32 up;				/* parent anode or fnode */

-  struct bplus_header btree;		/* b+tree, 40 extents or 60 subtrees */
+  struct bplus_header_fixed btree;	/* b+tree, 40 extents or 60 subtrees */
    union {
      struct bplus_leaf_node external[40];
      struct bplus_internal_node internal[60];

However, this newly created type, or rather the member `__hdr` (also
created when calling struct_group_tagged()) _must_ always be the first
member in the flexible struct `struct bplus_header`.

Then we need to use container_first() to retrieve a pointer to the flexible
structure [2], via which we can access the flexible-array member when
necessary:

diff --git a/fs/hpfs/anode.c b/fs/hpfs/anode.c
index c14c9a035ee0c0..a366f6ac71e436 100644
--- a/fs/hpfs/anode.c
+++ b/fs/hpfs/anode.c
@@ -27,7 +27,7 @@ secno hpfs_bplus_lookup(struct super_block *s, struct inode *inode,
  				a = le32_to_cpu(btree->u.internal[i].down);
  				brelse(bh);
  				if (!(anode = hpfs_map_anode(s, a, &bh))) return -1;
-				btree = &anode->btree;
+				btree = container_first(&anode->btree, struct bplus_header, __hdr);
  				goto go_down;
  			}
  		hpfs_error(s, "sector %08x not found in internal anode %08x", sec, a);
@@ -69,12 +69,16 @@ secno hpfs_add_sector_to_btree(struct super_block *s, secno node, int fnod, unsi
  	int n;
  	unsigned fs;
  	int c1, c2 = 0;
+	struct bplus_header *anode_btree = container_first(&anode->btree, struct bplus_header, __hdr);
+	struct bplus_header *ranode_btree = container_first(&ranode->btree, struct bplus_header, __hdr);
+	struct bplus_header *fnode_btree = container_first(&fnode->btree, struct bplus_header, __hdr);
+

So, if we use container_first() (or container_of_first(), you tell
which name you prefer), we are asserting that that `__hdr` member
is always the first member in `struct bplus_header`.

I've explained all this at Plumbers last year. In any case, let
me know if you need more clarification. :)

Thanks!
--
Gustavo

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/diff/fs/hpfs/hpfs.h?h=testing/wfamnae-next20250124&id=f66219294267a2fba220f4f3118e11c5cda63d0b
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/diff/fs/hpfs/anode.c?h=testing/wfamnae-next20250124&id=f66219294267a2fba220f4f3118e11c5cda63d0b