Re: [PATCH v4 03/17] xfs: Embed struct xfs_name in xfs_da_args

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Nov 06, 2019 at 06:27:47PM -0700, Allison Collins wrote:
> This patch embeds an xfs_name in xfs_da_args, replacing the name,
> namelen, and flags members.  This helps to clean up the xfs_da_args
> structure and make it more uniform with the new xfs_name parameter
> being passed around.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c        |  36 +++++++-------
>  fs/xfs/libxfs/xfs_attr_leaf.c   | 106 +++++++++++++++++++++-------------------
>  fs/xfs/libxfs/xfs_attr_remote.c |   2 +-
>  fs/xfs/libxfs/xfs_da_btree.c    |   5 +-
>  fs/xfs/libxfs/xfs_da_btree.h    |   4 +-
>  fs/xfs/libxfs/xfs_dir2.c        |  18 +++----
>  fs/xfs/libxfs/xfs_dir2_block.c  |   6 +--
>  fs/xfs/libxfs/xfs_dir2_leaf.c   |   6 +--
>  fs/xfs/libxfs/xfs_dir2_node.c   |   8 +--
>  fs/xfs/libxfs/xfs_dir2_sf.c     |  30 ++++++------
>  fs/xfs/scrub/attr.c             |  12 ++---
>  fs/xfs/xfs_trace.h              |  20 ++++----
>  12 files changed, 128 insertions(+), 125 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 5a9624a..b77b985 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -72,13 +72,12 @@ xfs_attr_args_init(
>  	args->geo = dp->i_mount->m_attr_geo;
>  	args->whichfork = XFS_ATTR_FORK;
>  	args->dp = dp;
> -	args->flags = flags;
> -	args->name = name->name;
> -	args->namelen = name->len;
> -	if (args->namelen >= MAXNAMELEN)
> +	name->type = flags;

Is there a purpose for modifying the caller's @name, instead of setting
args.name.type = flags after the memcpy?

> +	memcpy(&args->name, name, sizeof(struct xfs_name));
> +	if (args->name.len >= MAXNAMELEN)
>  		return -EFAULT;		/* match IRIX behaviour */
>  
> -	args->hashval = xfs_da_hashname(args->name, args->namelen);
> +	args->hashval = xfs_da_hashname(args->name.name, args->name.len);
>  	return 0;
>  }
>  
> @@ -236,7 +235,7 @@ xfs_attr_try_sf_addname(
>  	 * Commit the shortform mods, and we're done.
>  	 * NOTE: this is also the error path (EEXIST, etc).
>  	 */
> -	if (!error && (args->flags & ATTR_KERNOTIME) == 0)
> +	if (!error && (args->name.type & ATTR_KERNOTIME) == 0)
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>  
>  	if (mp->m_flags & XFS_MOUNT_WSYNC)
> @@ -357,6 +356,9 @@ xfs_attr_set(
>  	if (error)
>  		return error;
>  
> +	/* Use name now stored in args */
> +	name = &args.name;

You could probably set this as part of the variable declaration, e.g.

struct xfs_name		*name = &args.name;

> +
>  	args.value = value;
>  	args.valuelen = valuelen;
>  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> @@ -372,7 +374,7 @@ xfs_attr_set(
>  	 */
>  	if (XFS_IFORK_Q(dp) == 0) {
>  		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
> -			XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen, valuelen);
> +			XFS_ATTR_SF_ENTSIZE_BYNAME(args.name.len, valuelen);

If you're going to keep the convenience variable @name, then please use
it throughout the function.

>  
>  		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
>  		if (error)

<snip>

> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 4fd1223..129ec09 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -2040,8 +2040,9 @@ xfs_da_compname(
>  	const unsigned char *name,
>  	int		len)
>  {
> -	return (args->namelen == len && memcmp(args->name, name, len) == 0) ?
> -					XFS_CMP_EXACT : XFS_CMP_DIFFERENT;

Wow that's gross. :)

	if (args->name.len == len && !memcmp(args->name.name, name, len))
		return XFS_CMP_EXACT;

	return XFS_CMP_DIFFERENT;

Hmm, is that better?

The rest looks reasonable...

--D