| Message ID | Z5m76U5zh34WbeQY@kspp (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,next] container_of: add container_first() macro | expand |
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? > > 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. thanks, greg k-h
On Wed, Jan 29, 2025 at 06:54:48AM +0100, 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? > Heh. I had a long coversation with someone where I tried to explain four times that casting the first member was a thing. In the end, they were able to accept that it works but only "accidentally." We merged their patch as a cleanup. https://lore.kernel.org/all/20250116193432.716db3a2@gandalf.local.home/ We have quite a bit of code like: drivers/iommu/iommufd/iommufd_private.h 243 static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx, 244 u32 id) 245 { 246 return container_of(iommufd_get_object(ictx, id, 247 IOMMUFD_OBJ_IOAS), 248 struct iommufd_ioas, obj); 249 } It's just a cast like you say, but it looks like pointer math. It would be more readable as container_of_first(). The weird thing is that when people check if (IS_ERR()) on a container_of() then normally the code is correct, but when they check for NULL then normally the NULL check is a harmless no-op. regards, dan carpenter
On Wed, Jan 29, 2025 at 10:29:24AM +0300, Dan Carpenter wrote: > drivers/iommu/iommufd/iommufd_private.h > 243 static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx, > 244 u32 id) > 245 { > 246 return container_of(iommufd_get_object(ictx, id, > 247 IOMMUFD_OBJ_IOAS), > 248 struct iommufd_ioas, obj); > 249 } > > It's just a cast like you say, but it looks like pointer math. It would > be more readable as container_of_first(). I left out the important bit... iommufd_get_object() returns error pointers. It doesn't make sense to pass error pointers to container_of() unless the offset is zero. Smatch could check that the offset is zero, but Coccinelle can't. There are a bunch of advantages to Coccinelle at times so this will improve the readability and make it easier for static checkers as well. regards, dan carpenter
On Wed, Jan 29, 2025 at 10:33:56AM +0300, Dan Carpenter wrote: > On Wed, Jan 29, 2025 at 10:29:24AM +0300, Dan Carpenter wrote: > > drivers/iommu/iommufd/iommufd_private.h > > 243 static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx, > > 244 u32 id) > > 245 { > > 246 return container_of(iommufd_get_object(ictx, id, > > 247 IOMMUFD_OBJ_IOAS), > > 248 struct iommufd_ioas, obj); > > 249 } > > > > It's just a cast like you say, but it looks like pointer math. It would > > be more readable as container_of_first(). > > I left out the important bit... iommufd_get_object() returns error pointers. > It doesn't make sense to pass error pointers to container_of() unless the > offset is zero. Ick, agreed. you should never be checking the return value of container_of(). Shouldn't we somehow check for that? Is there the inverse of __must_check where we "just" want to use the value but never compare it to anything? > Smatch could check that the offset is zero, but Coccinelle can't. There are > a bunch of advantages to Coccinelle at times so this will improve the > readability and make it easier for static checkers as well. But I don't see how container_first() will do anything to help the above type of code. Gustavo, have any examples of what you need this for? thanks, greg k-h
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
On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > 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 That's fine, but that's a build-time issue, you should enforce that in the structure itself, why are you forcing people to remember to use this macro when you want to use the field? There's nothing preventing anyone from using container_of() instead here, and nothing will catch that from what I can tell. > 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: You don't _need_ to use it, you could use container_of() just fine. Which is my point, this is always going to get stale and wrong over time, you are trying to enforce in the .c code when someone access a field that should have been checked in the .h definition. > 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); Ick, so when is anyone going to "know" that container_of() couldn't be used here instead? What is going to enforce this? > 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`. Yes, you are asserting it in the c code, but a global replace of container_first() with container_of() isn't going to do anything if that starts to happen. And a driver/fs author doesn't care here which one matters, they will just use the first one that compiles and runs properly. In other words, nothing is going to enforce this check making it semi-useless over time. Can't you put something in the .h file to "ensure" that the field is first there? Then a normal container_of() can just be used like everyone is used to without having to worry about anything over time. thanks, greg k-h
On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > > > > 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 > > That's fine, but that's a build-time issue, you should enforce that in > the structure itself, why are you forcing people to remember to use this > macro when you want to use the field? There's nothing preventing anyone > from using container_of() instead here, and nothing will catch that from > what I can tell. The new definition has a static_assert() in it so it's enforced about build time. +#define container_first(ptr, type, member) ({ \ + static_assert(offsetof(type, member) == 0, "not first member"); \ + container_of_const(ptr, type, member); }) That was the discussion at plumbers, Gustavo just wanted to use container_of() but I told him I was tired of code which assumes that container_of() is just a cast. If we're going to write code with that assumption then lets create a different macro for it and let's make the build break if someone changes it. regards, dan carpenter
On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote: > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > > > > > > > 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 > > > > That's fine, but that's a build-time issue, you should enforce that in > > the structure itself, why are you forcing people to remember to use this > > macro when you want to use the field? There's nothing preventing anyone > > from using container_of() instead here, and nothing will catch that from > > what I can tell. > > The new definition has a static_assert() in it so it's enforced about > build time. Yes, but that forces you to "know" to do that in the .c file. How do you know to use this, and if you remove it or change it to container_of(), it works just fine again. > +#define container_first(ptr, type, member) ({ \ > + static_assert(offsetof(type, member) == 0, "not first member"); \ > + container_of_const(ptr, type, member); }) > > That was the discussion at plumbers, Gustavo just wanted to use > container_of() but I told him I was tired of code which assumes that > container_of() is just a cast. If we're going to write code with that > assumption then lets create a different macro for it and let's make the > build break if someone changes it. That's fine, but it should be where the variable layout is, NOT where you dereference the pointer as at that point in time, you don't know or care if the location is in the first location at all. thanks, greg k-h
On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote: > On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote: > > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: > > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > > > > > > > > > > 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 > > > > > > That's fine, but that's a build-time issue, you should enforce that in > > > the structure itself, why are you forcing people to remember to use this > > > macro when you want to use the field? There's nothing preventing anyone > > > from using container_of() instead here, and nothing will catch that from > > > what I can tell. > > > > The new definition has a static_assert() in it so it's enforced about > > build time. > > Yes, but that forces you to "know" to do that in the .c file. How do > you know to use this, and if you remove it or change it to > container_of(), it works just fine again. > I guess my use case is different from Gustavo's. For him, using container_of() is fine. We probably don't even need an assert because once you see a struct_group_tagged() then you know the order is important. For me, it's code like I mentioned which does: p = container_of(); if (IS_ERR(p)) ... And I did see you suggest that people re-write that kind of code, but no one is going to do that. :P People know that container_of() is just a cast in that case. It works fine. It's just a bit ugly. regards, dan carpenter
On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote: > On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote: > > On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote: > > > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: > > > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > > > > > > > > > > > > > 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 > > > > > > > > That's fine, but that's a build-time issue, you should enforce that in > > > > the structure itself, why are you forcing people to remember to use this > > > > macro when you want to use the field? There's nothing preventing anyone > > > > from using container_of() instead here, and nothing will catch that from > > > > what I can tell. > > > > > > The new definition has a static_assert() in it so it's enforced about > > > build time. > > > > Yes, but that forces you to "know" to do that in the .c file. How do > > you know to use this, and if you remove it or change it to > > container_of(), it works just fine again. > > > > I guess my use case is different from Gustavo's. For him, using > container_of() is fine. We probably don't even need an assert because > once you see a struct_group_tagged() then you know the order is important. Who knows this? The developer? What are they supposed to "know" here? I sure don't :) Having some sort of "__must_be_first" marking for a field is fine and break the build if that doesn't happen. Otherwise this is something that is not going to be used properly over time. > For me, it's code like I mentioned which does: > > p = container_of(); > if (IS_ERR(p)) > ... > > And I did see you suggest that people re-write that kind of code, but no > one is going to do that. :P People know that container_of() is just a > cast in that case. It works fine. It's just a bit ugly. It doesn't work if the field isn't first, so no, it shouldn't be working fine, and that should be flagged and fixed and never allowed to come back again. Can't we do that with coccinelle? thanks, greg k-h
On 30/01/25 02:38, Greg KH wrote: > On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote: >> On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote: >>> On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote: >>>> On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: >>>>> On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: >>>>>> >>>>>> >>>>>> 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 >>>>> >>>>> That's fine, but that's a build-time issue, you should enforce that in >>>>> the structure itself, why are you forcing people to remember to use this >>>>> macro when you want to use the field? There's nothing preventing anyone >>>>> from using container_of() instead here, and nothing will catch that from >>>>> what I can tell. >>>> >>>> The new definition has a static_assert() in it so it's enforced about >>>> build time. >>> >>> Yes, but that forces you to "know" to do that in the .c file. How do >>> you know to use this, and if you remove it or change it to >>> container_of(), it works just fine again. >>> >> >> I guess my use case is different from Gustavo's. For him, using >> container_of() is fine. We probably don't even need an assert because >> once you see a struct_group_tagged() then you know the order is important. > > Who knows this? The developer? What are they supposed to "know" here? > I sure don't :) > > Having some sort of "__must_be_first" marking for a field is fine and > break the build if that doesn't happen. Otherwise this is something > that is not going to be used properly over time. I'm currently dealing with this situation in the following way: struct libipw_hdr_3addr { - __le16 frame_ctl; - __le16 duration_id; - u8 addr1[ETH_ALEN]; - u8 addr2[ETH_ALEN]; - u8 addr3[ETH_ALEN]; - __le16 seq_ctl; + /* New members MUST be added within the __struct_group() macro below. */ + __struct_group(libipw_hdr_3addr_hdr, hdr, __packed, + __le16 frame_ctl; + __le16 duration_id; + u8 addr1[ETH_ALEN]; + u8 addr2[ETH_ALEN]; + u8 addr3[ETH_ALEN]; + __le16 seq_ctl; + ); u8 payload[]; } __packed; +static_assert(offsetof(struct libipw_hdr_3addr, payload) == sizeof(struct libipw_hdr_3addr_hdr), + "struct member likely outside of __struct_group()"); "We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use `static_assert()`. This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes." [1] We could probably create struct_group_first() instead of container_first(). Anyways, so far so good. For some reason I was under the impression that you two guys wanted the container_first() macro. OK, that's it from my side. Have a good one! -Gustavo [1] https://git.kernel.org/linus/089332e703b681c8 > >> For me, it's code like I mentioned which does: >> >> p = container_of(); >> if (IS_ERR(p)) >> ... >> >> And I did see you suggest that people re-write that kind of code, but no >> one is going to do that. :P People know that container_of() is just a >> cast in that case. It works fine. It's just a bit ugly. > > It doesn't work if the field isn't first, so no, it shouldn't be working > fine, and that should be flagged and fixed and never allowed to come > back again. Can't we do that with coccinelle? > > thanks, > > greg k-h >
On Thu, Jan 30, 2025 at 10:06:00AM +1030, Gustavo A. R. Silva wrote: > > > On 30/01/25 02:38, Greg KH wrote: > > On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote: > > > On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote: > > > > On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote: > > > > > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote: > > > > > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote: > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > That's fine, but that's a build-time issue, you should enforce that in > > > > > > the structure itself, why are you forcing people to remember to use this > > > > > > macro when you want to use the field? There's nothing preventing anyone > > > > > > from using container_of() instead here, and nothing will catch that from > > > > > > what I can tell. > > > > > > > > > > The new definition has a static_assert() in it so it's enforced about > > > > > build time. > > > > > > > > Yes, but that forces you to "know" to do that in the .c file. How do > > > > you know to use this, and if you remove it or change it to > > > > container_of(), it works just fine again. > > > > > > > > > > I guess my use case is different from Gustavo's. For him, using > > > container_of() is fine. We probably don't even need an assert because > > > once you see a struct_group_tagged() then you know the order is important. > > > > Who knows this? The developer? What are they supposed to "know" here? > > I sure don't :) > > > > Having some sort of "__must_be_first" marking for a field is fine and > > break the build if that doesn't happen. Otherwise this is something > > that is not going to be used properly over time. > > I'm currently dealing with this situation in the following way: > > struct libipw_hdr_3addr { > - __le16 frame_ctl; > - __le16 duration_id; > - u8 addr1[ETH_ALEN]; > - u8 addr2[ETH_ALEN]; > - u8 addr3[ETH_ALEN]; > - __le16 seq_ctl; > + /* New members MUST be added within the __struct_group() macro below. */ > + __struct_group(libipw_hdr_3addr_hdr, hdr, __packed, > + __le16 frame_ctl; > + __le16 duration_id; > + u8 addr1[ETH_ALEN]; > + u8 addr2[ETH_ALEN]; > + u8 addr3[ETH_ALEN]; > + __le16 seq_ctl; > + ); > u8 payload[]; > } __packed; > +static_assert(offsetof(struct libipw_hdr_3addr, payload) == sizeof(struct libipw_hdr_3addr_hdr), > + "struct member likely outside of __struct_group()"); > > "We also want to ensure that when new members need to be added to the > flexible structure, they are always included within the newly created > tagged struct. For this, we use `static_assert()`. This ensures that the > memory layout for both the flexible structure and the new tagged struct > is the same after any changes." [1] That's nice, much better and it is right where the structure is defined making it more likely to stay there over time. > We could probably create struct_group_first() instead of container_first(). Like this? #define struct_group_first(s, n) static_assert(offsetof(s, n) == sizeof(s), "struct member likely outside of __struct_group(), please fix!") And then the above would be: struct_group_fist(struct libipw_hdr_3addr, payload); Hm, use of "payload" here feels odd but you get the idea. > Anyways, so far so good. For some reason I was under the impression > that you two guys wanted the container_first() macro. I don't :) thanks, greg k-h
On Thu, Jan 30, 2025 at 07:14:01AM +0100, Greg KH wrote: > > Anyways, so far so good. For some reason I was under the impression > > that you two guys wanted the container_first() macro. > > I don't :) Greg, container_first() is like peeing in the shower. Once you try it you'll never go back. :P Anyway, it's not really related to the work you're doing here so don't worry about it. regards, dan carpenter
diff --git a/include/linux/container_of.h b/include/linux/container_of.h index 713890c867be..1a5e5f32db92 100644 --- a/include/linux/container_of.h +++ b/include/linux/container_of.h @@ -35,4 +35,15 @@ default: ((type *)container_of(ptr, type, member)) \ ) +/** + * container_first - cast first member of a structure out to the containing + * structure and preserve the const-ness of the pointer. + * @ptr: the pointer to the member. + * @type: the type of the container struct this is embedded in. + * @member: the name of the member within the struct. + */ +#define container_first(ptr, type, member) ({ \ + static_assert(offsetof(type, member) == 0, "not first member"); \ + container_of_const(ptr, type, member); }) + #endif /* _LINUX_CONTAINER_OF_H */