On Wed, Dec 14, 2022 at 11:52:37AM +1100, Dave Chinner wrote:
> On Tue, Dec 13, 2022 at 08:54:45PM +0000, Matthew Wilcox (Oracle) wrote:
> > With a container_of() that preserves const, the compiler warns about
> > all these places which are currently casting away the const. For
> > the IUL_ITEM() helper, we want to also make it const-preserving,
> > and in every other case, we want to just add a const qualifier.
>
> ....
>
> > diff --git a/fs/xfs/xfs_iunlink_item.c b/fs/xfs/xfs_iunlink_item.c
> > index 43005ce8bd48..ff82a93f8a24 100644
> > --- a/fs/xfs/xfs_iunlink_item.c
> > +++ b/fs/xfs/xfs_iunlink_item.c
> > @@ -20,10 +20,7 @@
> >
> > struct kmem_cache *xfs_iunlink_cache;
> >
> > -static inline struct xfs_iunlink_item *IUL_ITEM(struct xfs_log_item *lip)
> > -{
> > - return container_of(lip, struct xfs_iunlink_item, item);
> > -}
> > +#define IUL_ITEM(lip) container_of(lip, struct xfs_iunlink_item, item)
>
> I think this is somewhat of a step backwards. We moved these log
> item type conversions from macros to static inlines to add type
> checking so the compiler would catch the type conversion bugs we
> found that the macros didn't warn about....
But container_of() does warn about bad types. It doesn't check that
'lip' is an xfs_log_item pointer, of course, but it does check that
either lip has the same type as the member 'item' in
struct xfs_iunlink_item, or lip is a void pointer, which is the
same check that the compiler is going to do with a static inline
function.
> Which makes me ask: why do we even care about const here? What
> actual real world problem are you trying to fix with these changes?
Why do we care about const anywhere? container_of() is an unintended
way to drop the constness of a pointer, so it's a code hygeine issue.
