diff mbox series

[4/5] xfs: consolidate attribute value copying

Message ID 20190829113505.27223-5-david@fromorbit.com (mailing list archive)
State Accepted
Headers show
Series xfs: allocate xattr buffer on demand | expand

Commit Message

Dave Chinner Aug. 29, 2019, 11:35 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

The same code is used to copy do the attribute copying in three
different places. Consolidate them into a single function in
preparation from on-demand buffer allocation.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 88 +++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 39 deletions(-)

Comments

Darrick J. Wong Aug. 29, 2019, 9:26 p.m. UTC | #1 
On Thu, Aug 29, 2019 at 09:35:04PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The same code is used to copy do the attribute copying in three
> different places. Consolidate them into a single function in
> preparation from on-demand buffer allocation.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 88 +++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8085c4f0e5a0..f6a595e76343 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -393,6 +393,44 @@ xfs_attr_namesp_match(int arg_flags, int ondisk_flags)
>  	return XFS_ATTR_NSP_ONDISK(ondisk_flags) == XFS_ATTR_NSP_ARGS_TO_ONDISK(arg_flags);
>  }
>  
> +static int
> +xfs_attr_copy_value(
> +	struct xfs_da_args	*args,
> +	unsigned char		*value,
> +	int			valuelen)
> +{
> +	/*
> +	 * No copy if all we have to do is get the length
> +	 */
> +	if (args->flags & ATTR_KERNOVAL) {
> +		args->valuelen = valuelen;
> +		return 0;
> +	}
> +
> +	/*
> +	 * No copy if the length of the existing buffer is too small
> +	 */
> +	if (args->valuelen < valuelen) {
> +		args->valuelen = valuelen;
> +		return -ERANGE;
> +	}
> +	args->valuelen = valuelen;
> +
> +	/* remote block xattr requires IO for copy-in */
> +	if (args->rmtblkno)
> +		return xfs_attr_rmtval_get(args);
> +
> +	/*
> +	 * This is to prevent a GCC warning because the remote xattr case
> +	 * doesn't have a value to pass in. In that case, we never reach here,
> +	 * but GCC can't work that out and so throws a "passing NULL to
> +	 * memcpy" warning.
> +	 */
> +	if (!value)
> +		return -EINVAL;
> +	memcpy(args->value, value, valuelen);
> +	return 0;
> +}
>  
>  /*========================================================================
>   * External routines when attribute fork size < XFS_LITINO(mp).
> @@ -727,11 +765,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args)
>   * exist or we can't retrieve the value.
>   */
>  int
> -xfs_attr_shortform_getvalue(xfs_da_args_t *args)
> +xfs_attr_shortform_getvalue(
> +	struct xfs_da_args	*args)
>  {
> -	xfs_attr_shortform_t *sf;
> -	xfs_attr_sf_entry_t *sfe;
> -	int i;
> +	struct xfs_attr_shortform *sf;
> +	struct xfs_attr_sf_entry *sfe;
> +	int			i;
>  
>  	ASSERT(args->dp->i_afp->if_flags == XFS_IFINLINE);
>  	sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;
> @@ -744,18 +783,8 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args)
>  			continue;
>  		if (!xfs_attr_namesp_match(args->flags, sfe->flags))
>  			continue;
> -		if (args->flags & ATTR_KERNOVAL) {
> -			args->valuelen = sfe->valuelen;
> -			return 0;
> -		}
> -		if (args->valuelen < sfe->valuelen) {
> -			args->valuelen = sfe->valuelen;
> -			return -ERANGE;
> -		}
> -		args->valuelen = sfe->valuelen;
> -		memcpy(args->value, &sfe->nameval[args->namelen],
> -						    args->valuelen);
> -		return 0;
> +		return xfs_attr_copy_value(args, &sfe->nameval[args->namelen],
> +						sfe->valuelen);
>  	}
>  	return -ENOATTR;
>  }
> @@ -2368,7 +2397,6 @@ xfs_attr3_leaf_getvalue(
>  	struct xfs_attr_leaf_entry *entry;
>  	struct xfs_attr_leaf_name_local *name_loc;
>  	struct xfs_attr_leaf_name_remote *name_rmt;
> -	int			valuelen;
>  
>  	leaf = bp->b_addr;
>  	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
> @@ -2380,18 +2408,9 @@ xfs_attr3_leaf_getvalue(
>  		name_loc = xfs_attr3_leaf_name_local(leaf, args->index);
>  		ASSERT(name_loc->namelen == args->namelen);
>  		ASSERT(memcmp(args->name, name_loc->nameval, args->namelen) == 0);
> -		valuelen = be16_to_cpu(name_loc->valuelen);
> -		if (args->flags & ATTR_KERNOVAL) {
> -			args->valuelen = valuelen;
> -			return 0;
> -		}
> -		if (args->valuelen < valuelen) {
> -			args->valuelen = valuelen;
> -			return -ERANGE;
> -		}
> -		args->valuelen = valuelen;
> -		memcpy(args->value, &name_loc->nameval[args->namelen], valuelen);
> -		return 0;
> +		return xfs_attr_copy_value(args,
> +					&name_loc->nameval[args->namelen],
> +					be16_to_cpu(name_loc->valuelen));
>  	}
>  
>  	name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
> @@ -2401,16 +2420,7 @@ xfs_attr3_leaf_getvalue(
>  	args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  	args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
>  					       args->rmtvaluelen);
> -	if (args->flags & ATTR_KERNOVAL) {
> -		args->valuelen = args->rmtvaluelen;
> -		return 0;
> -	}
> -	if (args->valuelen < args->rmtvaluelen) {
> -		args->valuelen = args->rmtvaluelen;
> -		return -ERANGE;
> -	}
> -	args->valuelen = args->rmtvaluelen;
> -	return xfs_attr_rmtval_get(args);
> +	return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
>  }
>  
>  /*========================================================================
> -- 
> 2.23.0.rc1
>
Christoph Hellwig Aug. 30, 2019, 5:35 a.m. UTC | #2 
On Thu, Aug 29, 2019 at 09:35:04PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The same code is used to copy do the attribute copying in three
> different places. Consolidate them into a single function in
> preparation from on-demand buffer allocation.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 8085c4f0e5a0..f6a595e76343 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -393,6 +393,44 @@  xfs_attr_namesp_match(int arg_flags, int ondisk_flags)
 	return XFS_ATTR_NSP_ONDISK(ondisk_flags) == XFS_ATTR_NSP_ARGS_TO_ONDISK(arg_flags);
 }
 
+static int
+xfs_attr_copy_value(
+	struct xfs_da_args	*args,
+	unsigned char		*value,
+	int			valuelen)
+{
+	/*
+	 * No copy if all we have to do is get the length
+	 */
+	if (args->flags & ATTR_KERNOVAL) {
+		args->valuelen = valuelen;
+		return 0;
+	}
+
+	/*
+	 * No copy if the length of the existing buffer is too small
+	 */
+	if (args->valuelen < valuelen) {
+		args->valuelen = valuelen;
+		return -ERANGE;
+	}
+	args->valuelen = valuelen;
+
+	/* remote block xattr requires IO for copy-in */
+	if (args->rmtblkno)
+		return xfs_attr_rmtval_get(args);
+
+	/*
+	 * This is to prevent a GCC warning because the remote xattr case
+	 * doesn't have a value to pass in. In that case, we never reach here,
+	 * but GCC can't work that out and so throws a "passing NULL to
+	 * memcpy" warning.
+	 */
+	if (!value)
+		return -EINVAL;
+	memcpy(args->value, value, valuelen);
+	return 0;
+}
 
 /*========================================================================
  * External routines when attribute fork size < XFS_LITINO(mp).
@@ -727,11 +765,12 @@  xfs_attr_shortform_lookup(xfs_da_args_t *args)
  * exist or we can't retrieve the value.
  */
 int
-xfs_attr_shortform_getvalue(xfs_da_args_t *args)
+xfs_attr_shortform_getvalue(
+	struct xfs_da_args	*args)
 {
-	xfs_attr_shortform_t *sf;
-	xfs_attr_sf_entry_t *sfe;
-	int i;
+	struct xfs_attr_shortform *sf;
+	struct xfs_attr_sf_entry *sfe;
+	int			i;
 
 	ASSERT(args->dp->i_afp->if_flags == XFS_IFINLINE);
 	sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;
@@ -744,18 +783,8 @@  xfs_attr_shortform_getvalue(xfs_da_args_t *args)
 			continue;
 		if (!xfs_attr_namesp_match(args->flags, sfe->flags))
 			continue;
-		if (args->flags & ATTR_KERNOVAL) {
-			args->valuelen = sfe->valuelen;
-			return 0;
-		}
-		if (args->valuelen < sfe->valuelen) {
-			args->valuelen = sfe->valuelen;
-			return -ERANGE;
-		}
-		args->valuelen = sfe->valuelen;
-		memcpy(args->value, &sfe->nameval[args->namelen],
-						    args->valuelen);
-		return 0;
+		return xfs_attr_copy_value(args, &sfe->nameval[args->namelen],
+						sfe->valuelen);
 	}
 	return -ENOATTR;
 }
@@ -2368,7 +2397,6 @@  xfs_attr3_leaf_getvalue(
 	struct xfs_attr_leaf_entry *entry;
 	struct xfs_attr_leaf_name_local *name_loc;
 	struct xfs_attr_leaf_name_remote *name_rmt;
-	int			valuelen;
 
 	leaf = bp->b_addr;
 	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
@@ -2380,18 +2408,9 @@  xfs_attr3_leaf_getvalue(
 		name_loc = xfs_attr3_leaf_name_local(leaf, args->index);
 		ASSERT(name_loc->namelen == args->namelen);
 		ASSERT(memcmp(args->name, name_loc->nameval, args->namelen) == 0);
-		valuelen = be16_to_cpu(name_loc->valuelen);
-		if (args->flags & ATTR_KERNOVAL) {
-			args->valuelen = valuelen;
-			return 0;
-		}
-		if (args->valuelen < valuelen) {
-			args->valuelen = valuelen;
-			return -ERANGE;
-		}
-		args->valuelen = valuelen;
-		memcpy(args->value, &name_loc->nameval[args->namelen], valuelen);
-		return 0;
+		return xfs_attr_copy_value(args,
+					&name_loc->nameval[args->namelen],
+					be16_to_cpu(name_loc->valuelen));
 	}
 
 	name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
@@ -2401,16 +2420,7 @@  xfs_attr3_leaf_getvalue(
 	args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
 	args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
 					       args->rmtvaluelen);
-	if (args->flags & ATTR_KERNOVAL) {
-		args->valuelen = args->rmtvaluelen;
-		return 0;
-	}
-	if (args->valuelen < args->rmtvaluelen) {
-		args->valuelen = args->rmtvaluelen;
-		return -ERANGE;
-	}
-	args->valuelen = args->rmtvaluelen;
-	return xfs_attr_rmtval_get(args);
+	return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
 }
 
 /*========================================================================