From: Allison Collins <allison.henderson@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v5 02/14] xfs: Replace attribute parameters with struct xfs_name
Date: Fri, 13 Dec 2019 23:57:43 -0700 [thread overview]
Message-ID: <cd64e8ab-ccc7-fc09-1410-d877d90ac3af@oracle.com> (raw)
In-Reply-To: <20191213130714.GB43376@bfoster>
On 12/13/19 6:07 AM, Brian Foster wrote:
> On Wed, Dec 11, 2019 at 09:15:01PM -0700, Allison Collins wrote:
>> This patch replaces the attribute name and length 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 | 22 +++++++++-------------
>> fs/xfs/libxfs/xfs_attr.h | 12 +++++-------
>> fs/xfs/libxfs/xfs_types.c | 10 ++++++++++
>> fs/xfs/libxfs/xfs_types.h | 1 +
>> fs/xfs/xfs_acl.c | 27 +++++++++++++--------------
>> fs/xfs/xfs_ioctl.c | 23 +++++++++++++----------
>> fs/xfs/xfs_iops.c | 6 +++---
>> fs/xfs/xfs_xattr.c | 23 +++++++++++++----------
>> 8 files changed, 67 insertions(+), 57 deletions(-)
>>
> ...
>> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
>> index 4f59554..c81a904 100644
>> --- a/fs/xfs/libxfs/xfs_types.c
>> +++ b/fs/xfs/libxfs/xfs_types.c
>> @@ -12,6 +12,16 @@
>> #include "xfs_bit.h"
>> #include "xfs_mount.h"
>>
>> +/* Initialize a struct xfs_name with a null terminated string name */
>> +void
>> +xfs_name_init(
>> + struct xfs_name *xname,
>> + const char *name)
>> +{
>> + xname->name = name;
>> + xname->len = (strlen(name));
>
> Are the extra braces necessary here? Also, perhaps we should initialize
> ->type to zero or something so it doesn't carry garbage data.
I may have borrowed the extra braces from somewhere else in the code.
Will clean out and also add the type init.
>
>> +}
>> +
>> /* Find the size of the AG, in blocks. */
>> xfs_agblock_t
>> xfs_ag_block_count(
> ...
>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
>> index b58e18c..7b0e5b7 100644
>> --- a/fs/xfs/xfs_acl.c
>> +++ b/fs/xfs/xfs_acl.c
>> @@ -123,7 +123,7 @@ xfs_get_acl(struct inode *inode, int type)
>> struct xfs_inode *ip = XFS_I(inode);
>> struct posix_acl *acl = NULL;
>> struct xfs_acl *xfs_acl = NULL;
>> - unsigned char *ea_name;
>> + struct xfs_name name;
>> int error;
>> int len;
>>
>> @@ -131,10 +131,10 @@ xfs_get_acl(struct inode *inode, int type)
>>
>> switch (type) {
>> case ACL_TYPE_ACCESS:
>> - ea_name = SGI_ACL_FILE;
>> + name.name = SGI_ACL_FILE;
>> break;
>> case ACL_TYPE_DEFAULT:
>> - ea_name = SGI_ACL_DEFAULT;
>> + name.name = SGI_ACL_DEFAULT;
>> break;
>> default:
>> BUG();
>> @@ -145,9 +145,9 @@ xfs_get_acl(struct inode *inode, int type)
>> * go out to the disk.
>> */
>> len = XFS_ACL_MAX_SIZE(ip->i_mount);
>> - error = xfs_attr_get(ip, ea_name, strlen(ea_name),
>> - (unsigned char **)&xfs_acl, &len,
>> - ATTR_ALLOC | ATTR_ROOT);
>> + xfs_name_init(&name, name.name);
>
> Could we call xfs_name_init() in the switch branches above to avoid this
> partial init weirdness? Same question applies below, otherwise looks Ok
> to me.
>
> Brian
Sure, I will scoot that up. Thx!
Allison
>
>> + error = xfs_attr_get(ip, &name, (unsigned char **)&xfs_acl, &len,
>> + ATTR_ALLOC | ATTR_ROOT);
>> if (error) {
>> /*
>> * If the attribute doesn't exist make sure we have a negative
>> @@ -167,17 +167,17 @@ int
>> __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>> {
>> struct xfs_inode *ip = XFS_I(inode);
>> - unsigned char *ea_name;
>> + struct xfs_name name;
>> int error;
>>
>> switch (type) {
>> case ACL_TYPE_ACCESS:
>> - ea_name = SGI_ACL_FILE;
>> + name.name = SGI_ACL_FILE;
>> break;
>> case ACL_TYPE_DEFAULT:
>> if (!S_ISDIR(inode->i_mode))
>> return acl ? -EACCES : 0;
>> - ea_name = SGI_ACL_DEFAULT;
>> + name.name = SGI_ACL_DEFAULT;
>> break;
>> default:
>> return -EINVAL;
>> @@ -197,17 +197,16 @@ __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, strlen(ea_name),
>> - (unsigned char *)xfs_acl, len, ATTR_ROOT);
>> + xfs_name_init(&name, name.name);
>> + error = xfs_attr_set(ip, &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,
>> - strlen(ea_name),
>> - ATTR_ROOT);
>> + error = xfs_attr_remove(ip, &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 f5a9bf9..4fc8698 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -357,7 +357,9 @@ xfs_attrmulti_attr_get(
>> {
>> unsigned char *kbuf;
>> int error = -EFAULT;
>> - size_t namelen;
>> + struct xfs_name xname;
>> +
>> + xfs_name_init(&xname, name);
>>
>> if (*len > XFS_XATTR_SIZE_MAX)
>> return -EINVAL;
>> @@ -365,9 +367,7 @@ xfs_attrmulti_attr_get(
>> if (!kbuf)
>> return -ENOMEM;
>>
>> - namelen = strlen(name);
>> - error = xfs_attr_get(XFS_I(inode), name, namelen, &kbuf, (int *)len,
>> - flags);
>> + error = xfs_attr_get(XFS_I(inode), &xname, &kbuf, (int *)len, flags);
>> if (error)
>> goto out_kfree;
>>
>> @@ -389,7 +389,9 @@ xfs_attrmulti_attr_set(
>> {
>> unsigned char *kbuf;
>> int error;
>> - size_t namelen;
>> + struct xfs_name xname;
>> +
>> + xfs_name_init(&xname, name);
>>
>> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
>> return -EPERM;
>> @@ -400,8 +402,7 @@ xfs_attrmulti_attr_set(
>> if (IS_ERR(kbuf))
>> return PTR_ERR(kbuf);
>>
>> - namelen = strlen(name);
>> - error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
>> + error = xfs_attr_set(XFS_I(inode), &xname, kbuf, len, flags);
>> if (!error)
>> xfs_forget_acl(inode, name, flags);
>> kfree(kbuf);
>> @@ -415,12 +416,14 @@ xfs_attrmulti_attr_remove(
>> uint32_t flags)
>> {
>> int error;
>> - size_t namelen;
>> + struct xfs_name xname;
>> +
>> + xfs_name_init(&xname, name);
>>
>> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
>> return -EPERM;
>> - namelen = strlen(name);
>> - error = xfs_attr_remove(XFS_I(inode), name, namelen, flags);
>> +
>> + error = xfs_attr_remove(XFS_I(inode), &xname, 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 81f2f93..e85bbf5 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -48,11 +48,11 @@ xfs_initxattrs(
>> const struct xattr *xattr;
>> struct xfs_inode *ip = XFS_I(inode);
>> int error = 0;
>> + struct xfs_name name;
>>
>> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> - error = xfs_attr_set(ip, xattr->name,
>> - strlen(xattr->name),
>> - xattr->value, xattr->value_len,
>> + xfs_name_init(&name, xattr->name);
>> + error = xfs_attr_set(ip, &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 1f1601f..5623682 100644
>> --- a/fs/xfs/xfs_xattr.c
>> +++ b/fs/xfs/xfs_xattr.c
>> @@ -21,10 +21,12 @@ 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);
>> + int xflags = handler->flags;
>> + struct xfs_inode *ip = XFS_I(inode);
>> + int error, asize = size;
>> + struct xfs_name xname;
>> +
>> + xfs_name_init(&xname, name);
>>
>> /* Convert Linux syscall to XFS internal ATTR flags */
>> if (!size) {
>> @@ -32,8 +34,8 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
>> value = NULL;
>> }
>>
>> - error = xfs_attr_get(ip, name, namelen, (unsigned char **)&value,
>> - &asize, xflags);
>> + error = xfs_attr_get(ip, &xname, (unsigned char **)&value, &asize,
>> + xflags);
>> if (error)
>> return error;
>> return asize;
>> @@ -69,7 +71,9 @@ 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);
>> + struct xfs_name xname;
>> +
>> + xfs_name_init(&xname, name);
>>
>> /* Convert Linux syscall to XFS internal ATTR flags */
>> if (flags & XATTR_CREATE)
>> @@ -78,9 +82,8 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
>> xflags |= 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, xflags);
>> + error = xfs_attr_set(ip, &xname, (void *)value, size, xflags);
>> if (!error)
>> xfs_forget_acl(inode, name, xflags);
>>
>> --
>> 2.7.4
>>
>
next prev parent reply other threads:[~2019-12-14 6:57 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-12 4:14 [PATCH v5 00/14] xfs: Delay Ready Attributes Allison Collins
2019-12-12 4:15 ` [PATCH v5 01/14] xfs: Remove all strlen in all xfs_attr_* functions for attr names Allison Collins
2019-12-12 4:15 ` [PATCH v5 02/14] xfs: Replace attribute parameters with struct xfs_name Allison Collins
2019-12-13 13:07 ` Brian Foster
2019-12-14 6:57 ` Allison Collins [this message]
2019-12-12 4:15 ` [PATCH v5 03/14] xfs: Embed struct xfs_name in xfs_da_args Allison Collins
2019-12-12 4:15 ` [PATCH v5 04/14] xfs: Add xfs_has_attr and subroutines Allison Collins
2019-12-13 13:08 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:18 ` Christoph Hellwig
2019-12-25 4:21 ` Allison Collins
2020-01-21 22:30 ` Darrick J. Wong
2020-01-22 0:25 ` Allison Collins
2020-01-25 16:27 ` Allison Collins
2020-01-25 23:08 ` Christoph Hellwig
2020-01-26 4:03 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 05/14] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
2019-12-24 12:14 ` Christoph Hellwig
2019-12-25 17:43 ` Allison Collins
2020-01-06 14:46 ` Brian Foster
2020-01-06 18:29 ` Allison Collins
2020-01-06 21:45 ` Darrick J. Wong
2020-01-06 23:33 ` Dave Chinner
2019-12-12 4:15 ` [PATCH v5 06/14] xfs: Factor up trans handling in xfs_attr3_leaf_flipflags Allison Collins
2019-12-24 12:16 ` Christoph Hellwig
2019-12-25 20:49 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 07/14] xfs: Factor out xfs_attr_leaf_addname helper Allison Collins
2019-12-13 14:15 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:22 ` Christoph Hellwig
2019-12-26 17:04 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 08/14] xfs: Factor up xfs_attr_try_sf_addname Allison Collins
2019-12-13 14:15 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:25 ` Christoph Hellwig
2019-12-27 3:23 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 09/14] xfs: Factor up trans roll from xfs_attr3_leaf_setflag Allison Collins
2019-12-24 12:26 ` Christoph Hellwig
2019-12-12 4:15 ` [PATCH v5 10/14] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
2019-12-12 4:15 ` [PATCH v5 11/14] xfs: Factor up trans roll in xfs_attr3_leaf_clearflag Allison Collins
2019-12-24 12:28 ` Christoph Hellwig
2019-12-12 4:15 ` [PATCH v5 12/14] xfs: Check for -ENOATTR or -EEXIST Allison Collins
2019-12-13 14:16 ` Brian Foster
2019-12-14 7:56 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 13/14] xfs: Add delay ready attr remove routines Allison Collins
2019-12-13 17:30 ` Brian Foster
2019-12-14 19:21 ` Allison Collins
2019-12-24 12:30 ` Christoph Hellwig
2019-12-24 23:18 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 14/14] xfs: Add delay ready attr set routines Allison Collins
2019-12-24 12:02 ` [PATCH v5 00/14] xfs: Delay Ready Attributes 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=cd64e8ab-ccc7-fc09-1410-d877d90ac3af@oracle.com \
--to=allison.henderson@oracle.com \
--cc=bfoster@redhat.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).