Re: [RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Brian Foster <bfoster@redhat.com>]

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@redhat.com>
> ---
>  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
>