clean up the attr interface v3

clean up the attr interface v3

[Christoph Hellwig]

xfsprogs patches porting over the libxfs changes are available here:

    http://git.infradead.org/users/hch/xfsprogs.git/shortlog/refs/heads/attr-cleanup

Changes since v2:
 - add more comments
 - fix up an error handling corner case in __xfs_set_acl
 - add more cowbell^H^H^H^H^H^H^Hbool
 - add a new patch to reject invalid namespaces flags in
   XFS_IOC_ATTRLIST_BY_HANDLE
 - remove ATTR_ENTSIZE entirely

Changes since v1:
 - rebased to for-next, which includes the fixes from the first
   version

[PATCH 01/30] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE

[Christoph Hellwig]

While the flags field in the ABI and the on-disk format allows for
multiple namespace flags, that is a logically invalid combination and
listing multiple namespace flags will return no results as no attr
can have both set.  Reject this case early with -EINVAL.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c   | 2 ++
 fs/xfs/xfs_ioctl32.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d42de92cb283..d974bf099d45 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -317,6 +317,8 @@ xfs_attrlist_by_handle(
 	 */
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -EINVAL;
+	if (al_hreq.flags == (ATTR_ROOT | ATTR_SECURE))
+		return -EINVAL;
 
 	dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
 	if (IS_ERR(dentry))
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 769581a79c58..9705172e5410 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -375,6 +375,8 @@ xfs_compat_attrlist_by_handle(
 	 */
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -EINVAL;
+	if (al_hreq.flags == (ATTR_ROOT | ATTR_SECURE))
+		return -EINVAL;
 
 	dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
 	if (IS_ERR(dentry))
-- 
2.24.1

Re: [PATCH 01/30] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 

> While the flags field in the ABI and the on-disk format allows for
> multiple namespace flags, that is a logically invalid combination and
> listing multiple namespace flags will return no results as no attr
> can have both set.  Reject this case early with -EINVAL.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c   | 2 ++
>  fs/xfs/xfs_ioctl32.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d42de92cb283..d974bf099d45 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -317,6 +317,8 @@ xfs_attrlist_by_handle(
>  	 */
>  	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
>  		return -EINVAL;

The above statement makes sure that al_hreq.flags has only ATTR_ROOT
and/or ATTR_SECURE flags set ...

> +	if (al_hreq.flags == (ATTR_ROOT | ATTR_SECURE))
> +		return -EINVAL;
>
... Hence if the execution control arrives here, we can be sure that the
presence of no other bits need to be checked.

Therefore the code is logically correct.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

>  	dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
>  	if (IS_ERR(dentry))
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 769581a79c58..9705172e5410 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -375,6 +375,8 @@ xfs_compat_attrlist_by_handle(
>  	 */
>  	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
>  		return -EINVAL;
> +	if (al_hreq.flags == (ATTR_ROOT | ATTR_SECURE))
> +		return -EINVAL;
> 
>  	dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
>  	if (IS_ERR(dentry))
> 

-- 
chandan

[PATCH 02/30] xfs: remove the ATTR_INCOMPLETE flag

[Christoph Hellwig]

Replace the ATTR_INCOMPLETE flag with a new boolean field in struct
xfs_attr_list_context.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.h | 5 ++---
 fs/xfs/scrub/attr.c      | 2 +-
 fs/xfs/xfs_attr_list.c   | 6 +-----
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 4243b2272642..71bcf1298e4c 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -36,11 +36,10 @@ struct xfs_attr_list_context;
 #define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
 #define ATTR_KERNOVAL	0x2000	/* [kernel] get attr size only, not value */
 
-#define ATTR_INCOMPLETE	0x4000	/* [kernel] return INCOMPLETE attr keys */
 #define ATTR_ALLOC	0x8000	/* [kernel] allocate xattr buffer on demand */
 
 #define ATTR_KERNEL_FLAGS \
-	(ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_INCOMPLETE | ATTR_ALLOC)
+	(ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_ALLOC)
 
 #define XFS_ATTR_FLAGS \
 	{ ATTR_DONTFOLLOW, 	"DONTFOLLOW" }, \
@@ -51,7 +50,6 @@ struct xfs_attr_list_context;
 	{ ATTR_REPLACE,		"REPLACE" }, \
 	{ ATTR_KERNOTIME,	"KERNOTIME" }, \
 	{ ATTR_KERNOVAL,	"KERNOVAL" }, \
-	{ ATTR_INCOMPLETE,	"INCOMPLETE" }, \
 	{ ATTR_ALLOC,		"ALLOC" }
 
 /*
@@ -123,6 +121,7 @@ typedef struct xfs_attr_list_context {
 	 * error values to the xfs_attr_list caller.
 	 */
 	int				seen_enough;
+	bool				allow_incomplete;
 
 	ssize_t				count;		/* num used entries */
 	int				dupcnt;		/* count dup hashvals seen */
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index d9f0dd444b80..d804558cdbca 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -497,7 +497,7 @@ xchk_xattr(
 	sx.context.resynch = 1;
 	sx.context.put_listent = xchk_xattr_listent;
 	sx.context.tp = sc->tp;
-	sx.context.flags = ATTR_INCOMPLETE;
+	sx.context.allow_incomplete = true;
 	sx.sc = sc;
 
 	/*
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index d37743bdf274..5139ef983cd6 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -452,7 +452,7 @@ xfs_attr3_leaf_list_int(
 		}
 
 		if ((entry->flags & XFS_ATTR_INCOMPLETE) &&
-		    !(context->flags & ATTR_INCOMPLETE))
+		    !context->allow_incomplete)
 			continue;		/* skip incomplete entries */
 
 		if (entry->flags & XFS_ATTR_LOCAL) {
@@ -632,10 +632,6 @@ xfs_attr_list(
 	    (cursor->hashval || cursor->blkno || cursor->offset))
 		return -EINVAL;
 
-	/* Only internal consumers can retrieve incomplete attrs. */
-	if (flags & ATTR_INCOMPLETE)
-		return -EINVAL;
-
 	/*
 	 * Check for a properly aligned buffer.
 	 */
-- 
2.24.1

[PATCH 03/30] xfs: merge xfs_attr_remove into xfs_attr_set

[Christoph Hellwig]

The Linux xattr and acl APIs use a single call for set an remove.  Modify
the high-level XFS API to match that and let xfs_attr_set handle removing
attributes as well.  With a little bit of reordering this removes a lot
of code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c | 178 ++++++++++++++-------------------------
 fs/xfs/libxfs/xfs_attr.h |   2 -
 fs/xfs/xfs_acl.c         |  33 +++-----
 fs/xfs/xfs_ioctl.c       |   4 +-
 fs/xfs/xfs_xattr.c       |   9 +-
 5 files changed, 77 insertions(+), 149 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e6149720ce02..bb391b96cd78 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -336,6 +336,10 @@ xfs_attr_remove_args(
 	return error;
 }
 
+/*
+ * Note: If value is NULL the attribute will be removed, just like the
+ * Linux ->setattr API.
+ */
 int
 xfs_attr_set(
 	struct xfs_inode	*dp,
@@ -350,149 +354,92 @@ xfs_attr_set(
 	struct xfs_trans_res	tres;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
 	int			error, local;
-
-	XFS_STATS_INC(mp, xs_attr_set);
+	unsigned int		total;
 
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
 		return -EIO;
 
-	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
-	if (error)
-		return error;
-
-	args.value = value;
-	args.valuelen = valuelen;
-	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
-	args.total = xfs_attr_calc_size(&args, &local);
-
 	error = xfs_qm_dqattach(dp);
 	if (error)
 		return error;
 
-	/*
-	 * If the inode doesn't have an attribute fork, add one.
-	 * (inode must not be locked when we call this routine)
-	 */
-	if (XFS_IFORK_Q(dp) == 0) {
-		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
-			XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen, valuelen);
-
-		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
-		if (error)
-			return error;
-	}
-
-	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
-	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
-	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
-
-	/*
-	 * Root fork attributes can use reserved data blocks for this
-	 * operation if necessary
-	 */
-	error = xfs_trans_alloc(mp, &tres, args.total, 0,
-			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
+	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
 	if (error)
 		return error;
 
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,
-				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-				       XFS_QMOPT_RES_REGBLKS);
-	if (error)
-		goto out_trans_cancel;
-
-	xfs_trans_ijoin(args.trans, dp, 0);
-	error = xfs_attr_set_args(&args);
-	if (error)
-		goto out_trans_cancel;
-	if (!args.trans) {
-		/* shortform attribute has already been committed */
-		goto out_unlock;
-	}
-
-	/*
-	 * If this is a synchronous mount, make sure that the
-	 * transaction goes to disk before returning to the user.
-	 */
-	if (mp->m_flags & XFS_MOUNT_WSYNC)
-		xfs_trans_set_sync(args.trans);
-
-	if ((flags & ATTR_KERNOTIME) == 0)
-		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
+	args.value = value;
+	args.valuelen = valuelen;
 
 	/*
-	 * Commit the last in the sequence of transactions.
+	 * We have no control over the attribute names that userspace passes us
+	 * to remove, so we have to allow the name lookup prior to attribute
+	 * removal to fail as well.
 	 */
-	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
-	error = xfs_trans_commit(args.trans);
-out_unlock:
-	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-	return error;
-
-out_trans_cancel:
-	if (args.trans)
-		xfs_trans_cancel(args.trans);
-	goto out_unlock;
-}
+	args.op_flags = XFS_DA_OP_OKNOENT;
 
-/*
- * Generic handler routine to remove a name from an attribute list.
- * Transitions attribute list from Btree to shortform as necessary.
- */
-int
-xfs_attr_remove(
-	struct xfs_inode	*dp,
-	const unsigned char	*name,
-	size_t			namelen,
-	int			flags)
-{
-	struct xfs_mount	*mp = dp->i_mount;
-	struct xfs_da_args	args;
-	int			error;
+	if (value) {
+		XFS_STATS_INC(mp, xs_attr_set);
 
-	XFS_STATS_INC(mp, xs_attr_remove);
+		args.op_flags |= XFS_DA_OP_ADDNAME;
+		args.total = xfs_attr_calc_size(&args, &local);
 
-	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
-		return -EIO;
+		/*
+		 * If the inode doesn't have an attribute fork, add one.
+		 * (inode must not be locked when we call this routine)
+		 */
+		if (XFS_IFORK_Q(dp) == 0) {
+			int sf_size = sizeof(struct xfs_attr_sf_hdr) +
+				XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen,
+						valuelen);
 
-	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
-	if (error)
-		return error;
+			error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
+			if (error)
+				return error;
+		}
 
-	/*
-	 * we have no control over the attribute names that userspace passes us
-	 * to remove, so we have to allow the name lookup prior to attribute
-	 * removal to fail.
-	 */
-	args.op_flags = XFS_DA_OP_OKNOENT;
+		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
+				 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
+		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
+		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
+		total = args.total;
+	} else {
+		XFS_STATS_INC(mp, xs_attr_remove);
 
-	error = xfs_qm_dqattach(dp);
-	if (error)
-		return error;
+		tres = M_RES(mp)->tr_attrrm;
+		total = XFS_ATTRRM_SPACE_RES(mp);
+	}
 
 	/*
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
 	 */
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
-			XFS_ATTRRM_SPACE_RES(mp), 0,
-			(flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0,
-			&args.trans);
+	error = xfs_trans_alloc(mp, &tres, total, 0,
+			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
 	if (error)
 		return error;
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
-	/*
-	 * No need to make quota reservations here. We expect to release some
-	 * blocks not allocate in the common case.
-	 */
 	xfs_trans_ijoin(args.trans, dp, 0);
+	if (value) {
+		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
 
-	error = xfs_attr_remove_args(&args);
-	if (error)
-		goto out;
+		if (rsvd)
+			quota_flags |= XFS_QMOPT_FORCE_RES;
+		error = xfs_trans_reserve_quota_nblks(args.trans, dp,
+				args.total, 0, quota_flags);
+		if (error)
+			goto out_trans_cancel;
+		error = xfs_attr_set_args(&args);
+		if (error)
+			goto out_trans_cancel;
+		/* shortform attribute has already been committed */
+		if (!args.trans)
+			goto out_unlock;
+	} else {
+		error = xfs_attr_remove_args(&args);
+		if (error)
+			goto out_trans_cancel;
+	}
 
 	/*
 	 * If this is a synchronous mount, make sure that the
@@ -509,15 +456,14 @@ xfs_attr_remove(
 	 */
 	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
 	error = xfs_trans_commit(args.trans);
+out_unlock:
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-
 	return error;
 
-out:
+out_trans_cancel:
 	if (args.trans)
 		xfs_trans_cancel(args.trans);
-	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-	return error;
+	goto out_unlock;
 }
 
 /*========================================================================
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 71bcf1298e4c..db58a6c7dea5 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -152,8 +152,6 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
 int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
 		 size_t namelen, unsigned char *value, int valuelen, int flags);
 int xfs_attr_set_args(struct xfs_da_args *args);
-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 cd743fad8478..4e76063ff956 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -168,6 +168,8 @@ __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_acl *xfs_acl = NULL;
+	int len = 0;
 	int error;
 
 	switch (type) {
@@ -184,9 +186,7 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	}
 
 	if (acl) {
-		struct xfs_acl *xfs_acl;
-		int len = XFS_ACL_MAX_SIZE(ip->i_mount);
-
+		len = XFS_ACL_MAX_SIZE(ip->i_mount);
 		xfs_acl = kmem_zalloc_large(len, 0);
 		if (!xfs_acl)
 			return -ENOMEM;
@@ -196,26 +196,17 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		/* subtract away the unused acl entries */
 		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);
-
-		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);
-
-		/*
-		 * If the attribute didn't exist to start with that's fine.
-		 */
-		if (error == -ENOATTR)
-			error = 0;
 	}
 
+	error = xfs_attr_set(ip, ea_name, strlen(ea_name),
+			(unsigned char *)xfs_acl, len, ATTR_ROOT);
+	kmem_free(xfs_acl);
+
+	/*
+	 * If the attribute didn't exist to start with that's fine.
+	 */
+	if (!acl && error == -ENOATTR)
+		error = 0;
 	if (!error)
 		set_cached_acl(inode, type, acl);
 	return error;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d974bf099d45..79c418888e9a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -417,12 +417,10 @@ xfs_attrmulti_attr_remove(
 	uint32_t		flags)
 {
 	int			error;
-	size_t			namelen;
 
 	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_set(XFS_I(inode), name, strlen(name), NULL, 0, flags);
 	if (!error)
 		xfs_forget_acl(inode, name, flags);
 	return error;
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index b0fedb543f97..1670bfbc9ad2 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -69,7 +69,6 @@ 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,14 +76,10 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
 	if (flags & XATTR_REPLACE)
 		xflags |= ATTR_REPLACE;
 
-	if (value)
-		error = xfs_attr_set(ip, name, namelen, (void *)value, size,
-				xflags);
-	else
-		error = xfs_attr_remove(ip, name, namelen, xflags);
+	error = xfs_attr_set(ip, (unsigned char *)name, strlen(name),
+				(void *)value, size, xflags);
 	if (!error)
 		xfs_forget_acl(inode, name, xflags);
-
 	return error;
 }
 
-- 
2.24.1

Re: [PATCH 03/30] xfs: merge xfs_attr_remove into xfs_attr_set

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 

> The Linux xattr and acl APIs use a single call for set an remove.  Modify
> the high-level XFS API to match that and let xfs_attr_set handle removing
> attributes as well.  With a little bit of reordering this removes a lot
> of code.
>

The newly introduced changes match with the code flow that earlier existed
separately in xfs_attr_set() and xfs_attr_remove().

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 178 ++++++++++++++-------------------------
>  fs/xfs/libxfs/xfs_attr.h |   2 -
>  fs/xfs/xfs_acl.c         |  33 +++-----
>  fs/xfs/xfs_ioctl.c       |   4 +-
>  fs/xfs/xfs_xattr.c       |   9 +-
>  5 files changed, 77 insertions(+), 149 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e6149720ce02..bb391b96cd78 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -336,6 +336,10 @@ xfs_attr_remove_args(
>  	return error;
>  }
> 
> +/*
> + * Note: If value is NULL the attribute will be removed, just like the
> + * Linux ->setattr API.
> + */
>  int
>  xfs_attr_set(
>  	struct xfs_inode	*dp,
> @@ -350,149 +354,92 @@ xfs_attr_set(
>  	struct xfs_trans_res	tres;
>  	int			rsvd = (flags & ATTR_ROOT) != 0;
>  	int			error, local;
> -
> -	XFS_STATS_INC(mp, xs_attr_set);
> +	unsigned int		total;
> 
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>  		return -EIO;
> 
> -	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
> -	if (error)
> -		return error;
> -
> -	args.value = value;
> -	args.valuelen = valuelen;
> -	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> -	args.total = xfs_attr_calc_size(&args, &local);
> -
>  	error = xfs_qm_dqattach(dp);
>  	if (error)
>  		return error;
> 
> -	/*
> -	 * If the inode doesn't have an attribute fork, add one.
> -	 * (inode must not be locked when we call this routine)
> -	 */
> -	if (XFS_IFORK_Q(dp) == 0) {
> -		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
> -			XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen, valuelen);
> -
> -		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
> -		if (error)
> -			return error;
> -	}
> -
> -	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> -			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> -	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> -	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> -
> -	/*
> -	 * Root fork attributes can use reserved data blocks for this
> -	 * operation if necessary
> -	 */
> -	error = xfs_trans_alloc(mp, &tres, args.total, 0,
> -			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
> +	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
>  	if (error)
>  		return error;
> 
> -	xfs_ilock(dp, XFS_ILOCK_EXCL);
> -	error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,
> -				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
> -				       XFS_QMOPT_RES_REGBLKS);
> -	if (error)
> -		goto out_trans_cancel;
> -
> -	xfs_trans_ijoin(args.trans, dp, 0);
> -	error = xfs_attr_set_args(&args);
> -	if (error)
> -		goto out_trans_cancel;
> -	if (!args.trans) {
> -		/* shortform attribute has already been committed */
> -		goto out_unlock;
> -	}
> -
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * transaction goes to disk before returning to the user.
> -	 */
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args.trans);
> -
> -	if ((flags & ATTR_KERNOTIME) == 0)
> -		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
> +	args.value = value;
> +	args.valuelen = valuelen;
> 
>  	/*
> -	 * Commit the last in the sequence of transactions.
> +	 * We have no control over the attribute names that userspace passes us
> +	 * to remove, so we have to allow the name lookup prior to attribute
> +	 * removal to fail as well.
>  	 */
> -	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
> -	error = xfs_trans_commit(args.trans);
> -out_unlock:
> -	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> -	return error;
> -
> -out_trans_cancel:
> -	if (args.trans)
> -		xfs_trans_cancel(args.trans);
> -	goto out_unlock;
> -}
> +	args.op_flags = XFS_DA_OP_OKNOENT;
> 
> -/*
> - * Generic handler routine to remove a name from an attribute list.
> - * Transitions attribute list from Btree to shortform as necessary.
> - */
> -int
> -xfs_attr_remove(
> -	struct xfs_inode	*dp,
> -	const unsigned char	*name,
> -	size_t			namelen,
> -	int			flags)
> -{
> -	struct xfs_mount	*mp = dp->i_mount;
> -	struct xfs_da_args	args;
> -	int			error;
> +	if (value) {
> +		XFS_STATS_INC(mp, xs_attr_set);
> 
> -	XFS_STATS_INC(mp, xs_attr_remove);
> +		args.op_flags |= XFS_DA_OP_ADDNAME;
> +		args.total = xfs_attr_calc_size(&args, &local);
> 
> -	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> -		return -EIO;
> +		/*
> +		 * If the inode doesn't have an attribute fork, add one.
> +		 * (inode must not be locked when we call this routine)
> +		 */
> +		if (XFS_IFORK_Q(dp) == 0) {
> +			int sf_size = sizeof(struct xfs_attr_sf_hdr) +
> +				XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen,
> +						valuelen);
> 
> -	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
> -	if (error)
> -		return error;
> +			error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
> +			if (error)
> +				return error;
> +		}
> 
> -	/*
> -	 * we have no control over the attribute names that userspace passes us
> -	 * to remove, so we have to allow the name lookup prior to attribute
> -	 * removal to fail.
> -	 */
> -	args.op_flags = XFS_DA_OP_OKNOENT;
> +		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> +				 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> +		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> +		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> +		total = args.total;
> +	} else {
> +		XFS_STATS_INC(mp, xs_attr_remove);
> 
> -	error = xfs_qm_dqattach(dp);
> -	if (error)
> -		return error;
> +		tres = M_RES(mp)->tr_attrrm;
> +		total = XFS_ATTRRM_SPACE_RES(mp);
> +	}
> 
>  	/*
>  	 * Root fork attributes can use reserved data blocks for this
>  	 * operation if necessary
>  	 */
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
> -			XFS_ATTRRM_SPACE_RES(mp), 0,
> -			(flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0,
> -			&args.trans);
> +	error = xfs_trans_alloc(mp, &tres, total, 0,
> +			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
>  	if (error)
>  		return error;
> 
>  	xfs_ilock(dp, XFS_ILOCK_EXCL);
> -	/*
> -	 * No need to make quota reservations here. We expect to release some
> -	 * blocks not allocate in the common case.
> -	 */
>  	xfs_trans_ijoin(args.trans, dp, 0);
> +	if (value) {
> +		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
> 
> -	error = xfs_attr_remove_args(&args);
> -	if (error)
> -		goto out;
> +		if (rsvd)
> +			quota_flags |= XFS_QMOPT_FORCE_RES;
> +		error = xfs_trans_reserve_quota_nblks(args.trans, dp,
> +				args.total, 0, quota_flags);
> +		if (error)
> +			goto out_trans_cancel;
> +		error = xfs_attr_set_args(&args);
> +		if (error)
> +			goto out_trans_cancel;
> +		/* shortform attribute has already been committed */
> +		if (!args.trans)
> +			goto out_unlock;
> +	} else {
> +		error = xfs_attr_remove_args(&args);
> +		if (error)
> +			goto out_trans_cancel;
> +	}
> 
>  	/*
>  	 * If this is a synchronous mount, make sure that the
> @@ -509,15 +456,14 @@ xfs_attr_remove(
>  	 */
>  	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
>  	error = xfs_trans_commit(args.trans);
> +out_unlock:
>  	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> -
>  	return error;
> 
> -out:
> +out_trans_cancel:
>  	if (args.trans)
>  		xfs_trans_cancel(args.trans);
> -	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> -	return error;
> +	goto out_unlock;
>  }
> 
>  /*========================================================================
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 71bcf1298e4c..db58a6c7dea5 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -152,8 +152,6 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
>  int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
>  		 size_t namelen, unsigned char *value, int valuelen, int flags);
>  int xfs_attr_set_args(struct xfs_da_args *args);
> -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 cd743fad8478..4e76063ff956 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -168,6 +168,8 @@ __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_acl *xfs_acl = NULL;
> +	int len = 0;
>  	int error;
> 
>  	switch (type) {
> @@ -184,9 +186,7 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	}
> 
>  	if (acl) {
> -		struct xfs_acl *xfs_acl;
> -		int len = XFS_ACL_MAX_SIZE(ip->i_mount);
> -
> +		len = XFS_ACL_MAX_SIZE(ip->i_mount);
>  		xfs_acl = kmem_zalloc_large(len, 0);
>  		if (!xfs_acl)
>  			return -ENOMEM;
> @@ -196,26 +196,17 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  		/* subtract away the unused acl entries */
>  		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);
> -
> -		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);
> -
> -		/*
> -		 * If the attribute didn't exist to start with that's fine.
> -		 */
> -		if (error == -ENOATTR)
> -			error = 0;
>  	}
> 
> +	error = xfs_attr_set(ip, ea_name, strlen(ea_name),
> +			(unsigned char *)xfs_acl, len, ATTR_ROOT);
> +	kmem_free(xfs_acl);
> +
> +	/*
> +	 * If the attribute didn't exist to start with that's fine.
> +	 */
> +	if (!acl && error == -ENOATTR)
> +		error = 0;
>  	if (!error)
>  		set_cached_acl(inode, type, acl);
>  	return error;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d974bf099d45..79c418888e9a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -417,12 +417,10 @@ xfs_attrmulti_attr_remove(
>  	uint32_t		flags)
>  {
>  	int			error;
> -	size_t			namelen;
> 
>  	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_set(XFS_I(inode), name, strlen(name), NULL, 0, flags);
>  	if (!error)
>  		xfs_forget_acl(inode, name, flags);
>  	return error;
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index b0fedb543f97..1670bfbc9ad2 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -69,7 +69,6 @@ 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,14 +76,10 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
>  	if (flags & XATTR_REPLACE)
>  		xflags |= ATTR_REPLACE;
> 
> -	if (value)
> -		error = xfs_attr_set(ip, name, namelen, (void *)value, size,
> -				xflags);
> -	else
> -		error = xfs_attr_remove(ip, name, namelen, xflags);
> +	error = xfs_attr_set(ip, (unsigned char *)name, strlen(name),
> +				(void *)value, size, xflags);
>  	if (!error)
>  		xfs_forget_acl(inode, name, xflags);
> -
>  	return error;
>  }
> 
> 


-- 
chandan

[PATCH 04/30] xfs: merge xfs_attrmulti_attr_remove into xfs_attrmulti_attr_set

[Christoph Hellwig]

Merge the ioctl handlers just like the low-level xfs_attr_set function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c   | 34 ++++++++++------------------------
 fs/xfs/xfs_ioctl.h   |  6 ------
 fs/xfs/xfs_ioctl32.c |  4 ++--
 3 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 79c418888e9a..b806003caacd 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -389,18 +389,20 @@ xfs_attrmulti_attr_set(
 	uint32_t		len,
 	uint32_t		flags)
 {
-	unsigned char		*kbuf;
+	unsigned char		*kbuf = NULL;
 	int			error;
 	size_t			namelen;
 
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return -EPERM;
-	if (len > XFS_XATTR_SIZE_MAX)
-		return -EINVAL;
 
-	kbuf = memdup_user(ubuf, len);
-	if (IS_ERR(kbuf))
-		return PTR_ERR(kbuf);
+	if (ubuf) {
+		if (len > XFS_XATTR_SIZE_MAX)
+			return -EINVAL;
+		kbuf = memdup_user(ubuf, len);
+		if (IS_ERR(kbuf))
+			return PTR_ERR(kbuf);
+	}
 
 	namelen = strlen(name);
 	error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
@@ -410,22 +412,6 @@ xfs_attrmulti_attr_set(
 	return error;
 }
 
-int
-xfs_attrmulti_attr_remove(
-	struct inode		*inode,
-	unsigned char		*name,
-	uint32_t		flags)
-{
-	int			error;
-
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		return -EPERM;
-	error = xfs_attr_set(XFS_I(inode), name, strlen(name), NULL, 0, flags);
-	if (!error)
-		xfs_forget_acl(inode, name, flags);
-	return error;
-}
-
 STATIC int
 xfs_attrmulti_by_handle(
 	struct file		*parfilp,
@@ -504,8 +490,8 @@ xfs_attrmulti_by_handle(
 			ops[i].am_error = mnt_want_write_file(parfilp);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_remove(
-					d_inode(dentry), attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					d_inode(dentry), attr_name, NULL, 0,
 					ops[i].am_flags);
 			mnt_drop_write_file(parfilp);
 			break;
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index 420bd95dc326..819504df00ae 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -46,12 +46,6 @@ xfs_attrmulti_attr_set(
 	uint32_t		len,
 	uint32_t		flags);
 
-extern int
-xfs_attrmulti_attr_remove(
-	struct inode		*inode,
-	unsigned char		*name,
-	uint32_t		flags);
-
 extern struct dentry *
 xfs_handle_to_dentry(
 	struct file		*parfilp,
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 9705172e5410..e085f304e539 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -488,8 +488,8 @@ xfs_compat_attrmulti_by_handle(
 			ops[i].am_error = mnt_want_write_file(parfilp);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_remove(
-					d_inode(dentry), attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					d_inode(dentry), attr_name, NULL, 0,
 					ops[i].am_flags);
 			mnt_drop_write_file(parfilp);
 			break;
-- 
2.24.1

Re: [PATCH 04/30] xfs: merge xfs_attrmulti_attr_remove into xfs_attrmulti_attr_set

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 
> Merge the ioctl handlers just like the low-level xfs_attr_set function.
>

The newly introduced changes match with the code flow that earlier existed
separately in xfs_attrmulti_attr_set() and xfs_attrmulti_attr_remove().

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c   | 34 ++++++++++------------------------
>  fs/xfs/xfs_ioctl.h   |  6 ------
>  fs/xfs/xfs_ioctl32.c |  4 ++--
>  3 files changed, 12 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 79c418888e9a..b806003caacd 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -389,18 +389,20 @@ xfs_attrmulti_attr_set(
>  	uint32_t		len,
>  	uint32_t		flags)
>  {
> -	unsigned char		*kbuf;
> +	unsigned char		*kbuf = NULL;
>  	int			error;
>  	size_t			namelen;
> 
>  	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
>  		return -EPERM;
> -	if (len > XFS_XATTR_SIZE_MAX)
> -		return -EINVAL;
> 
> -	kbuf = memdup_user(ubuf, len);
> -	if (IS_ERR(kbuf))
> -		return PTR_ERR(kbuf);
> +	if (ubuf) {
> +		if (len > XFS_XATTR_SIZE_MAX)
> +			return -EINVAL;
> +		kbuf = memdup_user(ubuf, len);
> +		if (IS_ERR(kbuf))
> +			return PTR_ERR(kbuf);
> +	}
> 
>  	namelen = strlen(name);
>  	error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
> @@ -410,22 +412,6 @@ xfs_attrmulti_attr_set(
>  	return error;
>  }
> 
> -int
> -xfs_attrmulti_attr_remove(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	uint32_t		flags)
> -{
> -	int			error;
> -
> -	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> -		return -EPERM;
> -	error = xfs_attr_set(XFS_I(inode), name, strlen(name), NULL, 0, flags);
> -	if (!error)
> -		xfs_forget_acl(inode, name, flags);
> -	return error;
> -}
> -
>  STATIC int
>  xfs_attrmulti_by_handle(
>  	struct file		*parfilp,
> @@ -504,8 +490,8 @@ xfs_attrmulti_by_handle(
>  			ops[i].am_error = mnt_want_write_file(parfilp);
>  			if (ops[i].am_error)
>  				break;
> -			ops[i].am_error = xfs_attrmulti_attr_remove(
> -					d_inode(dentry), attr_name,
> +			ops[i].am_error = xfs_attrmulti_attr_set(
> +					d_inode(dentry), attr_name, NULL, 0,
>  					ops[i].am_flags);
>  			mnt_drop_write_file(parfilp);
>  			break;
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index 420bd95dc326..819504df00ae 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -46,12 +46,6 @@ xfs_attrmulti_attr_set(
>  	uint32_t		len,
>  	uint32_t		flags);
> 
> -extern int
> -xfs_attrmulti_attr_remove(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	uint32_t		flags);
> -
>  extern struct dentry *
>  xfs_handle_to_dentry(
>  	struct file		*parfilp,
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 9705172e5410..e085f304e539 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -488,8 +488,8 @@ xfs_compat_attrmulti_by_handle(
>  			ops[i].am_error = mnt_want_write_file(parfilp);
>  			if (ops[i].am_error)
>  				break;
> -			ops[i].am_error = xfs_attrmulti_attr_remove(
> -					d_inode(dentry), attr_name,
> +			ops[i].am_error = xfs_attrmulti_attr_set(
> +					d_inode(dentry), attr_name, NULL, 0,
>  					ops[i].am_flags);
>  			mnt_drop_write_file(parfilp);
>  			break;
> 


-- 
chandan

[PATCH 05/30] xfs: use strndup_user in XFS_IOC_ATTRMULTI_BY_HANDLE

[Christoph Hellwig]

Simplify the user copy code by using strndup_user.  This means that we
now do one memory allocation per operation instead of one per ioctl,
but memory allocations are cheap compared to the actual file system
operations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c   | 17 +++++------------
 fs/xfs/xfs_ioctl32.c | 17 +++++------------
 2 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index b806003caacd..bb490a954c0b 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -448,11 +448,6 @@ xfs_attrmulti_by_handle(
 		goto out_dput;
 	}
 
-	error = -ENOMEM;
-	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
-	if (!attr_name)
-		goto out_kfree_ops;
-
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
 		if ((ops[i].am_flags & ATTR_ROOT) &&
@@ -462,12 +457,11 @@ xfs_attrmulti_by_handle(
 		}
 		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
 
-		ops[i].am_error = strncpy_from_user((char *)attr_name,
-				ops[i].am_attrname, MAXNAMELEN);
-		if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
-			error = -ERANGE;
-		if (ops[i].am_error < 0)
+		attr_name = strndup_user(ops[i].am_attrname, MAXNAMELEN);
+		if (IS_ERR(attr_name)) {
+			ops[i].am_error = PTR_ERR(attr_name);
 			break;
+		}
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
@@ -498,13 +492,12 @@ xfs_attrmulti_by_handle(
 		default:
 			ops[i].am_error = -EINVAL;
 		}
+		kfree(attr_name);
 	}
 
 	if (copy_to_user(am_hreq.ops, ops, size))
 		error = -EFAULT;
 
-	kfree(attr_name);
- out_kfree_ops:
 	kfree(ops);
  out_dput:
 	dput(dentry);
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index e085f304e539..936c2f62fb6c 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -445,11 +445,6 @@ xfs_compat_attrmulti_by_handle(
 		goto out_dput;
 	}
 
-	error = -ENOMEM;
-	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
-	if (!attr_name)
-		goto out_kfree_ops;
-
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
 		if ((ops[i].am_flags & ATTR_ROOT) &&
@@ -459,13 +454,12 @@ xfs_compat_attrmulti_by_handle(
 		}
 		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
 
-		ops[i].am_error = strncpy_from_user((char *)attr_name,
-				compat_ptr(ops[i].am_attrname),
+		attr_name = strndup_user(compat_ptr(ops[i].am_attrname),
 				MAXNAMELEN);
-		if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
-			error = -ERANGE;
-		if (ops[i].am_error < 0)
+		if (IS_ERR(attr_name)) {
+			ops[i].am_error = PTR_ERR(attr_name);
 			break;
+		}
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
@@ -496,13 +490,12 @@ xfs_compat_attrmulti_by_handle(
 		default:
 			ops[i].am_error = -EINVAL;
 		}
+		kfree(attr_name);
 	}
 
 	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
 		error = -EFAULT;
 
-	kfree(attr_name);
- out_kfree_ops:
 	kfree(ops);
  out_dput:
 	dput(dentry);
-- 
2.24.1

Re: [PATCH 05/30] xfs: use strndup_user in XFS_IOC_ATTRMULTI_BY_HANDLE

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 

> Simplify the user copy code by using strndup_user.  This means that we
> now do one memory allocation per operation instead of one per ioctl,
> but memory allocations are cheap compared to the actual file system
> operations.
>

The newly introduced changes logically match with the code flow that existed
earlier.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c   | 17 +++++------------
>  fs/xfs/xfs_ioctl32.c | 17 +++++------------
>  2 files changed, 10 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index b806003caacd..bb490a954c0b 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -448,11 +448,6 @@ xfs_attrmulti_by_handle(
>  		goto out_dput;
>  	}
> 
> -	error = -ENOMEM;
> -	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> -	if (!attr_name)
> -		goto out_kfree_ops;
> -
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
>  		if ((ops[i].am_flags & ATTR_ROOT) &&
> @@ -462,12 +457,11 @@ xfs_attrmulti_by_handle(
>  		}
>  		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> 
> -		ops[i].am_error = strncpy_from_user((char *)attr_name,
> -				ops[i].am_attrname, MAXNAMELEN);
> -		if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> -			error = -ERANGE;
> -		if (ops[i].am_error < 0)
> +		attr_name = strndup_user(ops[i].am_attrname, MAXNAMELEN);
> +		if (IS_ERR(attr_name)) {
> +			ops[i].am_error = PTR_ERR(attr_name);
>  			break;
> +		}
> 
>  		switch (ops[i].am_opcode) {
>  		case ATTR_OP_GET:
> @@ -498,13 +492,12 @@ xfs_attrmulti_by_handle(
>  		default:
>  			ops[i].am_error = -EINVAL;
>  		}
> +		kfree(attr_name);
>  	}
> 
>  	if (copy_to_user(am_hreq.ops, ops, size))
>  		error = -EFAULT;
> 
> -	kfree(attr_name);
> - out_kfree_ops:
>  	kfree(ops);
>   out_dput:
>  	dput(dentry);
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index e085f304e539..936c2f62fb6c 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -445,11 +445,6 @@ xfs_compat_attrmulti_by_handle(
>  		goto out_dput;
>  	}
> 
> -	error = -ENOMEM;
> -	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> -	if (!attr_name)
> -		goto out_kfree_ops;
> -
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
>  		if ((ops[i].am_flags & ATTR_ROOT) &&
> @@ -459,13 +454,12 @@ xfs_compat_attrmulti_by_handle(
>  		}
>  		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> 
> -		ops[i].am_error = strncpy_from_user((char *)attr_name,
> -				compat_ptr(ops[i].am_attrname),
> +		attr_name = strndup_user(compat_ptr(ops[i].am_attrname),
>  				MAXNAMELEN);
> -		if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> -			error = -ERANGE;
> -		if (ops[i].am_error < 0)
> +		if (IS_ERR(attr_name)) {
> +			ops[i].am_error = PTR_ERR(attr_name);
>  			break;
> +		}
> 
>  		switch (ops[i].am_opcode) {
>  		case ATTR_OP_GET:
> @@ -496,13 +490,12 @@ xfs_compat_attrmulti_by_handle(
>  		default:
>  			ops[i].am_error = -EINVAL;
>  		}
> +		kfree(attr_name);
>  	}
> 
>  	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
>  		error = -EFAULT;
> 
> -	kfree(attr_name);
> - out_kfree_ops:
>  	kfree(ops);
>   out_dput:
>  	dput(dentry);
> 

-- 
chandan

[PATCH 06/30] xfs: factor out a helper for a single XFS_IOC_ATTRMULTI_BY_HANDLE op

[Christoph Hellwig]

Add a new helper to handle a single attr multi ioctl operation that
can be shared between the native and compat ioctl implementation.

There is a slight change in heavior in that we don't break out of the
loop when copying in the attribute name fails.  The previous behavior
was rather inconsistent here as it continued for any other kind of
error.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c   | 97 +++++++++++++++++++++++---------------------
 fs/xfs/xfs_ioctl.h   | 18 ++------
 fs/xfs/xfs_ioctl32.c | 50 +++--------------------
 3 files changed, 59 insertions(+), 106 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index bb490a954c0b..cfdd80b4ea2d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -349,7 +349,7 @@ xfs_attrlist_by_handle(
 	return error;
 }
 
-int
+static int
 xfs_attrmulti_attr_get(
 	struct inode		*inode,
 	unsigned char		*name,
@@ -381,7 +381,7 @@ xfs_attrmulti_attr_get(
 	return error;
 }
 
-int
+static int
 xfs_attrmulti_attr_set(
 	struct inode		*inode,
 	unsigned char		*name,
@@ -412,6 +412,51 @@ xfs_attrmulti_attr_set(
 	return error;
 }
 
+int
+xfs_ioc_attrmulti_one(
+	struct file		*parfilp,
+	struct inode		*inode,
+	uint32_t		opcode,
+	void __user		*uname,
+	void __user		*value,
+	uint32_t		*len,
+	uint32_t		flags)
+{
+	unsigned char		*name;
+	int			error;
+
+	if ((flags & ATTR_ROOT) && (flags & ATTR_SECURE))
+		return -EINVAL;
+	flags &= ~ATTR_KERNEL_FLAGS;
+
+	name = strndup_user(uname, MAXNAMELEN);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	switch (opcode) {
+	case ATTR_OP_GET:
+		error = xfs_attrmulti_attr_get(inode, name, value, len, flags);
+		break;
+	case ATTR_OP_REMOVE:
+		value = NULL;
+		*len = 0;
+		/*FALLTHRU*/
+	case ATTR_OP_SET:
+		error = mnt_want_write_file(parfilp);
+		if (error)
+			break;
+		error = xfs_attrmulti_attr_set(inode, name, value, *len, flags);
+		mnt_drop_write_file(parfilp);
+		break;
+	default:
+		error = -EINVAL;
+		break;
+	}
+
+	kfree(name);
+	return error;
+}
+
 STATIC int
 xfs_attrmulti_by_handle(
 	struct file		*parfilp,
@@ -422,7 +467,6 @@ xfs_attrmulti_by_handle(
 	xfs_fsop_attrmulti_handlereq_t am_hreq;
 	struct dentry		*dentry;
 	unsigned int		i, size;
-	unsigned char		*attr_name;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -450,49 +494,10 @@ xfs_attrmulti_by_handle(
 
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
-		if ((ops[i].am_flags & ATTR_ROOT) &&
-		    (ops[i].am_flags & ATTR_SECURE)) {
-			ops[i].am_error = -EINVAL;
-			continue;
-		}
-		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
-
-		attr_name = strndup_user(ops[i].am_attrname, MAXNAMELEN);
-		if (IS_ERR(attr_name)) {
-			ops[i].am_error = PTR_ERR(attr_name);
-			break;
-		}
-
-		switch (ops[i].am_opcode) {
-		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(
-					d_inode(dentry), attr_name,
-					ops[i].am_attrvalue, &ops[i].am_length,
-					ops[i].am_flags);
-			break;
-		case ATTR_OP_SET:
-			ops[i].am_error = mnt_want_write_file(parfilp);
-			if (ops[i].am_error)
-				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(
-					d_inode(dentry), attr_name,
-					ops[i].am_attrvalue, ops[i].am_length,
-					ops[i].am_flags);
-			mnt_drop_write_file(parfilp);
-			break;
-		case ATTR_OP_REMOVE:
-			ops[i].am_error = mnt_want_write_file(parfilp);
-			if (ops[i].am_error)
-				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(
-					d_inode(dentry), attr_name, NULL, 0,
-					ops[i].am_flags);
-			mnt_drop_write_file(parfilp);
-			break;
-		default:
-			ops[i].am_error = -EINVAL;
-		}
-		kfree(attr_name);
+		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
+				d_inode(dentry), ops[i].am_opcode,
+				ops[i].am_attrname, ops[i].am_attrvalue,
+				&ops[i].am_length, ops[i].am_flags);
 	}
 
 	if (copy_to_user(am_hreq.ops, ops, size))
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index 819504df00ae..bb50cb3dc61f 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -30,21 +30,9 @@ xfs_readlink_by_handle(
 	struct file		*parfilp,
 	xfs_fsop_handlereq_t	*hreq);
 
-extern int
-xfs_attrmulti_attr_get(
-	struct inode		*inode,
-	unsigned char		*name,
-	unsigned char		__user *ubuf,
-	uint32_t		*len,
-	uint32_t		flags);
-
-extern int
-xfs_attrmulti_attr_set(
-	struct inode		*inode,
-	unsigned char		*name,
-	const unsigned char	__user *ubuf,
-	uint32_t		len,
-	uint32_t		flags);
+int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
+		uint32_t opcode, void __user *uname, void __user *value,
+		uint32_t *len, uint32_t flags);
 
 extern struct dentry *
 xfs_handle_to_dentry(
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 936c2f62fb6c..e1daf095c585 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -418,7 +418,6 @@ xfs_compat_attrmulti_by_handle(
 	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
 	struct dentry				*dentry;
 	unsigned int				i, size;
-	unsigned char				*attr_name;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -447,50 +446,11 @@ xfs_compat_attrmulti_by_handle(
 
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
-		if ((ops[i].am_flags & ATTR_ROOT) &&
-		    (ops[i].am_flags & ATTR_SECURE)) {
-			ops[i].am_error = -EINVAL;
-			continue;
-		}
-		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
-
-		attr_name = strndup_user(compat_ptr(ops[i].am_attrname),
-				MAXNAMELEN);
-		if (IS_ERR(attr_name)) {
-			ops[i].am_error = PTR_ERR(attr_name);
-			break;
-		}
-
-		switch (ops[i].am_opcode) {
-		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(
-					d_inode(dentry), attr_name,
-					compat_ptr(ops[i].am_attrvalue),
-					&ops[i].am_length, ops[i].am_flags);
-			break;
-		case ATTR_OP_SET:
-			ops[i].am_error = mnt_want_write_file(parfilp);
-			if (ops[i].am_error)
-				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(
-					d_inode(dentry), attr_name,
-					compat_ptr(ops[i].am_attrvalue),
-					ops[i].am_length, ops[i].am_flags);
-			mnt_drop_write_file(parfilp);
-			break;
-		case ATTR_OP_REMOVE:
-			ops[i].am_error = mnt_want_write_file(parfilp);
-			if (ops[i].am_error)
-				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(
-					d_inode(dentry), attr_name, NULL, 0,
-					ops[i].am_flags);
-			mnt_drop_write_file(parfilp);
-			break;
-		default:
-			ops[i].am_error = -EINVAL;
-		}
-		kfree(attr_name);
+		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
+				d_inode(dentry), ops[i].am_opcode,
+				compat_ptr(ops[i].am_attrname),
+				compat_ptr(ops[i].am_attrvalue),
+				&ops[i].am_length, ops[i].am_flags);
 	}
 
 	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
-- 
2.24.1

Re: [PATCH 06/30] xfs: factor out a helper for a single XFS_IOC_ATTRMULTI_BY_HANDLE op

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 
> Add a new helper to handle a single attr multi ioctl operation that
> can be shared between the native and compat ioctl implementation.
> 
> There is a slight change in heavior in that we don't break out of the
> loop when copying in the attribute name fails.  The previous behavior
> was rather inconsistent here as it continued for any other kind of
> error.
>

Apart from "not breaking out of the for loop" change, The other changes
logically match with the code flow that existed earlier.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c   | 97 +++++++++++++++++++++++---------------------
>  fs/xfs/xfs_ioctl.h   | 18 ++------
>  fs/xfs/xfs_ioctl32.c | 50 +++--------------------
>  3 files changed, 59 insertions(+), 106 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index bb490a954c0b..cfdd80b4ea2d 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -349,7 +349,7 @@ xfs_attrlist_by_handle(
>  	return error;
>  }
> 
> -int
> +static int
>  xfs_attrmulti_attr_get(
>  	struct inode		*inode,
>  	unsigned char		*name,
> @@ -381,7 +381,7 @@ xfs_attrmulti_attr_get(
>  	return error;
>  }
> 
> -int
> +static int
>  xfs_attrmulti_attr_set(
>  	struct inode		*inode,
>  	unsigned char		*name,
> @@ -412,6 +412,51 @@ xfs_attrmulti_attr_set(
>  	return error;
>  }
> 
> +int
> +xfs_ioc_attrmulti_one(
> +	struct file		*parfilp,
> +	struct inode		*inode,
> +	uint32_t		opcode,
> +	void __user		*uname,
> +	void __user		*value,
> +	uint32_t		*len,
> +	uint32_t		flags)
> +{
> +	unsigned char		*name;
> +	int			error;
> +
> +	if ((flags & ATTR_ROOT) && (flags & ATTR_SECURE))
> +		return -EINVAL;
> +	flags &= ~ATTR_KERNEL_FLAGS;
> +
> +	name = strndup_user(uname, MAXNAMELEN);
> +	if (IS_ERR(name))
> +		return PTR_ERR(name);
> +
> +	switch (opcode) {
> +	case ATTR_OP_GET:
> +		error = xfs_attrmulti_attr_get(inode, name, value, len, flags);
> +		break;
> +	case ATTR_OP_REMOVE:
> +		value = NULL;
> +		*len = 0;
> +		/*FALLTHRU*/
> +	case ATTR_OP_SET:
> +		error = mnt_want_write_file(parfilp);
> +		if (error)
> +			break;
> +		error = xfs_attrmulti_attr_set(inode, name, value, *len, flags);
> +		mnt_drop_write_file(parfilp);
> +		break;
> +	default:
> +		error = -EINVAL;
> +		break;
> +	}
> +
> +	kfree(name);
> +	return error;
> +}
> +
>  STATIC int
>  xfs_attrmulti_by_handle(
>  	struct file		*parfilp,
> @@ -422,7 +467,6 @@ xfs_attrmulti_by_handle(
>  	xfs_fsop_attrmulti_handlereq_t am_hreq;
>  	struct dentry		*dentry;
>  	unsigned int		i, size;
> -	unsigned char		*attr_name;
> 
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -450,49 +494,10 @@ xfs_attrmulti_by_handle(
> 
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
> -		if ((ops[i].am_flags & ATTR_ROOT) &&
> -		    (ops[i].am_flags & ATTR_SECURE)) {
> -			ops[i].am_error = -EINVAL;
> -			continue;
> -		}
> -		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> -
> -		attr_name = strndup_user(ops[i].am_attrname, MAXNAMELEN);
> -		if (IS_ERR(attr_name)) {
> -			ops[i].am_error = PTR_ERR(attr_name);
> -			break;
> -		}
> -
> -		switch (ops[i].am_opcode) {
> -		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(
> -					d_inode(dentry), attr_name,
> -					ops[i].am_attrvalue, &ops[i].am_length,
> -					ops[i].am_flags);
> -			break;
> -		case ATTR_OP_SET:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name,
> -					ops[i].am_attrvalue, ops[i].am_length,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		case ATTR_OP_REMOVE:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name, NULL, 0,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		default:
> -			ops[i].am_error = -EINVAL;
> -		}
> -		kfree(attr_name);
> +		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
> +				d_inode(dentry), ops[i].am_opcode,
> +				ops[i].am_attrname, ops[i].am_attrvalue,
> +				&ops[i].am_length, ops[i].am_flags);
>  	}
> 
>  	if (copy_to_user(am_hreq.ops, ops, size))
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index 819504df00ae..bb50cb3dc61f 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -30,21 +30,9 @@ xfs_readlink_by_handle(
>  	struct file		*parfilp,
>  	xfs_fsop_handlereq_t	*hreq);
> 
> -extern int
> -xfs_attrmulti_attr_get(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	unsigned char		__user *ubuf,
> -	uint32_t		*len,
> -	uint32_t		flags);
> -
> -extern int
> -xfs_attrmulti_attr_set(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	const unsigned char	__user *ubuf,
> -	uint32_t		len,
> -	uint32_t		flags);
> +int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
> +		uint32_t opcode, void __user *uname, void __user *value,
> +		uint32_t *len, uint32_t flags);
> 
>  extern struct dentry *
>  xfs_handle_to_dentry(
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 936c2f62fb6c..e1daf095c585 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -418,7 +418,6 @@ xfs_compat_attrmulti_by_handle(
>  	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
>  	struct dentry				*dentry;
>  	unsigned int				i, size;
> -	unsigned char				*attr_name;
> 
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -447,50 +446,11 @@ xfs_compat_attrmulti_by_handle(
> 
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
> -		if ((ops[i].am_flags & ATTR_ROOT) &&
> -		    (ops[i].am_flags & ATTR_SECURE)) {
> -			ops[i].am_error = -EINVAL;
> -			continue;
> -		}
> -		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> -
> -		attr_name = strndup_user(compat_ptr(ops[i].am_attrname),
> -				MAXNAMELEN);
> -		if (IS_ERR(attr_name)) {
> -			ops[i].am_error = PTR_ERR(attr_name);
> -			break;
> -		}
> -
> -		switch (ops[i].am_opcode) {
> -		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(
> -					d_inode(dentry), attr_name,
> -					compat_ptr(ops[i].am_attrvalue),
> -					&ops[i].am_length, ops[i].am_flags);
> -			break;
> -		case ATTR_OP_SET:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name,
> -					compat_ptr(ops[i].am_attrvalue),
> -					ops[i].am_length, ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		case ATTR_OP_REMOVE:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name, NULL, 0,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		default:
> -			ops[i].am_error = -EINVAL;
> -		}
> -		kfree(attr_name);
> +		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
> +				d_inode(dentry), ops[i].am_opcode,
> +				compat_ptr(ops[i].am_attrname),
> +				compat_ptr(ops[i].am_attrvalue),
> +				&ops[i].am_length, ops[i].am_flags);
>  	}
> 
>  	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
> 


-- 
chandan

[PATCH 07/30] xfs: remove the name == NULL check from xfs_attr_args_init

[Christoph Hellwig]

All callers provide a valid name pointer, remove the redundant check.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index bb391b96cd78..a968158b9bb1 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -65,10 +65,6 @@ xfs_attr_args_init(
 	size_t			namelen,
 	int			flags)
 {
-
-	if (!name)
-		return -EINVAL;
-
 	memset(args, 0, sizeof(*args));
 	args->geo = dp->i_mount->m_attr_geo;
 	args->whichfork = XFS_ATTR_FORK;
-- 
2.24.1

Re: [PATCH 07/30] xfs: remove the name == NULL check from xfs_attr_args_init

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 

> All callers provide a valid name pointer, remove the redundant check.
>

I went through the callers of xfs_attr_args_init() (and in turn to callers of
those callers) and found that 'name' arg can indeed never be NULL.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index bb391b96cd78..a968158b9bb1 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -65,10 +65,6 @@ xfs_attr_args_init(
>  	size_t			namelen,
>  	int			flags)
>  {
> -
> -	if (!name)
> -		return -EINVAL;
> -
>  	memset(args, 0, sizeof(*args));
>  	args->geo = dp->i_mount->m_attr_geo;
>  	args->whichfork = XFS_ATTR_FORK;
> 


-- 
chandan

[PATCH 08/30] xfs: remove the MAXNAMELEN check from xfs_attr_args_init

[Christoph Hellwig]

All the callers already check the length when allocating the
in-kernel xattrs buffers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index a968158b9bb1..f887d62e0956 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -72,9 +72,6 @@ xfs_attr_args_init(
 	args->flags = flags;
 	args->name = name;
 	args->namelen = namelen;
-	if (args->namelen >= MAXNAMELEN)
-		return -EFAULT;		/* match IRIX behaviour */
-
 	args->hashval = xfs_da_hashname(args->name, args->namelen);
 	return 0;
 }
-- 
2.24.1

Re: [PATCH 08/30] xfs: remove the MAXNAMELEN check from xfs_attr_args_init

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 
> All the callers already check the length when allocating the
> in-kernel xattrs buffers.
>

I checked all the callers apart from xfs_init_security(). For the ones I
checked,

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index a968158b9bb1..f887d62e0956 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -72,9 +72,6 @@ xfs_attr_args_init(
>  	args->flags = flags;
>  	args->name = name;
>  	args->namelen = namelen;
> -	if (args->namelen >= MAXNAMELEN)
> -		return -EFAULT;		/* match IRIX behaviour */
> -
>  	args->hashval = xfs_da_hashname(args->name, args->namelen);
>  	return 0;
>  }
> 


-- 
chandan

[PATCH 09/30] xfs: move struct xfs_da_args to xfs_types.h

[Christoph Hellwig]

To allow passing a struct xfs_da_args to the high-level attr helpers
it needs to be easily includable by files like xfs_xattr.c.  Move the
struct definition to xfs_types.h to allow for that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_da_btree.h | 64 ------------------------------------
 fs/xfs/libxfs/xfs_types.h    | 60 +++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 64 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index 0f4fbb0889ff..dd2f48b8ee07 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -36,70 +36,6 @@ struct xfs_da_geometry {
 	size_t		data_entry_offset;
 };
 
-/*========================================================================
- * Btree searching and modification structure definitions.
- *========================================================================*/
-
-/*
- * Search comparison results
- */
-enum xfs_dacmp {
-	XFS_CMP_DIFFERENT,	/* names are completely different */
-	XFS_CMP_EXACT,		/* names are exactly the same */
-	XFS_CMP_CASE		/* names are same but differ in case */
-};
-
-/*
- * Structure to ease passing around component names.
- */
-typedef struct xfs_da_args {
-	struct xfs_da_geometry *geo;	/* da block geometry */
-	const uint8_t		*name;		/* string (maybe not NULL terminated) */
-	int		namelen;	/* length of string (maybe no NULL) */
-	uint8_t		filetype;	/* filetype of inode for directories */
-	uint8_t		*value;		/* set of bytes (maybe contain NULLs) */
-	int		valuelen;	/* length of value */
-	int		flags;		/* argument flags (eg: ATTR_NOCREATE) */
-	xfs_dahash_t	hashval;	/* hash value of name */
-	xfs_ino_t	inumber;	/* input/output inode number */
-	struct xfs_inode *dp;		/* directory inode to manipulate */
-	struct xfs_trans *trans;	/* current trans (changes over time) */
-	xfs_extlen_t	total;		/* total blocks needed, for 1st bmap */
-	int		whichfork;	/* data or attribute fork */
-	xfs_dablk_t	blkno;		/* blkno of attr leaf of interest */
-	int		index;		/* index of attr of interest in blk */
-	xfs_dablk_t	rmtblkno;	/* remote attr value starting blkno */
-	int		rmtblkcnt;	/* remote attr value block count */
-	int		rmtvaluelen;	/* remote attr value length in bytes */
-	xfs_dablk_t	blkno2;		/* blkno of 2nd attr leaf of interest */
-	int		index2;		/* index of 2nd attr in blk */
-	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
-	int		rmtblkcnt2;	/* remote attr value block count */
-	int		rmtvaluelen2;	/* remote attr value length in bytes */
-	int		op_flags;	/* operation flags */
-	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
-} xfs_da_args_t;
-
-/*
- * Operation flags:
- */
-#define XFS_DA_OP_JUSTCHECK	0x0001	/* check for ok with no space */
-#define XFS_DA_OP_RENAME	0x0002	/* this is an atomic rename op */
-#define XFS_DA_OP_ADDNAME	0x0004	/* this is an add operation */
-#define XFS_DA_OP_OKNOENT	0x0008	/* lookup/add op, ENOENT ok, else die */
-#define XFS_DA_OP_CILOOKUP	0x0010	/* lookup to return CI name if found */
-#define XFS_DA_OP_ALLOCVAL	0x0020	/* lookup to alloc buffer if found  */
-#define XFS_DA_OP_INCOMPLETE	0x0040	/* lookup INCOMPLETE attr keys */
-
-#define XFS_DA_OP_FLAGS \
-	{ XFS_DA_OP_JUSTCHECK,	"JUSTCHECK" }, \
-	{ XFS_DA_OP_RENAME,	"RENAME" }, \
-	{ XFS_DA_OP_ADDNAME,	"ADDNAME" }, \
-	{ XFS_DA_OP_OKNOENT,	"OKNOENT" }, \
-	{ XFS_DA_OP_CILOOKUP,	"CILOOKUP" }, \
-	{ XFS_DA_OP_ALLOCVAL,	"ALLOCVAL" }, \
-	{ XFS_DA_OP_INCOMPLETE,	"INCOMPLETE" }
-
 /*
  * Storage for holding state during Btree searches and split/join ops.
  *
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 397d94775440..e2711d119665 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -175,6 +175,66 @@ enum xfs_ag_resv_type {
 	XFS_AG_RESV_RMAPBT,
 };
 
+/*
+ * Dir/attr btree search comparison results.
+ */
+enum xfs_dacmp {
+	XFS_CMP_DIFFERENT,	/* names are completely different */
+	XFS_CMP_EXACT,		/* names are exactly the same */
+	XFS_CMP_CASE		/* names are same but differ in case */
+};
+
+/*
+ * Structure to ease passing around dir/attr component names.
+ */
+typedef struct xfs_da_args {
+	struct xfs_da_geometry *geo;	/* da block geometry */
+	const uint8_t	*name;		/* string (maybe not NULL terminated) */
+	int		namelen;	/* length of string (maybe no NULL) */
+	uint8_t		filetype;	/* filetype of inode for directories */
+	uint8_t		*value;		/* set of bytes (maybe contain NULLs) */
+	int		valuelen;	/* length of value */
+	int		flags;		/* argument flags (eg: ATTR_NOCREATE) */
+	xfs_dahash_t	hashval;	/* hash value of name */
+	xfs_ino_t	inumber;	/* input/output inode number */
+	struct xfs_inode *dp;		/* directory inode to manipulate */
+	struct xfs_trans *trans;	/* current trans (changes over time) */
+	xfs_extlen_t	total;		/* total blocks needed, for 1st bmap */
+	int		whichfork;	/* data or attribute fork */
+	xfs_dablk_t	blkno;		/* blkno of attr leaf of interest */
+	int		index;		/* index of attr of interest in blk */
+	xfs_dablk_t	rmtblkno;	/* remote attr value starting blkno */
+	int		rmtblkcnt;	/* remote attr value block count */
+	int		rmtvaluelen;	/* remote attr value length in bytes */
+	xfs_dablk_t	blkno2;		/* blkno of 2nd attr leaf of interest */
+	int		index2;		/* index of 2nd attr in blk */
+	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
+	int		rmtblkcnt2;	/* remote attr value block count */
+	int		rmtvaluelen2;	/* remote attr value length in bytes */
+	int		op_flags;	/* operation flags */
+	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
+} xfs_da_args_t;
+
+/*
+ * Operation flags:
+ */
+#define XFS_DA_OP_JUSTCHECK	0x0001	/* check for ok with no space */
+#define XFS_DA_OP_RENAME	0x0002	/* this is an atomic rename op */
+#define XFS_DA_OP_ADDNAME	0x0004	/* this is an add operation */
+#define XFS_DA_OP_OKNOENT	0x0008	/* lookup/add op, ENOENT ok, else die */
+#define XFS_DA_OP_CILOOKUP	0x0010	/* lookup to return CI name if found */
+#define XFS_DA_OP_ALLOCVAL	0x0020	/* lookup to alloc buffer if found  */
+#define XFS_DA_OP_INCOMPLETE	0x0040	/* lookup INCOMPLETE attr keys */
+
+#define XFS_DA_OP_FLAGS \
+	{ XFS_DA_OP_JUSTCHECK,	"JUSTCHECK" }, \
+	{ XFS_DA_OP_RENAME,	"RENAME" }, \
+	{ XFS_DA_OP_ADDNAME,	"ADDNAME" }, \
+	{ XFS_DA_OP_OKNOENT,	"OKNOENT" }, \
+	{ XFS_DA_OP_CILOOKUP,	"CILOOKUP" }, \
+	{ XFS_DA_OP_ALLOCVAL,	"ALLOCVAL" }, \
+	{ XFS_DA_OP_INCOMPLETE,	"INCOMPLETE" }
+
 /*
  * Type verifier functions
  */
-- 
2.24.1

[PATCH 10/30] xfs: turn xfs_da_args.value into a void pointer

[Christoph Hellwig]

The xattr values are blobs and should not be typed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index e2711d119665..634814dd1d10 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -192,7 +192,7 @@ typedef struct xfs_da_args {
 	const uint8_t	*name;		/* string (maybe not NULL terminated) */
 	int		namelen;	/* length of string (maybe no NULL) */
 	uint8_t		filetype;	/* filetype of inode for directories */
-	uint8_t		*value;		/* set of bytes (maybe contain NULLs) */
+	void		*value;		/* set of bytes (maybe contain NULLs) */
 	int		valuelen;	/* length of value */
 	int		flags;		/* argument flags (eg: ATTR_NOCREATE) */
 	xfs_dahash_t	hashval;	/* hash value of name */
-- 
2.24.1

[PATCH 11/30] xfs: pass an initialized xfs_da_args structure to xfs_attr_set

[Christoph Hellwig]

Instead of converting from one style of arguments to another in
xfs_attr_set, pass the structure from higher up in the call chain.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 69 ++++++++++++++++++----------------------
 fs/xfs/libxfs/xfs_attr.h |  3 +-
 fs/xfs/xfs_acl.c         | 31 +++++++++---------
 fs/xfs/xfs_ioctl.c       | 20 +++++++-----
 fs/xfs/xfs_iops.c        | 13 +++++---
 fs/xfs/xfs_xattr.c       | 19 +++++++----
 6 files changed, 81 insertions(+), 74 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index f887d62e0956..eea6d90af276 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -330,22 +330,17 @@ xfs_attr_remove_args(
 }
 
 /*
- * Note: If value is NULL the attribute will be removed, just like the
+ * Note: If args->value is NULL the attribute will be removed, just like the
  * Linux ->setattr API.
  */
 int
 xfs_attr_set(
-	struct xfs_inode	*dp,
-	const unsigned char	*name,
-	size_t			namelen,
-	unsigned char		*value,
-	int			valuelen,
-	int			flags)
+	struct xfs_da_args	*args)
 {
+	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
-	struct xfs_da_args	args;
 	struct xfs_trans_res	tres;
-	int			rsvd = (flags & ATTR_ROOT) != 0;
+	int			rsvd = (args->flags & ATTR_ROOT) != 0;
 	int			error, local;
 	unsigned int		total;
 
@@ -356,25 +351,22 @@ xfs_attr_set(
 	if (error)
 		return error;
 
-	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
-	if (error)
-		return error;
-
-	args.value = value;
-	args.valuelen = valuelen;
+	args->geo = mp->m_attr_geo;
+	args->whichfork = XFS_ATTR_FORK;
+	args->hashval = xfs_da_hashname(args->name, args->namelen);
 
 	/*
 	 * We have no control over the attribute names that userspace passes us
 	 * to remove, so we have to allow the name lookup prior to attribute
 	 * removal to fail as well.
 	 */
-	args.op_flags = XFS_DA_OP_OKNOENT;
+	args->op_flags = XFS_DA_OP_OKNOENT;
 
-	if (value) {
+	if (args->value) {
 		XFS_STATS_INC(mp, xs_attr_set);
 
-		args.op_flags |= XFS_DA_OP_ADDNAME;
-		args.total = xfs_attr_calc_size(&args, &local);
+		args->op_flags |= XFS_DA_OP_ADDNAME;
+		args->total = xfs_attr_calc_size(args, &local);
 
 		/*
 		 * If the inode doesn't have an attribute fork, add one.
@@ -382,8 +374,8 @@ xfs_attr_set(
 		 */
 		if (XFS_IFORK_Q(dp) == 0) {
 			int sf_size = sizeof(struct xfs_attr_sf_hdr) +
-				XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen,
-						valuelen);
+				XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen,
+						args->valuelen);
 
 			error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
 			if (error)
@@ -391,10 +383,11 @@ xfs_attr_set(
 		}
 
 		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-				 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
+				 M_RES(mp)->tr_attrsetrt.tr_logres *
+					args->total;
 		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
-		total = args.total;
+		total = args->total;
 	} else {
 		XFS_STATS_INC(mp, xs_attr_remove);
 
@@ -407,29 +400,29 @@ xfs_attr_set(
 	 * operation if necessary
 	 */
 	error = xfs_trans_alloc(mp, &tres, total, 0,
-			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
+			rsvd ? XFS_TRANS_RESERVE : 0, &args->trans);
 	if (error)
 		return error;
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(args.trans, dp, 0);
-	if (value) {
+	xfs_trans_ijoin(args->trans, dp, 0);
+	if (args->value) {
 		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
 
 		if (rsvd)
 			quota_flags |= XFS_QMOPT_FORCE_RES;
-		error = xfs_trans_reserve_quota_nblks(args.trans, dp,
-				args.total, 0, quota_flags);
+		error = xfs_trans_reserve_quota_nblks(args->trans, dp,
+				args->total, 0, quota_flags);
 		if (error)
 			goto out_trans_cancel;
-		error = xfs_attr_set_args(&args);
+		error = xfs_attr_set_args(args);
 		if (error)
 			goto out_trans_cancel;
 		/* shortform attribute has already been committed */
-		if (!args.trans)
+		if (!args->trans)
 			goto out_unlock;
 	} else {
-		error = xfs_attr_remove_args(&args);
+		error = xfs_attr_remove_args(args);
 		if (error)
 			goto out_trans_cancel;
 	}
@@ -439,23 +432,23 @@ xfs_attr_set(
 	 * transaction goes to disk before returning to the user.
 	 */
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
-		xfs_trans_set_sync(args.trans);
+		xfs_trans_set_sync(args->trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
-		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
+	if ((args->flags & ATTR_KERNOTIME) == 0)
+		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
 
 	/*
 	 * Commit the last in the sequence of transactions.
 	 */
-	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
-	error = xfs_trans_commit(args.trans);
+	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
+	error = xfs_trans_commit(args->trans);
 out_unlock:
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 	return error;
 
 out_trans_cancel:
-	if (args.trans)
-		xfs_trans_cancel(args.trans);
+	if (args->trans)
+		xfs_trans_cancel(args->trans);
 	goto out_unlock;
 }
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index db58a6c7dea5..07ca543db831 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -149,8 +149,7 @@ 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,
 		 size_t namelen, unsigned char **value, int *valuelenp,
 		 int flags);
-int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
-		 size_t namelen, unsigned char *value, int valuelen, int flags);
+int xfs_attr_set(struct xfs_da_args *args);
 int xfs_attr_set_args(struct xfs_da_args *args);
 int xfs_attr_remove_args(struct xfs_da_args *args);
 int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 4e76063ff956..e9ae7cbe1973 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -166,41 +166,42 @@ xfs_get_acl(struct inode *inode, int type)
 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_acl *xfs_acl = NULL;
-	int len = 0;
-	int error;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_da_args	args = {
+		.dp		= ip,
+		.flags		= ATTR_ROOT,
+	};
+	int			error;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
-		ea_name = SGI_ACL_FILE;
+		args.name = SGI_ACL_FILE;
 		break;
 	case ACL_TYPE_DEFAULT:
 		if (!S_ISDIR(inode->i_mode))
 			return acl ? -EACCES : 0;
-		ea_name = SGI_ACL_DEFAULT;
+		args.name = SGI_ACL_DEFAULT;
 		break;
 	default:
 		return -EINVAL;
 	}
+	args.namelen = strlen(args.name);
 
 	if (acl) {
-		len = XFS_ACL_MAX_SIZE(ip->i_mount);
-		xfs_acl = kmem_zalloc_large(len, 0);
-		if (!xfs_acl)
+		args.valuelen = XFS_ACL_MAX_SIZE(ip->i_mount);
+		args.value = kmem_zalloc_large(args.valuelen, 0);
+		if (!args.value)
 			return -ENOMEM;
 
-		xfs_acl_to_disk(xfs_acl, acl);
+		xfs_acl_to_disk(args.value, acl);
 
 		/* subtract away the unused acl entries */
-		len -= sizeof(struct xfs_acl_entry) *
+		args.valuelen -= 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);
-	kmem_free(xfs_acl);
+	error = xfs_attr_set(&args);
+	kmem_free(args.value);
 
 	/*
 	 * 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 cfdd80b4ea2d..47a88b5cfa63 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -389,9 +389,13 @@ xfs_attrmulti_attr_set(
 	uint32_t		len,
 	uint32_t		flags)
 {
-	unsigned char		*kbuf = NULL;
+	struct xfs_da_args	args = {
+		.dp		= XFS_I(inode),
+		.flags		= flags,
+		.name		= name,
+		.namelen	= strlen(name),
+	};
 	int			error;
-	size_t			namelen;
 
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return -EPERM;
@@ -399,16 +403,16 @@ xfs_attrmulti_attr_set(
 	if (ubuf) {
 		if (len > XFS_XATTR_SIZE_MAX)
 			return -EINVAL;
-		kbuf = memdup_user(ubuf, len);
-		if (IS_ERR(kbuf))
-			return PTR_ERR(kbuf);
+		args.value = memdup_user(ubuf, len);
+		if (IS_ERR(args.value))
+			return PTR_ERR(args.value);
+		args.valuelen = len;
 	}
 
-	namelen = strlen(name);
-	error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
+	error = xfs_attr_set(&args);
 	if (!error)
 		xfs_forget_acl(inode, name, flags);
-	kfree(kbuf);
+	kfree(args.value);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..94cd4254656c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -50,10 +50,15 @@ xfs_initxattrs(
 	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);
+		struct xfs_da_args	args = {
+			.dp		= ip,
+			.flags		= ATTR_SECURE,
+			.name		= xattr->name,
+			.namelen	= strlen(xattr->name),
+			.value		= xattr->value,
+			.valuelen	= xattr->value_len,
+		};
+		error = xfs_attr_set(&args);
 		if (error < 0)
 			break;
 	}
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 1670bfbc9ad2..09f967f97699 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -66,20 +66,25 @@ 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);
+	struct xfs_da_args	args = {
+		.dp		= XFS_I(inode),
+		.flags		= handler->flags,
+		.name		= name,
+		.namelen	= strlen(name),
+		.value		= (unsigned char *)value,
+		.valuelen	= size,
+	};
 	int			error;
 
 	/* Convert Linux syscall to XFS internal ATTR flags */
 	if (flags & XATTR_CREATE)
-		xflags |= ATTR_CREATE;
+		args.flags |= ATTR_CREATE;
 	if (flags & XATTR_REPLACE)
-		xflags |= ATTR_REPLACE;
+		args.flags |= ATTR_REPLACE;
 
-	error = xfs_attr_set(ip, (unsigned char *)name, strlen(name),
-				(void *)value, size, xflags);
+	error = xfs_attr_set(&args);
 	if (!error)
-		xfs_forget_acl(inode, name, xflags);
+		xfs_forget_acl(inode, name, args.flags);
 	return error;
 }
 
-- 
2.24.1

Re: [PATCH 11/30] xfs: pass an initialized xfs_da_args structure to xfs_attr_set

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 
> Instead of converting from one style of arguments to another in
> xfs_attr_set, pass the structure from higher up in the call chain.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 69 ++++++++++++++++++----------------------
>  fs/xfs/libxfs/xfs_attr.h |  3 +-
>  fs/xfs/xfs_acl.c         | 31 +++++++++---------
>  fs/xfs/xfs_ioctl.c       | 20 +++++++-----
>  fs/xfs/xfs_iops.c        | 13 +++++---
>  fs/xfs/xfs_xattr.c       | 19 +++++++----
>  6 files changed, 81 insertions(+), 74 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index f887d62e0956..eea6d90af276 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -330,22 +330,17 @@ xfs_attr_remove_args(
>  }
> 
>  /*
> - * Note: If value is NULL the attribute will be removed, just like the
> + * Note: If args->value is NULL the attribute will be removed, just like the
>   * Linux ->setattr API.
>   */
>  int
>  xfs_attr_set(
> -	struct xfs_inode	*dp,
> -	const unsigned char	*name,
> -	size_t			namelen,
> -	unsigned char		*value,
> -	int			valuelen,
> -	int			flags)
> +	struct xfs_da_args	*args)
>  {
> +	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
> -	struct xfs_da_args	args;
>  	struct xfs_trans_res	tres;
> -	int			rsvd = (flags & ATTR_ROOT) != 0;
> +	int			rsvd = (args->flags & ATTR_ROOT) != 0;
>  	int			error, local;
>  	unsigned int		total;
> 
> @@ -356,25 +351,22 @@ xfs_attr_set(
>  	if (error)
>  		return error;
> 
> -	error = xfs_attr_args_init(&args, dp, name, namelen, flags);
> -	if (error)
> -		return error;
> -
> -	args.value = value;
> -	args.valuelen = valuelen;
> +	args->geo = mp->m_attr_geo;
> +	args->whichfork = XFS_ATTR_FORK;
> +	args->hashval = xfs_da_hashname(args->name, args->namelen);
> 
>  	/*
>  	 * We have no control over the attribute names that userspace passes us
>  	 * to remove, so we have to allow the name lookup prior to attribute
>  	 * removal to fail as well.
>  	 */
> -	args.op_flags = XFS_DA_OP_OKNOENT;
> +	args->op_flags = XFS_DA_OP_OKNOENT;
> 
> -	if (value) {
> +	if (args->value) {
>  		XFS_STATS_INC(mp, xs_attr_set);
> 
> -		args.op_flags |= XFS_DA_OP_ADDNAME;
> -		args.total = xfs_attr_calc_size(&args, &local);
> +		args->op_flags |= XFS_DA_OP_ADDNAME;
> +		args->total = xfs_attr_calc_size(args, &local);
> 
>  		/*
>  		 * If the inode doesn't have an attribute fork, add one.
> @@ -382,8 +374,8 @@ xfs_attr_set(
>  		 */
>  		if (XFS_IFORK_Q(dp) == 0) {
>  			int sf_size = sizeof(struct xfs_attr_sf_hdr) +
> -				XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen,
> -						valuelen);
> +				XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen,
> +						args->valuelen);
> 
>  			error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
>  			if (error)
> @@ -391,10 +383,11 @@ xfs_attr_set(
>  		}
> 
>  		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> -				 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> +				 M_RES(mp)->tr_attrsetrt.tr_logres *
> +					args->total;
>  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> -		total = args.total;
> +		total = args->total;
>  	} else {
>  		XFS_STATS_INC(mp, xs_attr_remove);
> 
> @@ -407,29 +400,29 @@ xfs_attr_set(
>  	 * operation if necessary
>  	 */
>  	error = xfs_trans_alloc(mp, &tres, total, 0,
> -			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
> +			rsvd ? XFS_TRANS_RESERVE : 0, &args->trans);
>  	if (error)
>  		return error;
> 
>  	xfs_ilock(dp, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(args.trans, dp, 0);
> -	if (value) {
> +	xfs_trans_ijoin(args->trans, dp, 0);
> +	if (args->value) {
>  		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
> 
>  		if (rsvd)
>  			quota_flags |= XFS_QMOPT_FORCE_RES;
> -		error = xfs_trans_reserve_quota_nblks(args.trans, dp,
> -				args.total, 0, quota_flags);
> +		error = xfs_trans_reserve_quota_nblks(args->trans, dp,
> +				args->total, 0, quota_flags);
>  		if (error)
>  			goto out_trans_cancel;
> -		error = xfs_attr_set_args(&args);
> +		error = xfs_attr_set_args(args);
>  		if (error)
>  			goto out_trans_cancel;
>  		/* shortform attribute has already been committed */
> -		if (!args.trans)
> +		if (!args->trans)
>  			goto out_unlock;
>  	} else {
> -		error = xfs_attr_remove_args(&args);
> +		error = xfs_attr_remove_args(args);
>  		if (error)
>  			goto out_trans_cancel;
>  	}
> @@ -439,23 +432,23 @@ xfs_attr_set(
>  	 * transaction goes to disk before returning to the user.
>  	 */
>  	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args.trans);
> +		xfs_trans_set_sync(args->trans);
> 
> -	if ((flags & ATTR_KERNOTIME) == 0)
> -		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
> +	if ((args->flags & ATTR_KERNOTIME) == 0)
> +		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> 
>  	/*
>  	 * Commit the last in the sequence of transactions.
>  	 */
> -	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
> -	error = xfs_trans_commit(args.trans);
> +	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> +	error = xfs_trans_commit(args->trans);
>  out_unlock:
>  	xfs_iunlock(dp, XFS_ILOCK_EXCL);
>  	return error;
> 
>  out_trans_cancel:
> -	if (args.trans)
> -		xfs_trans_cancel(args.trans);
> +	if (args->trans)
> +		xfs_trans_cancel(args->trans);
>  	goto out_unlock;
>  }
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index db58a6c7dea5..07ca543db831 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -149,8 +149,7 @@ 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,
>  		 size_t namelen, unsigned char **value, int *valuelenp,
>  		 int flags);
> -int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> -		 size_t namelen, unsigned char *value, int valuelen, int flags);
> +int xfs_attr_set(struct xfs_da_args *args);
>  int xfs_attr_set_args(struct xfs_da_args *args);
>  int xfs_attr_remove_args(struct xfs_da_args *args);
>  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 4e76063ff956..e9ae7cbe1973 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -166,41 +166,42 @@ xfs_get_acl(struct inode *inode, int type)
>  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_acl *xfs_acl = NULL;
> -	int len = 0;
> -	int error;
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_da_args	args = {
> +		.dp		= ip,
> +		.flags		= ATTR_ROOT,
> +	};
> +	int			error;
> 
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
> -		ea_name = SGI_ACL_FILE;
> +		args.name = SGI_ACL_FILE;
>  		break;
>  	case ACL_TYPE_DEFAULT:
>  		if (!S_ISDIR(inode->i_mode))
>  			return acl ? -EACCES : 0;
> -		ea_name = SGI_ACL_DEFAULT;
> +		args.name = SGI_ACL_DEFAULT;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
> +	args.namelen = strlen(args.name);
> 
>  	if (acl) {
> -		len = XFS_ACL_MAX_SIZE(ip->i_mount);
> -		xfs_acl = kmem_zalloc_large(len, 0);
> -		if (!xfs_acl)
> +		args.valuelen = XFS_ACL_MAX_SIZE(ip->i_mount);
> +		args.value = kmem_zalloc_large(args.valuelen, 0);
> +		if (!args.value)
>  			return -ENOMEM;
> 
> -		xfs_acl_to_disk(xfs_acl, acl);
> +		xfs_acl_to_disk(args.value, acl);
> 
>  		/* subtract away the unused acl entries */
> -		len -= sizeof(struct xfs_acl_entry) *
> +		args.valuelen -= 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);
> -	kmem_free(xfs_acl);
> +	error = xfs_attr_set(&args);
> +	kmem_free(args.value);
> 
>  	/*
>  	 * 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 cfdd80b4ea2d..47a88b5cfa63 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -389,9 +389,13 @@ xfs_attrmulti_attr_set(
>  	uint32_t		len,
>  	uint32_t		flags)
>  {
> -	unsigned char		*kbuf = NULL;
> +	struct xfs_da_args	args = {
> +		.dp		= XFS_I(inode),
> +		.flags		= flags,
> +		.name		= name,
> +		.namelen	= strlen(name),
> +	};
>  	int			error;
> -	size_t			namelen;
> 
>  	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
>  		return -EPERM;
> @@ -399,16 +403,16 @@ xfs_attrmulti_attr_set(
>  	if (ubuf) {
>  		if (len > XFS_XATTR_SIZE_MAX)
>  			return -EINVAL;
> -		kbuf = memdup_user(ubuf, len);
> -		if (IS_ERR(kbuf))
> -			return PTR_ERR(kbuf);
> +		args.value = memdup_user(ubuf, len);
> +		if (IS_ERR(args.value))
> +			return PTR_ERR(args.value);
> +		args.valuelen = len;
>  	}
> 
> -	namelen = strlen(name);
> -	error = xfs_attr_set(XFS_I(inode), name, namelen, kbuf, len, flags);
> +	error = xfs_attr_set(&args);
>  	if (!error)
>  		xfs_forget_acl(inode, name, flags);
> -	kfree(kbuf);
> +	kfree(args.value);
>  	return error;
>  }
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..94cd4254656c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -50,10 +50,15 @@ xfs_initxattrs(
>  	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);
> +		struct xfs_da_args	args = {
> +			.dp		= ip,
> +			.flags		= ATTR_SECURE,
> +			.name		= xattr->name,
> +			.namelen	= strlen(xattr->name),
> +			.value		= xattr->value,
> +			.valuelen	= xattr->value_len,
> +		};
> +		error = xfs_attr_set(&args);
>  		if (error < 0)
>  			break;
>  	}
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 1670bfbc9ad2..09f967f97699 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -66,20 +66,25 @@ 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);
> +	struct xfs_da_args	args = {
> +		.dp		= XFS_I(inode),
> +		.flags		= handler->flags,
> +		.name		= name,
> +		.namelen	= strlen(name),
> +		.value		= (unsigned char *)value,

Since xfs_da_args.value is of type "void *', Wouldn't it be more uniform if
'value' is typecasted with (void *)? 

Apart from the above very trival nit, the changes look good to me,

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> +		.valuelen	= size,
> +	};
>  	int			error;
> 
>  	/* Convert Linux syscall to XFS internal ATTR flags */
>  	if (flags & XATTR_CREATE)
> -		xflags |= ATTR_CREATE;
> +		args.flags |= ATTR_CREATE;
>  	if (flags & XATTR_REPLACE)
> -		xflags |= ATTR_REPLACE;
> +		args.flags |= ATTR_REPLACE;
> 
> -	error = xfs_attr_set(ip, (unsigned char *)name, strlen(name),
> -				(void *)value, size, xflags);
> +	error = xfs_attr_set(&args);
>  	if (!error)
> -		xfs_forget_acl(inode, name, xflags);
> +		xfs_forget_acl(inode, name, args.flags);
>  	return error;
>  }
> 
> 


-- 
chandan

Re: [PATCH 11/30] xfs: pass an initialized xfs_da_args structure to xfs_attr_set

[Christoph Hellwig]

On Fri, Feb 07, 2020 at 03:12:06PM +0530, Chandan Rajendra wrote:
> >  		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);
> > +	struct xfs_da_args	args = {
> > +		.dp		= XFS_I(inode),
> > +		.flags		= handler->flags,
> > +		.name		= name,
> > +		.namelen	= strlen(name),
> > +		.value		= (unsigned char *)value,
> 
> Since xfs_da_args.value is of type "void *', Wouldn't it be more uniform if
> 'value' is typecasted with (void *)? 

Yes, fixed.

[PATCH 12/30] xfs: pass an initialized xfs_da_args to xfs_attr_get

[Christoph Hellwig]

Instead of converting from one style of arguments to another in
xfs_attr_set, pass the structure from higher up in the call chain.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 80 ++++++++++++----------------------------
 fs/xfs/libxfs/xfs_attr.h |  4 +-
 fs/xfs/xfs_acl.c         | 35 ++++++++----------
 fs/xfs/xfs_ioctl.c       | 25 ++++++++-----
 fs/xfs/xfs_xattr.c       | 24 ++++++------
 5 files changed, 68 insertions(+), 100 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index eea6d90af276..288b39e81efd 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -56,26 +56,6 @@ STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
 STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
 STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
 
-
-STATIC int
-xfs_attr_args_init(
-	struct xfs_da_args	*args,
-	struct xfs_inode	*dp,
-	const unsigned char	*name,
-	size_t			namelen,
-	int			flags)
-{
-	memset(args, 0, sizeof(*args));
-	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->hashval = xfs_da_hashname(args->name, args->namelen);
-	return 0;
-}
-
 int
 xfs_inode_hasattr(
 	struct xfs_inode	*ip)
@@ -115,15 +95,15 @@ xfs_attr_get_ilocked(
 /*
  * Retrieve an extended attribute by name, and its value if requested.
  *
- * If ATTR_KERNOVAL is set in @flags, then the caller does not want the value,
- * just an indication whether the attribute exists and the size of the value if
- * it exists. The size is returned in @valuelenp,
+ * If ATTR_KERNOVAL is set in args->flags, then the caller does not want the
+ * value, just an indication whether the attribute exists and the size of the
+ * value if it exists. The size is returned in args.valuelen.
  *
  * If the attribute is found, but exceeds the size limit set by the caller in
- * @valuelenp, return -ERANGE with the size of the attribute that was found in
- * @valuelenp.
+ * args->valuelen, return -ERANGE with the size of the attribute that was found
+ * in args->valuelen.
  *
- * If ATTR_ALLOC is set in @flags, allocate the buffer for the value after
+ * If ATTR_ALLOC is set in args->flags, allocate the buffer for the value after
  * existence of the attribute has been determined. On success, return that
  * buffer to the caller and leave them to free it. On failure, free any
  * allocated buffer and ensure the buffer pointer returned to the caller is
@@ -131,51 +111,37 @@ xfs_attr_get_ilocked(
  */
 int
 xfs_attr_get(
-	struct xfs_inode	*ip,
-	const unsigned char	*name,
-	size_t			namelen,
-	unsigned char		**value,
-	int			*valuelenp,
-	int			flags)
+	struct xfs_da_args	*args)
 {
-	struct xfs_da_args	args;
 	uint			lock_mode;
 	int			error;
 
-	ASSERT((flags & (ATTR_ALLOC | ATTR_KERNOVAL)) || *value);
+	ASSERT((args->flags & (ATTR_ALLOC | ATTR_KERNOVAL)) || args->value);
 
-	XFS_STATS_INC(ip->i_mount, xs_attr_get);
+	XFS_STATS_INC(args->dp->i_mount, xs_attr_get);
 
-	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
+	if (XFS_FORCED_SHUTDOWN(args->dp->i_mount))
 		return -EIO;
 
-	error = xfs_attr_args_init(&args, ip, name, namelen, flags);
-	if (error)
-		return error;
+	args->geo = args->dp->i_mount->m_attr_geo;
+	args->whichfork = XFS_ATTR_FORK;
+	args->hashval = xfs_da_hashname(args->name, args->namelen);
 
 	/* Entirely possible to look up a name which doesn't exist */
-	args.op_flags = XFS_DA_OP_OKNOENT;
-	if (flags & ATTR_ALLOC)
-		args.op_flags |= XFS_DA_OP_ALLOCVAL;
-	else
-		args.value = *value;
-	args.valuelen = *valuelenp;
+	args->op_flags = XFS_DA_OP_OKNOENT;
+	if (args->flags & ATTR_ALLOC)
+		args->op_flags |= XFS_DA_OP_ALLOCVAL;
 
-	lock_mode = xfs_ilock_attr_map_shared(ip);
-	error = xfs_attr_get_ilocked(ip, &args);
-	xfs_iunlock(ip, lock_mode);
-	*valuelenp = args.valuelen;
+	lock_mode = xfs_ilock_attr_map_shared(args->dp);
+	error = xfs_attr_get_ilocked(args->dp, args);
+	xfs_iunlock(args->dp, lock_mode);
 
 	/* on error, we have to clean up allocated value buffers */
-	if (error) {
-		if (flags & ATTR_ALLOC) {
-			kmem_free(args.value);
-			*value = NULL;
-		}
-		return error;
+	if (error && (args->flags & ATTR_ALLOC)) {
+		kmem_free(args->value);
+		args->value = NULL;
 	}
-	*value = args.value;
-	return 0;
+	return error;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 07ca543db831..be77d13a2902 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -146,9 +146,7 @@ int xfs_attr_list_int_ilocked(struct xfs_attr_list_context *);
 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,
-		 size_t namelen, unsigned char **value, int *valuelenp,
-		 int flags);
+int xfs_attr_get(struct xfs_da_args *args);
 int xfs_attr_set(struct xfs_da_args *args);
 int xfs_attr_set_args(struct xfs_da_args *args);
 int xfs_attr_remove_args(struct xfs_da_args *args);
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index e9ae7cbe1973..780924984492 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -120,34 +120,31 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
 struct posix_acl *
 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;
-	int error;
-	int len;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct posix_acl	*acl = NULL;
+	struct xfs_da_args	args = {
+		.dp		= ip,
+		.flags		= ATTR_ALLOC | ATTR_ROOT,
+		.valuelen	= XFS_ACL_MAX_SIZE(mp),
+	};
+	int			error;
 
 	trace_xfs_get_acl(ip);
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
-		ea_name = SGI_ACL_FILE;
+		args.name = SGI_ACL_FILE;
 		break;
 	case ACL_TYPE_DEFAULT:
-		ea_name = SGI_ACL_DEFAULT;
+		args.name = SGI_ACL_DEFAULT;
 		break;
 	default:
 		BUG();
 	}
+	args.namelen = strlen(args.name);
 
-	/*
-	 * If we have a cached ACLs value just return it, not need to
-	 * 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);
+	error = xfs_attr_get(&args);
 	if (error) {
 		/*
 		 * If the attribute doesn't exist make sure we have a negative
@@ -156,9 +153,9 @@ xfs_get_acl(struct inode *inode, int type)
 		if (error != -ENOATTR)
 			acl = ERR_PTR(error);
 	} else  {
-		acl = xfs_acl_from_disk(ip->i_mount, xfs_acl, len,
-					XFS_ACL_MAX_ENTRIES(ip->i_mount));
-		kmem_free(xfs_acl);
+		acl = xfs_acl_from_disk(mp, args.value, args.valuelen,
+					XFS_ACL_MAX_ENTRIES(mp));
+		kmem_free(args.value);
 	}
 	return acl;
 }
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 47a88b5cfa63..2da22595f828 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -357,27 +357,32 @@ xfs_attrmulti_attr_get(
 	uint32_t		*len,
 	uint32_t		flags)
 {
-	unsigned char		*kbuf;
-	int			error = -EFAULT;
-	size_t			namelen;
+	struct xfs_da_args	args = {
+		.dp		= XFS_I(inode),
+		.flags		= flags,
+		.name		= name,
+		.namelen	= strlen(name),
+		.valuelen	= *len,
+	};
+	int			error;
 
 	if (*len > XFS_XATTR_SIZE_MAX)
 		return -EINVAL;
-	kbuf = kmem_zalloc_large(*len, 0);
-	if (!kbuf)
+
+	args.value = kmem_zalloc_large(*len, 0);
+	if (!args.value)
 		return -ENOMEM;
 
-	namelen = strlen(name);
-	error = xfs_attr_get(XFS_I(inode), name, namelen, &kbuf, (int *)len,
-			     flags);
+	error = xfs_attr_get(&args);
 	if (error)
 		goto out_kfree;
 
-	if (copy_to_user(ubuf, kbuf, *len))
+	*len = args.valuelen;
+	if (copy_to_user(ubuf, args.value, args.valuelen))
 		error = -EFAULT;
 
 out_kfree:
-	kmem_free(kbuf);
+	kmem_free(args.value);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 09f967f97699..b3ce5e8777f9 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -21,22 +21,24 @@ 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_da_args	args = {
+		.dp		= XFS_I(inode),
+		.flags		= handler->flags,
+		.name		= name,
+		.namelen	= strlen(name),
+		.value		= value,
+		.valuelen	= size,
+	};
+	int			error;
 
 	/* Convert Linux syscall to XFS internal ATTR flags */
-	if (!size) {
-		xflags |= ATTR_KERNOVAL;
-		value = NULL;
-	}
+	if (!size)
+		args.flags |= ATTR_KERNOVAL;
 
-	error = xfs_attr_get(ip, name, namelen, (unsigned char **)&value,
-			     &asize, xflags);
+	error = xfs_attr_get(&args);
 	if (error)
 		return error;
-	return asize;
+	return args.valuelen;
 }
 
 void
-- 
2.24.1

Re: [PATCH 12/30] xfs: pass an initialized xfs_da_args to xfs_attr_get

[Chandan Rajendra]

On Wednesday, January 29, 2020 10:32 PM Christoph Hellwig wrote: 
> Instead of converting from one style of arguments to another in
> xfs_attr_set, pass the structure from higher up in the call chain.
>

The newly introduced changes logically match with the code flow that existed
earlier.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 80 ++++++++++++----------------------------
>  fs/xfs/libxfs/xfs_attr.h |  4 +-
>  fs/xfs/xfs_acl.c         | 35 ++++++++----------
>  fs/xfs/xfs_ioctl.c       | 25 ++++++++-----
>  fs/xfs/xfs_xattr.c       | 24 ++++++------
>  5 files changed, 68 insertions(+), 100 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index eea6d90af276..288b39e81efd 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -56,26 +56,6 @@ STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>  STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>  STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
> 
> -
> -STATIC int
> -xfs_attr_args_init(
> -	struct xfs_da_args	*args,
> -	struct xfs_inode	*dp,
> -	const unsigned char	*name,
> -	size_t			namelen,
> -	int			flags)
> -{
> -	memset(args, 0, sizeof(*args));
> -	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->hashval = xfs_da_hashname(args->name, args->namelen);
> -	return 0;
> -}
> -
>  int
>  xfs_inode_hasattr(
>  	struct xfs_inode	*