From: Brian Foster <bfoster@redhat.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/9] xfs: Remove all strlen in all xfs_attr_* functions for attr names.
Date: Wed, 17 Apr 2019 11:42:31 -0400 [thread overview]
Message-ID: <20190417154231.GD16377@bfoster> (raw)
In-Reply-To: <20190412225036.22939-2-allison.henderson@oracle.com>
On Fri, Apr 12, 2019 at 03:50:28PM -0700, Allison Henderson wrote:
> This helps to pre-simplify the extra handling of the null terminator in
> delayed operations which use memcpy rather than strlen. Later
> when we introduce parent pointers, attribute names will become binary,
> so strlen will not work at all. Removing uses of strlen now will
> help reduce complexities later
>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
This looks fine to me, Dave's suggestions aside:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr.c | 12 ++++++++----
> fs/xfs/libxfs/xfs_attr.h | 9 ++++++---
> fs/xfs/xfs_acl.c | 12 +++++++-----
> fs/xfs/xfs_ioctl.c | 13 ++++++++++---
> fs/xfs/xfs_iops.c | 6 ++++--
> fs/xfs/xfs_xattr.c | 10 ++++++----
> 6 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 2dd9ee2..3da6b0d 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -67,6 +67,7 @@ xfs_attr_args_init(
> struct xfs_da_args *args,
> struct xfs_inode *dp,
> const unsigned char *name,
> + size_t namelen,
> int flags)
> {
>
> @@ -79,7 +80,7 @@ xfs_attr_args_init(
> args->dp = dp;
> args->flags = flags;
> args->name = name;
> - args->namelen = strlen((const char *)name);
> + args->namelen = namelen;
> if (args->namelen >= MAXNAMELEN)
> return -EFAULT; /* match IRIX behaviour */
>
> @@ -125,6 +126,7 @@ int
> xfs_attr_get(
> struct xfs_inode *ip,
> const unsigned char *name,
> + size_t namelen,
> unsigned char *value,
> int *valuelenp,
> int flags)
> @@ -138,7 +140,7 @@ xfs_attr_get(
> if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> return -EIO;
>
> - error = xfs_attr_args_init(&args, ip, name, flags);
> + error = xfs_attr_args_init(&args, ip, name, namelen, flags);
> if (error)
> return error;
>
> @@ -317,6 +319,7 @@ int
> xfs_attr_set(
> struct xfs_inode *dp,
> const unsigned char *name,
> + size_t namelen,
> unsigned char *value,
> int valuelen,
> int flags)
> @@ -333,7 +336,7 @@ xfs_attr_set(
> if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> return -EIO;
>
> - error = xfs_attr_args_init(&args, dp, name, flags);
> + error = xfs_attr_args_init(&args, dp, name, namelen, flags);
> if (error)
> return error;
>
> @@ -425,6 +428,7 @@ int
> xfs_attr_remove(
> struct xfs_inode *dp,
> const unsigned char *name,
> + size_t namelen,
> int flags)
> {
> struct xfs_mount *mp = dp->i_mount;
> @@ -436,7 +440,7 @@ xfs_attr_remove(
> if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> return -EIO;
>
> - error = xfs_attr_args_init(&args, dp, name, flags);
> + error = xfs_attr_args_init(&args, dp, name, namelen, flags);
> if (error)
> return error;
>
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 2297d84..52f63dc 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -137,11 +137,14 @@ int xfs_attr_list_int(struct xfs_attr_list_context *);
> int xfs_inode_hasattr(struct xfs_inode *ip);
> int xfs_attr_get_ilocked(struct xfs_inode *ip, struct xfs_da_args *args);
> int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
> - unsigned char *value, int *valuelenp, int flags);
> + size_t namelen, unsigned char *value, int *valuelenp,
> + int flags);
> int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> - unsigned char *value, int valuelen, int flags);
> + size_t namelen, unsigned char *value, int valuelen,
> + int flags);
> int xfs_attr_set_args(struct xfs_da_args *args, struct xfs_buf **leaf_bp);
> -int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
> +int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name,
> + size_t namelen, int flags);
> int xfs_attr_remove_args(struct xfs_da_args *args);
> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> int flags, struct attrlist_cursor_kern *cursor);
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 8039e35..142de8d 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -141,8 +141,8 @@ xfs_get_acl(struct inode *inode, int type)
> if (!xfs_acl)
> return ERR_PTR(-ENOMEM);
>
> - error = xfs_attr_get(ip, ea_name, (unsigned char *)xfs_acl,
> - &len, ATTR_ROOT);
> + error = xfs_attr_get(ip, ea_name, strlen(ea_name),
> + (unsigned char *)xfs_acl, &len, ATTR_ROOT);
> if (error) {
> /*
> * If the attribute doesn't exist make sure we have a negative
> @@ -192,15 +192,17 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> len -= sizeof(struct xfs_acl_entry) *
> (XFS_ACL_MAX_ENTRIES(ip->i_mount) - acl->a_count);
>
> - error = xfs_attr_set(ip, ea_name, (unsigned char *)xfs_acl,
> - len, ATTR_ROOT);
> + error = xfs_attr_set(ip, ea_name, strlen(ea_name),
> + (unsigned char *)xfs_acl, len, ATTR_ROOT);
>
> kmem_free(xfs_acl);
> } else {
> /*
> * A NULL ACL argument means we want to remove the ACL.
> */
> - error = xfs_attr_remove(ip, ea_name, ATTR_ROOT);
> + error = xfs_attr_remove(ip, ea_name,
> + strlen(ea_name),
> + ATTR_ROOT);
>
> /*
> * If the attribute didn't exist to start with that's fine.
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6ecdbb3..ab341d6 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -437,6 +437,7 @@ xfs_attrmulti_attr_get(
> {
> unsigned char *kbuf;
> int error = -EFAULT;
> + size_t namelen;
>
> if (*len > XFS_XATTR_SIZE_MAX)
> return -EINVAL;
> @@ -444,7 +445,9 @@ xfs_attrmulti_attr_get(
> if (!kbuf)
> return -ENOMEM;
>
> - error = xfs_attr_get(XFS_I(inode), name, kbuf, (int *)len, flags);
> + namelen = strlen(name);
> + error = xfs_attr_get(XFS_I(inode), name, namelen,
> + kbuf, (int *)len, flags);
> if (error)
> goto out_kfree;
>
> @@ -466,6 +469,7 @@ xfs_attrmulti_attr_set(
> {
> unsigned char *kbuf;
> int error;
> + size_t namelen;
>
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EPERM;
> @@ -476,7 +480,8 @@ xfs_attrmulti_attr_set(
> if (IS_ERR(kbuf))
> return PTR_ERR(kbuf);
>
> - error = xfs_attr_set(XFS_I(inode), name, kbuf, len, flags);
> + namelen = strlen(name);
> + error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
> if (!error)
> xfs_forget_acl(inode, name, flags);
> kfree(kbuf);
> @@ -490,10 +495,12 @@ xfs_attrmulti_attr_remove(
> uint32_t flags)
> {
> int error;
> + size_t namelen;
>
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EPERM;
> - error = xfs_attr_remove(XFS_I(inode), name, flags);
> + namelen = strlen(name);
> + error = xfs_attr_remove(XFS_I(inode), name, namelen, flags);
> if (!error)
> xfs_forget_acl(inode, name, flags);
> return error;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 74047bd..e73c21a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -59,8 +59,10 @@ xfs_initxattrs(
> int error = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - error = xfs_attr_set(ip, xattr->name, xattr->value,
> - xattr->value_len, ATTR_SECURE);
> + error = xfs_attr_set(ip, xattr->name,
> + strlen(xattr->name),
> + xattr->value, xattr->value_len,
> + ATTR_SECURE);
> if (error < 0)
> break;
> }
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 9a63016..3013746 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -26,6 +26,7 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
> int xflags = handler->flags;
> struct xfs_inode *ip = XFS_I(inode);
> int error, asize = size;
> + size_t namelen = strlen(name);
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (!size) {
> @@ -33,7 +34,7 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
> value = NULL;
> }
>
> - error = xfs_attr_get(ip, (unsigned char *)name, value, &asize, xflags);
> + error = xfs_attr_get(ip, name, namelen, value, &asize, xflags);
> if (error)
> return error;
> return asize;
> @@ -69,6 +70,7 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
> int xflags = handler->flags;
> struct xfs_inode *ip = XFS_I(inode);
> int error;
> + size_t namelen = strlen(name);
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (flags & XATTR_CREATE)
> @@ -77,9 +79,9 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
> xflags |= ATTR_REPLACE;
>
> if (!value)
> - return xfs_attr_remove(ip, (unsigned char *)name, xflags);
> - error = xfs_attr_set(ip, (unsigned char *)name,
> - (void *)value, size, xflags);
> + return xfs_attr_remove(ip, name,
> + namelen, xflags);
> + error = xfs_attr_set(ip, name, namelen, (void *)value, size, xflags);
> if (!error)
> xfs_forget_acl(inode, name, xflags);
>
> --
> 2.7.4
>
next prev parent reply other threads:[~2019-04-17 15:42 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-12 22:50 [PATCH 0/9] xfs: Delayed Attributes Allison Henderson
2019-04-12 22:50 ` [PATCH 1/9] xfs: Remove all strlen in all xfs_attr_* functions for attr names Allison Henderson
2019-04-14 23:02 ` Dave Chinner
2019-04-15 20:08 ` Allison Henderson
2019-04-15 21:18 ` Dave Chinner
2019-04-16 1:33 ` Allison Henderson
2019-04-17 15:42 ` Brian Foster [this message]
2019-04-12 22:50 ` [PATCH 2/9] xfs: Hold inode locks in xfs_ialloc Allison Henderson
2019-04-17 15:44 ` Brian Foster
2019-04-17 17:35 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 3/9] xfs: Add trans toggle to attr routines Allison Henderson
2019-04-18 15:27 ` Brian Foster
2019-04-18 21:23 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 4/9] xfs: Set up infastructure for deferred attribute operations Allison Henderson
2019-04-18 15:48 ` Brian Foster
2019-04-18 21:27 ` Allison Henderson
2019-04-22 11:00 ` Brian Foster
2019-04-22 22:00 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 5/9] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred Allison Henderson
2019-04-18 15:49 ` Brian Foster
2019-04-18 21:28 ` Allison Henderson
2019-04-22 11:01 ` Brian Foster
2019-04-22 22:01 ` Allison Henderson
2019-04-23 13:00 ` Brian Foster
2019-04-24 2:24 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 6/9] xfs: Add xfs_has_attr and subroutines Allison Henderson
2019-04-15 2:46 ` Su Yue
2019-04-15 20:13 ` Allison Henderson
2019-04-22 13:00 ` Brian Foster
2019-04-22 22:01 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 7/9] xfs: Add attr context to log item Allison Henderson
2019-04-15 22:50 ` Darrick J. Wong
2019-04-16 2:30 ` Allison Henderson
2019-04-16 3:21 ` Allison Henderson
2019-04-22 13:03 ` Brian Foster
2019-04-22 22:01 ` Allison Henderson
2019-04-23 13:20 ` Brian Foster
2019-04-24 2:24 ` Allison Henderson
2019-04-24 4:10 ` Darrick J. Wong
2019-04-24 12:17 ` Brian Foster
2019-04-24 15:25 ` Darrick J. Wong
2019-04-24 16:57 ` Brian Foster
2019-04-12 22:50 ` [PATCH 8/9] xfs: Roll delayed attr operations by returning EAGAIN Allison Henderson
2019-04-15 23:31 ` Darrick J. Wong
2019-04-16 19:54 ` Allison Henderson
2019-04-23 14:19 ` Brian Foster
2019-04-24 2:24 ` Allison Henderson
2019-04-12 22:50 ` [PATCH 9/9] xfs: Remove roll_trans boolean Allison Henderson
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=20190417154231.GD16377@bfoster \
--to=bfoster@redhat.com \
--cc=allison.henderson@oracle.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.