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

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

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
>