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..?
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
>
