From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/5] xfs: consolidate attribute value copying
Date: Thu, 29 Aug 2019 14:26:30 -0700 [thread overview]
Message-ID: <20190829212630.GQ5354@magnolia> (raw)
In-Reply-To: <20190829113505.27223-5-david@fromorbit.com>
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
>
next prev parent reply other threads:[~2019-08-29 21:31 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-29 11:35 [PATCH 0/3 v3] xfs: allocate xattr buffer on demand Dave Chinner
2019-08-29 11:35 ` [PATCH 1/5] xfs: make attr lookup returns consistent Dave Chinner
2019-08-30 5:34 ` Christoph Hellwig
2019-08-29 11:35 ` [PATCH 2/5] xfs: remove unnecessary indenting from xfs_attr3_leaf_getvalue Dave Chinner
2019-08-29 21:23 ` Darrick J. Wong
2019-08-30 5:34 ` Christoph Hellwig
2019-08-29 11:35 ` [PATCH 3/5] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue Dave Chinner
2019-08-29 21:26 ` Darrick J. Wong
2019-08-30 5:35 ` Christoph Hellwig
2019-08-29 11:35 ` [PATCH 4/5] xfs: consolidate attribute value copying Dave Chinner
2019-08-29 21:26 ` Darrick J. Wong [this message]
2019-08-30 5:35 ` Christoph Hellwig
2019-08-29 11:35 ` [PATCH 5/5] xfs: allocate xattr buffer on demand Dave Chinner
2019-08-29 21:27 ` Darrick J. Wong
2019-08-30 5:36 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190829212630.GQ5354@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).