Re: [PATCH v2][next] container_of: add container_first() macro
From: Gustavo A. R. Silva
Date: Wed Jan 29 2025 - 18:37:57 EST
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