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

[Greg KH <gregkh@linuxfoundation.org>]

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