All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] xfs: rework the attr ->put_listent code a bit
@ 2016-03-11 22:10 Eric Sandeen
  2016-03-11 22:11 ` [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent Eric Sandeen
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Eric Sandeen @ 2016-03-11 22:10 UTC (permalink / raw)
  To: xfs

Lots of cruft in there, maybe more to do but here's the
obvious stuff; removing bad error returns, unused variables
and structure members, and then consolidating a little code.

 xfs_attr.h      |    4 +-
 xfs_attr_list.c |   86 ++++++++++++++++----------------------------------------
 xfs_xattr.c     |   17 +++++++----
 3 files changed, 39 insertions(+), 68 deletions(-)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent
  2016-03-11 22:10 [PATCH 0/4] xfs: rework the attr ->put_listent code a bit Eric Sandeen
@ 2016-03-11 22:11 ` Eric Sandeen
  2016-03-15  7:55   ` Christoph Hellwig
  2016-03-11 22:11 ` [PATCH 2/4] xfs: don't pass value into " Eric Sandeen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2016-03-11 22:11 UTC (permalink / raw)
  To: xfs

Today, the put_listent formatters return either 1 or 0; if
they return 1, some callers treat this as an error and return
it up the stack, despite "1" not being a valid (negative)
error code.

The intent seems to be that if the input buffer is full,
we set seen_enough or set count = -1, and return 1;
but some callers check the return before checking the
seen_enough or count fields of the context.

Fix this by only returning non-zero for actual errors
encountered, and rely on the caller to first check the
return value, then check the values in the context to
decide what to do.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_attr.h      |    1 +
 fs/xfs/xfs_attr_list.c |    8 +++-----
 fs/xfs/xfs_xattr.c     |   14 ++++++++++----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index dd48245..2343312 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -112,6 +112,7 @@ typedef struct attrlist_cursor_kern {
  *========================================================================*/
 
 
+/* Return 0 on success, or -errno; other state communicated via *context */
 typedef int (*put_listent_func_t)(struct xfs_attr_list_context *, int,
 			      unsigned char *, int, int, unsigned char *);
 
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 0ef7c2e..d5ab59f 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -108,16 +108,14 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 					   (int)sfe->namelen,
 					   (int)sfe->valuelen,
 					   &sfe->nameval[sfe->namelen]);
-
+			if (error)
+				return error;
 			/*
 			 * Either search callback finished early or
 			 * didn't fit it all in the buffer after all.
 			 */
 			if (context->seen_enough)
 				break;
-
-			if (error)
-				return error;
 			sfe = XFS_ATTR_SF_NEXTENTRY(sfe);
 		}
 		trace_xfs_attr_list_sf_all(context);
@@ -580,7 +578,7 @@ xfs_attr_put_listent(
 		trace_xfs_attr_list_full(context);
 		alist->al_more = 1;
 		context->seen_enough = 1;
-		return 1;
+		return 0;
 	}
 
 	aep = (attrlist_ent_t *)&context->alist[context->firstu];
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 110f1d7..f220129 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -146,7 +146,7 @@ __xfs_xattr_put_listent(
 	arraytop = context->count + prefix_len + namelen + 1;
 	if (arraytop > context->firstu) {
 		context->count = -1;	/* insufficient space */
-		return 1;
+		return 0;
 	}
 	offset = (char *)context->alist + context->count;
 	strncpy(offset, prefix, prefix_len);
@@ -221,11 +221,15 @@ xfs_xattr_put_listent(
 }
 
 ssize_t
-xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
+xfs_vn_listxattr(
+	struct dentry	*dentry,
+	char		*data,
+	size_t		size)
 {
 	struct xfs_attr_list_context context;
 	struct attrlist_cursor_kern cursor = { 0 };
-	struct inode		*inode = d_inode(dentry);
+	struct inode	*inode = d_inode(dentry);
+	int		error;
 
 	/*
 	 * First read the regular on-disk attributes.
@@ -239,7 +243,9 @@ xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
 	context.firstu = context.bufsize;
 	context.put_listent = xfs_xattr_put_listent;
 
-	xfs_attr_list_int(&context);
+	error = xfs_attr_list_int(&context);
+	if (error)
+		return error;
 	if (context.count < 0)
 		return -ERANGE;
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/4] xfs: don't pass value into attr ->put_listent
  2016-03-11 22:10 [PATCH 0/4] xfs: rework the attr ->put_listent code a bit Eric Sandeen
  2016-03-11 22:11 ` [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent Eric Sandeen
@ 2016-03-11 22:11 ` Eric Sandeen
  2016-03-15  7:55   ` Christoph Hellwig
  2016-03-11 22:12 ` [PATCH 3/4] xfs: remove put_value from attr ->put_listent context Eric Sandeen
  2016-03-11 22:12 ` [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int Eric Sandeen
  3 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2016-03-11 22:11 UTC (permalink / raw)
  To: xfs

The value is not used; only names and value lengths are
returned.  Remove the argument.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_attr.h      |    2 +-
 fs/xfs/xfs_attr_list.c |   18 ++++++------------
 fs/xfs/xfs_xattr.c     |    3 +--
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index 2343312..dab4f41 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -114,7 +114,7 @@ typedef struct attrlist_cursor_kern {
 
 /* Return 0 on success, or -errno; other state communicated via *context */
 typedef int (*put_listent_func_t)(struct xfs_attr_list_context *, int,
-			      unsigned char *, int, int, unsigned char *);
+			      unsigned char *, int, int);
 
 typedef struct xfs_attr_list_context {
 	struct xfs_inode		*dp;		/* inode */
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index d5ab59f..1d36d78 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -106,8 +106,7 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 					   sfe->flags,
 					   sfe->nameval,
 					   (int)sfe->namelen,
-					   (int)sfe->valuelen,
-					   &sfe->nameval[sfe->namelen]);
+					   (int)sfe->valuelen);
 			if (error)
 				return error;
 			/*
@@ -198,8 +197,7 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 					sbp->flags,
 					sbp->name,
 					sbp->namelen,
-					sbp->valuelen,
-					&sbp->name[sbp->namelen]);
+					sbp->valuelen);
 		if (error)
 			return error;
 		if (context->seen_enough)
@@ -428,8 +426,7 @@ xfs_attr3_leaf_list_int(
 						entry->flags,
 						name_loc->nameval,
 						(int)name_loc->namelen,
-						be16_to_cpu(name_loc->valuelen),
-						&name_loc->nameval[name_loc->namelen]);
+						be16_to_cpu(name_loc->valuelen));
 			if (retval)
 				return retval;
 		} else {
@@ -458,16 +455,14 @@ xfs_attr3_leaf_list_int(
 						entry->flags,
 						name_rmt->name,
 						(int)name_rmt->namelen,
-						valuelen,
-						args.value);
+						valuelen);
 				kmem_free(args.value);
 			} else {
 				retval = context->put_listent(context,
 						entry->flags,
 						name_rmt->name,
 						(int)name_rmt->namelen,
-						valuelen,
-						NULL);
+						valuelen);
 			}
 			if (retval)
 				return retval;
@@ -548,8 +543,7 @@ xfs_attr_put_listent(
 	int		flags,
 	unsigned char	*name,
 	int		namelen,
-	int		valuelen,
-	unsigned char	*value)
+	int		valuelen)
 {
 	struct attrlist *alist = (struct attrlist *)context->alist;
 	attrlist_ent_t *aep;
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index f220129..7fdcf33 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -166,8 +166,7 @@ xfs_xattr_put_listent(
 	int		flags,
 	unsigned char	*name,
 	int		namelen,
-	int		valuelen,
-	unsigned char	*value)
+	int		valuelen)
 {
 	char *prefix;
 	int prefix_len;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 3/4] xfs: remove put_value from attr ->put_listent context
  2016-03-11 22:10 [PATCH 0/4] xfs: rework the attr ->put_listent code a bit Eric Sandeen
  2016-03-11 22:11 ` [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent Eric Sandeen
  2016-03-11 22:11 ` [PATCH 2/4] xfs: don't pass value into " Eric Sandeen
@ 2016-03-11 22:12 ` Eric Sandeen
  2016-03-15  7:56   ` Christoph Hellwig
  2016-03-11 22:12 ` [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int Eric Sandeen
  3 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2016-03-11 22:12 UTC (permalink / raw)
  To: xfs

The put_value context member is never set; remove it
and the conditional test in xfs_attr3_leaf_list_int().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_attr.h      |    1 -
 fs/xfs/xfs_attr_list.c |   26 +-------------------------
 2 files changed, 1 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index dab4f41..e3da5d4 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -127,7 +127,6 @@ typedef struct xfs_attr_list_context {
 	int				firstu;		/* first used byte in buffer */
 	int				flags;		/* from VOP call */
 	int				resynch;	/* T/F: resynch with cursor */
-	int				put_value;	/* T/F: need value for listent */
 	put_listent_func_t		put_listent;	/* list output fmt function */
 	int				index;		/* index into output buffer */
 } xfs_attr_list_context_t;
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 1d36d78..900164c 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -435,35 +435,11 @@ xfs_attr3_leaf_list_int(
 
 			int valuelen = be32_to_cpu(name_rmt->valuelen);
 
-			if (context->put_value) {
-				xfs_da_args_t args;
-
-				memset((char *)&args, 0, sizeof(args));
-				args.geo = context->dp->i_mount->m_attr_geo;
-				args.dp = context->dp;
-				args.whichfork = XFS_ATTR_FORK;
-				args.valuelen = valuelen;
-				args.rmtvaluelen = valuelen;
-				args.value = kmem_alloc(valuelen, KM_SLEEP | KM_NOFS);
-				args.rmtblkno = be32_to_cpu(name_rmt->valueblk);
-				args.rmtblkcnt = xfs_attr3_rmt_blocks(
-							args.dp->i_mount, valuelen);
-				retval = xfs_attr_rmtval_get(&args);
-				if (retval)
-					return retval;
-				retval = context->put_listent(context,
-						entry->flags,
-						name_rmt->name,
-						(int)name_rmt->namelen,
-						valuelen);
-				kmem_free(args.value);
-			} else {
-				retval = context->put_listent(context,
+			retval = context->put_listent(context,
 						entry->flags,
 						name_rmt->name,
 						(int)name_rmt->namelen,
 						valuelen);
-			}
 			if (retval)
 				return retval;
 		}
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int
  2016-03-11 22:10 [PATCH 0/4] xfs: rework the attr ->put_listent code a bit Eric Sandeen
                   ` (2 preceding siblings ...)
  2016-03-11 22:12 ` [PATCH 3/4] xfs: remove put_value from attr ->put_listent context Eric Sandeen
@ 2016-03-11 22:12 ` Eric Sandeen
  2016-03-15  7:57   ` Christoph Hellwig
  3 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2016-03-11 22:12 UTC (permalink / raw)
  To: xfs

Consolidate the 2 calls to ->put_listent in
xfs_attr3_leaf_list_int(), by setting up name, namelen, and
valuelen for the local vs remote cases, then call ->put_listent
and do the error handling all in one spot.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_attr_list.c |   42 ++++++++++++++++++++----------------------
 1 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 900164c..007be5c 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -410,6 +410,9 @@ xfs_attr3_leaf_list_int(
 	 */
 	retval = 0;
 	for (; i < ichdr.count; entry++, i++) {
+		char *name;
+		int namelen, valuelen;
+
 		if (be32_to_cpu(entry->hashval) != cursor->hashval) {
 			cursor->hashval = be32_to_cpu(entry->hashval);
 			cursor->offset = 0;
@@ -419,30 +422,25 @@ xfs_attr3_leaf_list_int(
 			continue;		/* skip incomplete entries */
 
 		if (entry->flags & XFS_ATTR_LOCAL) {
-			xfs_attr_leaf_name_local_t *name_loc =
-				xfs_attr3_leaf_name_local(leaf, i);
-
-			retval = context->put_listent(context,
-						entry->flags,
-						name_loc->nameval,
-						(int)name_loc->namelen,
-						be16_to_cpu(name_loc->valuelen));
-			if (retval)
-				return retval;
+			xfs_attr_leaf_name_local_t *name_loc;
+
+			name_loc = xfs_attr3_leaf_name_local(leaf, i);
+			name = name_loc->nameval;
+			namelen = name_loc->namelen;
+			valuelen = be16_to_cpu(name_loc->valuelen);
 		} else {
-			xfs_attr_leaf_name_remote_t *name_rmt =
-				xfs_attr3_leaf_name_remote(leaf, i);
-
-			int valuelen = be32_to_cpu(name_rmt->valuelen);
-
-			retval = context->put_listent(context,
-						entry->flags,
-						name_rmt->name,
-						(int)name_rmt->namelen,
-						valuelen);
-			if (retval)
-				return retval;
+			xfs_attr_leaf_name_remote_t *name_rmt;
+
+			name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
+			name = name_rmt->name;
+			namelen = name_rmt->namelen;
+			valuelen = be32_to_cpu(name_rmt->valuelen);
 		}
+
+		retval = context->put_listent(context, entry->flags,
+					      name, namelen, valuelen);
+		if (retval)
+			return retval;
 		if (context->seen_enough)
 			break;
 		cursor->offset++;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent
  2016-03-11 22:11 ` [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent Eric Sandeen
@ 2016-03-15  7:55   ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-03-15  7:55 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] xfs: don't pass value into attr ->put_listent
  2016-03-11 22:11 ` [PATCH 2/4] xfs: don't pass value into " Eric Sandeen
@ 2016-03-15  7:55   ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-03-15  7:55 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] xfs: remove put_value from attr ->put_listent context
  2016-03-11 22:12 ` [PATCH 3/4] xfs: remove put_value from attr ->put_listent context Eric Sandeen
@ 2016-03-15  7:56   ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-03-15  7:56 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Fri, Mar 11, 2016 at 04:12:13PM -0600, Eric Sandeen wrote:
> The put_value context member is never set; remove it
> and the conditional test in xfs_attr3_leaf_list_int().

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

(although this should probably go along with the previous patch)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int
  2016-03-11 22:12 ` [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int Eric Sandeen
@ 2016-03-15  7:57   ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-03-15  7:57 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-03-15  7:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-11 22:10 [PATCH 0/4] xfs: rework the attr ->put_listent code a bit Eric Sandeen
2016-03-11 22:11 ` [PATCH 1/4] xfs: only return -errno or success from attr ->put_listent Eric Sandeen
2016-03-15  7:55   ` Christoph Hellwig
2016-03-11 22:11 ` [PATCH 2/4] xfs: don't pass value into " Eric Sandeen
2016-03-15  7:55   ` Christoph Hellwig
2016-03-11 22:12 ` [PATCH 3/4] xfs: remove put_value from attr ->put_listent context Eric Sandeen
2016-03-15  7:56   ` Christoph Hellwig
2016-03-11 22:12 ` [PATCH 4/4] xfs: collapse cases in xfs_attr3_leaf_list_int Eric Sandeen
2016-03-15  7:57   ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.