On Fri, Aug 09, 2019 at 02:37:10PM -0700, Allison Collins wrote:
> This patch replaces the attribute name, length and flags 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@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 40 ++++++++++++++++------------------------
> fs/xfs/libxfs/xfs_attr.h | 12 +++++-------
> fs/xfs/xfs_acl.c | 26 +++++++++++++-------------
> fs/xfs/xfs_ioctl.c | 26 ++++++++++++++++----------
> fs/xfs/xfs_iops.c | 10 ++++++----
> fs/xfs/xfs_xattr.c | 21 +++++++++------------
> 6 files changed, 65 insertions(+), 70 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 7761925..0c91116 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -61,9 +61,7 @@ STATIC int
> xfs_attr_args_init(
> struct xfs_da_args *args,
> struct xfs_inode *dp,
> - const unsigned char *name,
> - size_t namelen,
> - int flags)
> + struct xfs_name *name)
> {
>
> if (!name)
> @@ -73,9 +71,9 @@ xfs_attr_args_init(
> args->geo = dp->i_mount->m_attr_geo;
> args->whichfork = XFS_ATTR_FORK;
> args->dp = dp;
> - args->flags = flags;
> - args->name = name;
> - args->namelen = namelen;
> + args->flags = name->type;
> + args->name = name->name;
> + args->namelen = name->len;
/me wonders if you ought to just have an xfs_name embedded in the
xfs_da_args struct, seeing as the directory code uses it too.
> if (args->namelen >= MAXNAMELEN)
> return -EFAULT; /* match IRIX behaviour */
>
<skipping a bunch of changes that look fine to me>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b1b7b1b..bdf925c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -46,13 +46,15 @@ xfs_initxattrs(
> {
> const struct xattr *xattr;
> struct xfs_inode *ip = XFS_I(inode);
> + struct xfs_name name;
> int error = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - error = xfs_attr_set(ip, xattr->name,
> - strlen(xattr->name),
> - xattr->value, xattr->value_len,
> - ATTR_SECURE);
> + name.name = xattr->name;
> + name.len = strlen(xattr->name);
> + name.type = ATTR_SECURE;
You might as well declare and initialize name in the loop body.
> + error = xfs_attr_set(ip, &name,
> + xattr->value, xattr->value_len);
Does the attr value need a similar structure encapsulation too?
> if (error < 0)
> break;
> }
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index fe12d11..3c63930 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -20,18 +20,17 @@ 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);
> + struct xfs_name xname = {name, strlen(name), handler->flags};
I think the preferred format for this is to use the structure field
names explicity during assignment...
struct xfs_name xname = {
.name = name,
.namelen = strlen(name),
.type = handler->flags,
};
Also, please fix the variable/argument declaration indentation style of
this function to match the rest of xfs. :)
--D
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (!size) {
> - xflags |= ATTR_KERNOVAL;
> + xname.type |= ATTR_KERNOVAL;
> value = NULL;
> }
>
> - error = xfs_attr_get(ip, name, namelen, value, &asize, xflags);
> + error = xfs_attr_get(ip, &xname, value, &asize);
> if (error)
> return error;
> return asize;
> @@ -64,23 +63,21 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
> struct inode *inode, const char *name, const void *value,
> size_t size, int flags)
> {
> - int xflags = handler->flags;
> struct xfs_inode *ip = XFS_I(inode);
> int error;
> - size_t namelen = strlen(name);
> + struct xfs_name xname = {name, strlen(name), handler->flags};
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (flags & XATTR_CREATE)
> - xflags |= ATTR_CREATE;
> + xname.type |= ATTR_CREATE;
> if (flags & XATTR_REPLACE)
> - xflags |= ATTR_REPLACE;
> + xname.type |= 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);
> + error = xfs_attr_set(ip, &xname, (void *)value, size);
> if (!error)
> - xfs_forget_acl(inode, name, xflags);
> + xfs_forget_acl(inode, name, xname.type);
>
> return error;
> }
> --
> 2.7.4
>
