On Tue, Sep 01, 2020 at 10:13:41AM -0400, Brian Foster wrote:
> On Tue, Sep 01, 2020 at 11:59:19AM +0200, Carlos Maiolino wrote:
> > Hi folks, while working on the attr structs cleanup, I've noticed there
> > are so many places where we do:
> >
> > (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data;
> >
> > So, I thought it would be worth to add another inline function to do
> > this conversion and remove all these casts.
> >
> > To achieve this, it will be required to include xfs_inode.h header on
> > xfs_attr_sf.h, so it can access the xfs_inode definition. Also, if this
> > patch is an acceptable idea, it will make sense then to keep the
> > xfs_attr_sf_totsize() function also inside xfs_attr_sf.h (which has been
> > moved on my series to avoid the additional #include), so, I thought on
> > sending this RFC patch to get comments if it's a good idea or not, and,
> > if it is, I'll add this patch to my series before sending it over.
> >
> > I didn't focus on check if this patch is totally correct (only build
> > test), since my idea is to gather you guys opinions about having this
> > new inline function, so don't bother on reviewing the patch itself by
> > now, only the function name if you guys prefer some other name.
> >
> > Also, this patch is build on top of my clean up series (V2), not yet
> > sent to the list, so it won't apply anyway.
> >
> > Cheers.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 4 ++--
> > fs/xfs/libxfs/xfs_attr_leaf.c | 16 ++++++++--------
> > fs/xfs/libxfs/xfs_attr_sf.h | 6 ++++++
> > fs/xfs/xfs_attr_list.c | 2 +-
> > 4 files changed, 17 insertions(+), 11 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/libxfs/xfs_attr_sf.h b/fs/xfs/libxfs/xfs_attr_sf.h
> > index 540ad3332a9c8..a51aed1dab6c1 100644
> > --- a/fs/xfs/libxfs/xfs_attr_sf.h
> > +++ b/fs/xfs/libxfs/xfs_attr_sf.h
> > @@ -3,6 +3,8 @@
> > * Copyright (c) 2000,2002,2005 Silicon Graphics, Inc.
> > * All Rights Reserved.
> > */
> > +
> > +#include "xfs_inode.h"
>
> FWIW, I thought we tried to avoid including headers from other headers
> like this. I'm also wondering if it's an issue that we'd be including a
> a header that is external to libxfs from a libxfs header. Perhaps this
> could be simplified by passing the xfs_ifork pointer to the new helper
> rather than the xfs_inode and/or moving the helper to
> libxfs/xfs_inode_fork.h and putting a forward declaration of
> xfs_attr_shortform in there..?
If you change if_data to a (void *), all the casts become unnecessary,
right?
--D
> Brian
>
> > #ifndef __XFS_ATTR_SF_H__
> > #define __XFS_ATTR_SF_H__
> >
> > @@ -47,4 +49,8 @@ xfs_attr_sf_nextentry(struct xfs_attr_sf_entry *sfep) {
> > xfs_attr_sf_entsize(sfep));
> > }
> >
> > +static inline struct xfs_attr_shortform *
> > +xfs_attr_ifork_to_sf(struct xfs_inode *ino) {
> > + return (struct xfs_attr_shortform *)ino->i_afp->if_u1.if_data;
> > +}
> > #endif /* __XFS_ATTR_SF_H__ */
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 8f8837fe21cf0..7c0ebdeb43567 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -61,7 +61,7 @@ xfs_attr_shortform_list(
> > int error = 0;
> >
> > ASSERT(dp->i_afp != NULL);
> > - sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data;
> > + sf = xfs_attr_ifork_to_sf(dp);
> > ASSERT(sf != NULL);
> > if (!sf->hdr.count)
> > return 0;
> > --
> > 2.26.2
> >
>
