Re: [PATCH v2 02/18] xfs: Replace attribute parameters with struct xfs_name

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

On Fri, Aug 09, 2019 at 02:37:10PM -0700, Allison Collins wrote:
> This patch replaces the attribute name, length and flags parameters with a
> single struct xfs_name parameter.  This helps to clean up the numbers of
> parameters being passed around and pre-simplifies the code some.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 40 ++++++++++++++++------------------------
>  fs/xfs/libxfs/xfs_attr.h | 12 +++++-------
>  fs/xfs/xfs_acl.c         | 26 +++++++++++++-------------
>  fs/xfs/xfs_ioctl.c       | 26 ++++++++++++++++----------
>  fs/xfs/xfs_iops.c        | 10 ++++++----
>  fs/xfs/xfs_xattr.c       | 21 +++++++++------------
>  6 files changed, 65 insertions(+), 70 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 7761925..0c91116 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -61,9 +61,7 @@ STATIC int
>  xfs_attr_args_init(
>  	struct xfs_da_args	*args,
>  	struct xfs_inode	*dp,
> -	const unsigned char	*name,
> -	size_t			namelen,
> -	int			flags)
> +	struct xfs_name		*name)
>  {
>  
>  	if (!name)
> @@ -73,9 +71,9 @@ 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;
> -	args->namelen = namelen;
> +	args->flags = name->type;
> +	args->name = name->name;
> +	args->namelen = name->len;

/me wonders if you ought to just have an xfs_name embedded in the
xfs_da_args struct, seeing as the directory code uses it too.

>  	if (args->namelen >= MAXNAMELEN)
>  		return -EFAULT;		/* match IRIX behaviour */
>  

<skipping a bunch of changes that look fine to me>

> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b1b7b1b..bdf925c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -46,13 +46,15 @@ xfs_initxattrs(
>  {
>  	const struct xattr	*xattr;
>  	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_name		name;
>  	int			error = 0;
>  
>  	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> -		error = xfs_attr_set(ip, xattr->name,
> -				     strlen(xattr->name),
> -				     xattr->value, xattr->value_len,
> -				     ATTR_SECURE);
> +		name.name = xattr->name;
> +		name.len = strlen(xattr->name);
> +		name.type = ATTR_SECURE;

You might as well declare and initialize name in the loop body.

> +		error = xfs_attr_set(ip, &name,
> +				     xattr->value, xattr->value_len);

Does the attr value need a similar structure encapsulation too?

>  		if (error < 0)
>  			break;
>  	}
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index fe12d11..3c63930 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -20,18 +20,17 @@ static int
>  xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
>  		struct inode *inode, const char *name, void *value, size_t size)
>  {
> -	int xflags = handler->flags;
>  	struct xfs_inode *ip = XFS_I(inode);
>  	int error, asize = size;
> -	size_t namelen = strlen(name);
> +	struct xfs_name xname = {name, strlen(name), handler->flags};

I think the preferred format for this is to use the structure field
names explicity during assignment...

	struct xfs_name		xname = {
		.name		= name,
		.namelen	= strlen(name),
		.type		= handler->flags,
	};

Also, please fix the variable/argument declaration indentation style of
this function to match the rest of xfs. :)

--D

>  
>  	/* Convert Linux syscall to XFS internal ATTR flags */
>  	if (!size) {
> -		xflags |= ATTR_KERNOVAL;
> +		xname.type |= ATTR_KERNOVAL;
>  		value = NULL;
>  	}
>  
> -	error = xfs_attr_get(ip, name, namelen, value, &asize, xflags);
> +	error = xfs_attr_get(ip, &xname, value, &asize);
>  	if (error)
>  		return error;
>  	return asize;
> @@ -64,23 +63,21 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
>  		struct inode *inode, const char *name, const void *value,
>  		size_t size, int flags)
>  {
> -	int			xflags = handler->flags;
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	int			error;
> -	size_t			namelen = strlen(name);
> +	struct xfs_name		xname = {name, strlen(name), handler->flags};
>  
>  	/* Convert Linux syscall to XFS internal ATTR flags */
>  	if (flags & XATTR_CREATE)
> -		xflags |= ATTR_CREATE;
> +		xname.type |= ATTR_CREATE;
>  	if (flags & XATTR_REPLACE)
> -		xflags |= ATTR_REPLACE;
> +		xname.type |= ATTR_REPLACE;
>  
>  	if (!value)
> -		return xfs_attr_remove(ip, name,
> -				       namelen, xflags);
> -	error = xfs_attr_set(ip, name, namelen, (void *)value, size, xflags);
> +		return xfs_attr_remove(ip, &xname);
> +	error = xfs_attr_set(ip, &xname, (void *)value, size);
>  	if (!error)
> -		xfs_forget_acl(inode, name, xflags);
> +		xfs_forget_acl(inode, name, xname.type);
>  
>  	return error;
>  }
> -- 
> 2.7.4
>