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

[Greg KH <gregkh@linuxfoundation.org>]

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