Re: [PATCH v7 01/19] xfs: Replace attribute parameters with struct xfs_name

Allison Collins
2020-02-23T16:03:10Z

On 2/23/20 2:34 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <allison.henderson@oracle.com> 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>
>> ---
>
> I realize I am joining very late with lots of reviewed-by already applied,
> so unless I find anything big, please regard my comments and possible
> future improvements for the extended series rather than objections to this
> pretty much baked patch set.
Sure, no worries, it has a lot of history to keep track of.

>
> [...]
>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index d42de92..28c07c9 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;
>
>
> A struct inititializer macro would have been nice, so code like this:
>
> + struct xfs_name xname;
> +
> + xfs_name_init(&xname, name);
>
> Would become:
> + struct xfs_name xname = XFS_NAME_STR_INIT(name);
>
> As a matter of fact, in most of the cases a named local variable is
> not needed at
> all and the code could be written with an anonymous local struct variable macro:
>
> + error = xfs_attr_remove(XFS_I(inode), XFS_NAME_STR(name), flags);
>
The macro does look nice. I would be the third iteration of
initializers that this patch has been through though. Can I get a
consensus of how many people like the macro?

Allison

> Thanks,
> Amir.
>