Re: [PATCH 4/8] xfs: simplify xfs_attr_sf_findname

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Dec 17, 2023 at 06:03:46PM +0100, Christoph Hellwig wrote:
> xfs_attr_sf_findname has the simple job of finding a xfs_attr_sf_entry in
> the attr fork, but the convoluted calling convention obfuscates that.
> 
> Return the found entry as the return value instead of an pointer
> argument, as the -ENOATTR/-EEXIST can be trivally derived from that, and
> remove the basep argument, as it is equivalent of the offset of sfe in
> the data for if an sfe was found, or an offset of totsize if not was
> found.  To simplify the totsize computation add a xfs_attr_sf_endptr
> helper that returns the imaginative xfs_attr_sf_entry at the end of
> the current attrs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I like the simplification of xfs_attr_sf_findname's return value.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c      |  7 ++-
>  fs/xfs/libxfs/xfs_attr_leaf.c | 96 +++++++++++++----------------------
>  fs/xfs/libxfs/xfs_attr_leaf.h |  4 +-
>  fs/xfs/libxfs/xfs_attr_sf.h   |  6 +++
>  4 files changed, 47 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 7f822e72dfcd3e..bcf8748cb1a333 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -862,8 +862,11 @@ xfs_attr_lookup(
>  	if (!xfs_inode_hasattr(dp))
>  		return -ENOATTR;
>  
> -	if (dp->i_af.if_format == XFS_DINODE_FMT_LOCAL)
> -		return xfs_attr_sf_findname(args, NULL, NULL);
> +	if (dp->i_af.if_format == XFS_DINODE_FMT_LOCAL) {
> +		if (xfs_attr_sf_findname(args))
> +			return -EEXIST;
> +		return -ENOATTR;
> +	}
>  
>  	if (xfs_attr_is_leaf(dp)) {
>  		error = xfs_attr_leaf_hasname(args, &bp);
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 37474af8ee4633..7a623efd23a6a4 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -698,47 +698,24 @@ xfs_attr_shortform_create(
>  }
>  
>  /*
> - * Return -EEXIST if attr is found, or -ENOATTR if not
> - * args:  args containing attribute name and namelen
> - * sfep:  If not null, pointer will be set to the last attr entry found on
> -	  -EEXIST.  On -ENOATTR pointer is left at the last entry in the list
> - * basep: If not null, pointer is set to the byte offset of the entry in the
> - *	  list on -EEXIST.  On -ENOATTR, pointer is left at the byte offset of
> - *	  the last entry in the list
> + * Return the entry if the attr in args is found, or NULL if not.
>   */
> -int
> +struct xfs_attr_sf_entry *
>  xfs_attr_sf_findname(
> -	struct xfs_da_args	 *args,
> -	struct xfs_attr_sf_entry **sfep,
> -	unsigned int		 *basep)
> +	struct xfs_da_args		*args)
>  {
> -	struct xfs_attr_shortform *sf = args->dp->i_af.if_data;
> -	struct xfs_attr_sf_entry *sfe;
> -	unsigned int		base = sizeof(struct xfs_attr_sf_hdr);
> -	int			size = 0;
> -	int			end;
> -	int			i;
> +	struct xfs_attr_shortform	*sf = args->dp->i_af.if_data;
> +	struct xfs_attr_sf_entry	*sfe;
>  
> -	sfe = &sf->list[0];
> -	end = sf->hdr.count;
> -	for (i = 0; i < end; sfe = xfs_attr_sf_nextentry(sfe),
> -			     base += size, i++) {
> -		size = xfs_attr_sf_entsize(sfe);
> -		if (!xfs_attr_match(args, sfe->namelen, sfe->nameval,
> -				    sfe->flags))
> -			continue;
> -		break;
> +	for (sfe = &sf->list[0];
> +	     sfe < xfs_attr_sf_endptr(sf);
> +	     sfe = xfs_attr_sf_nextentry(sfe)) {
> +		if (xfs_attr_match(args, sfe->namelen, sfe->nameval,
> +				sfe->flags))
> +			return sfe;
>  	}
>  
> -	if (sfep != NULL)
> -		*sfep = sfe;
> -
> -	if (basep != NULL)
> -		*basep = base;
> -
> -	if (i == end)
> -		return -ENOATTR;
> -	return -EEXIST;
> +	return NULL;
>  }
>  
>  /*
> @@ -755,21 +732,19 @@ xfs_attr_shortform_add(
>  	struct xfs_ifork		*ifp = &dp->i_af;
>  	struct xfs_attr_shortform	*sf = ifp->if_data;
>  	struct xfs_attr_sf_entry	*sfe;
> -	int				offset, size;
> +	int				size;
>  
>  	trace_xfs_attr_sf_add(args);
>  
>  	dp->i_forkoff = forkoff;
>  
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> -	if (xfs_attr_sf_findname(args, &sfe, NULL) == -EEXIST)
> -		ASSERT(0);
> +	ASSERT(!xfs_attr_sf_findname(args));
>  
> -	offset = (char *)sfe - (char *)sf;
>  	size = xfs_attr_sf_entsize_byname(args->namelen, args->valuelen);
>  	sf = xfs_idata_realloc(dp, size, XFS_ATTR_FORK);
> -	sfe = (struct xfs_attr_sf_entry *)((char *)sf + offset);
>  
> +	sfe = xfs_attr_sf_endptr(sf);
>  	sfe->namelen = args->namelen;
>  	sfe->valuelen = args->valuelen;
>  	sfe->flags = args->attr_filter;
> @@ -809,39 +784,38 @@ xfs_attr_sf_removename(
>  	struct xfs_mount		*mp = dp->i_mount;
>  	struct xfs_attr_shortform	*sf = dp->i_af.if_data;
>  	struct xfs_attr_sf_entry	*sfe;
> -	int				size = 0, end, totsize;
> -	unsigned int			base;
> -	int				error;
> +	uint16_t			totsize = be16_to_cpu(sf->hdr.totsize);
> +	void				*next, *end;
> +	int				size = 0;
>  
>  	trace_xfs_attr_sf_remove(args);
>  
> -	error = xfs_attr_sf_findname(args, &sfe, &base);
> -
> -	/*
> -	 * If we are recovering an operation, finding nothing to
> -	 * remove is not an error - it just means there was nothing
> -	 * to clean up.
> -	 */
> -	if (error == -ENOATTR && (args->op_flags & XFS_DA_OP_RECOVERY))
> -		return 0;
> -	if (error != -EEXIST)
> -		return error;
> -	size = xfs_attr_sf_entsize(sfe);
> +	sfe = xfs_attr_sf_findname(args);
> +	if (!sfe) {
> +		/*
> +		 * If we are recovering an operation, finding nothing to remove
> +		 * is not an error, it just means there was nothing to clean up.
> +		 */
> +		if (args->op_flags & XFS_DA_OP_RECOVERY)
> +			return 0;
> +		return -ENOATTR;
> +	}
>  
>  	/*
>  	 * Fix up the attribute fork data, covering the hole
>  	 */
> -	end = base + size;
> -	totsize = be16_to_cpu(sf->hdr.totsize);
> -	if (end != totsize)
> -		memmove(&((char *)sf)[base], &((char *)sf)[end], totsize - end);
> +	size = xfs_attr_sf_entsize(sfe);
> +	next = xfs_attr_sf_nextentry(sfe);
> +	end = xfs_attr_sf_endptr(sf);
> +	if (next < end)
> +		memmove(sfe, next, end - next);
>  	sf->hdr.count--;
> -	be16_add_cpu(&sf->hdr.totsize, -size);
> +	totsize -= size;
> +	sf->hdr.totsize = cpu_to_be16(totsize);
>  
>  	/*
>  	 * Fix up the start offset of the attribute fork
>  	 */
> -	totsize -= size;
>  	if (totsize == sizeof(xfs_attr_sf_hdr_t) && xfs_has_attr2(mp) &&
>  	    (dp->i_df.if_format != XFS_DINODE_FMT_BTREE) &&
>  	    !(args->op_flags & (XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE))) {
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
> index ce6743463c8681..56fcd689eedfe7 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.h
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
> @@ -51,9 +51,7 @@ int	xfs_attr_shortform_lookup(struct xfs_da_args *args);
>  int	xfs_attr_shortform_getvalue(struct xfs_da_args *args);
>  int	xfs_attr_shortform_to_leaf(struct xfs_da_args *args);
>  int	xfs_attr_sf_removename(struct xfs_da_args *args);
> -int	xfs_attr_sf_findname(struct xfs_da_args *args,
> -			     struct xfs_attr_sf_entry **sfep,
> -			     unsigned int *basep);
> +struct xfs_attr_sf_entry *xfs_attr_sf_findname(struct xfs_da_args *args);
>  int	xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode *dp);
>  int	xfs_attr_shortform_bytesfit(struct xfs_inode *dp, int bytes);
>  xfs_failaddr_t xfs_attr_shortform_verify(struct xfs_attr_shortform *sfp,
> diff --git a/fs/xfs/libxfs/xfs_attr_sf.h b/fs/xfs/libxfs/xfs_attr_sf.h
> index 37578b369d9b98..db5819cbea0f91 100644
> --- a/fs/xfs/libxfs/xfs_attr_sf.h
> +++ b/fs/xfs/libxfs/xfs_attr_sf.h
> @@ -48,4 +48,10 @@ xfs_attr_sf_nextentry(struct xfs_attr_sf_entry *sfep)
>  	return (void *)sfep + xfs_attr_sf_entsize(sfep);
>  }
>  
> +static inline struct xfs_attr_sf_entry *
> +xfs_attr_sf_endptr(struct xfs_attr_shortform *sf)
> +{
> +	return (void *)sf + be16_to_cpu(sf->hdr.totsize);
> +}
> +
>  #endif	/* __XFS_ATTR_SF_H__ */
> -- 
> 2.39.2
> 
>