[PATCH 0/5] xfs_io: address various "inode" command issues

[PATCH 0/5] xfs_io: address various "inode" command issues

[Eric Sandeen]

With these 5 patches the fstest I sent yesterday will
pass.

A lot of it is code re-org, but there a couple small bugs
resolved as well.

[PATCH 1/5] xfs_io: fix inode command help and argsmax

[Eric Sandeen]

The short help implied that -n and -v were exclusive, and the longer
help wasn't particularly clear.

Further, argsmax is wrong; "-n -v num" is 3, not 2.

 # xfs_io -c "inode -n -v 123" /mnt/test2
 bad argument count 3 to inode, expected between 0 and 2 arguments
 # xfs_io -c "inode -vn 123" /mnt/test2
 128:32

Fix up all of those issues.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/open.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/io/open.c b/io/open.c
index 3eaa013..5efa739 100644
--- a/io/open.c
+++ b/io/open.c
@@ -757,16 +757,13 @@ inode_help(void)
 {
 	printf(_(
 "\n"
-"Query physical information about the inode"
+"Query physical information about an inode"
 "\n"
-" Default:	-- Return true(1) or false(0) if any inode greater than\n"
-"		   32bits has been found in the filesystem\n"
-"[num]		-- Return inode number [num] or 0 if the inode [num] is in use\n"
-"		   or not\n"
-" -n [num]	-- Return the next valid inode after [num]\n"
-" -v		-- verbose mode\n"
-"		   Display the inode number and its physical size (in bits)\n"
-"		   according to the argument used\n"
+" Default:	-- Return 1 if any inode number greater than 32 bits exists in\n"
+"		   the filesystem, or 0 if none exist\n"
+" num		-- Return inode number [num] if in use, or 0 if not in use\n"
+" -n num	-- Return the next used inode after [num]\n"
+" -v		-- Verbose mode - display returned inode number's size in bits\n"
 "\n"));
 }
 
@@ -956,9 +953,9 @@ open_init(void)
 
 	inode_cmd.name = "inode";
 	inode_cmd.cfunc = inode_f;
-	inode_cmd.args = _("[-n | -v] [num]");
+	inode_cmd.args = _("[-nv] [num]");
 	inode_cmd.argmin = 0;
-	inode_cmd.argmax = 2;
+	inode_cmd.argmax = 3;
 	inode_cmd.flags = CMD_NOMAP_OK;
 	inode_cmd.oneline =
 		_("Query inode number usage in the filesystem");
-- 
1.7.1

[PATCH 2/5] xfs_io: factor out new get_last_inode() helper

[Eric Sandeen]

The inode command by default finds the last allocated
inode in the filesystem via bulkstat, and this specific
function is open-coded after other cases are handled, leading
to a fairly long inode_f function and confusing code flow.

Clean it up by factoring it into a new function, more refactoring
will follow.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/open.c |   68 +++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/io/open.c b/io/open.c
index 5efa739..c8c82b7 100644
--- a/io/open.c
+++ b/io/open.c
@@ -767,14 +767,51 @@ inode_help(void)
 "\n"));
 }
 
+static __u64
+get_last_inode(void)
+{
+	__u64			lastip = 0;
+	__u64			lastgrp = 0;
+	__s32			ocount = 0;
+	__u64			last_ino;
+	struct xfs_inogrp	igroup[1024];
+	struct xfs_fsop_bulkreq	bulkreq;
+
+	bulkreq.lastip = &lastip;
+	bulkreq.ubuffer = &igroup;
+	bulkreq.icount = sizeof(igroup) / sizeof(struct xfs_inogrp);
+	bulkreq.ocount = &ocount;
+
+	for (;;) {
+		if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
+				&bulkreq)) {
+			perror("XFS_IOC_FSINUMBERS");
+			return 0;
+		}
+
+		/* Did we reach the last inode? */
+		if (ocount == 0)
+			break;
+
+		/* last inode in igroup table */
+		lastgrp = ocount;
+	}
+
+	lastgrp--;
+
+	/* The last inode number in use */
+	last_ino = igroup[lastgrp].xi_startino +
+		  libxfs_highbit64(igroup[lastgrp].xi_allocmask);
+
+	return last_ino;
+}
+
 static int
 inode_f(
 	  int			argc,
 	  char			**argv)
 {
 	__s32			count = 0;
-	__s32			lastgrp = 0;
-	__u64			last = 0;
 	__u64			lastino = 0;
 	__u64			userino = 0;
 	char			*p;
@@ -782,7 +819,6 @@ inode_f(
 	int			verbose = 0;
 	int			ret_next = 0;
 	int			cmd = 0;
-	struct xfs_inogrp	igroup[1024];
 	struct xfs_fsop_bulkreq	bulkreq;
 	struct xfs_bstat	bstat;
 
@@ -854,29 +890,13 @@ inode_f(
 		return command_usage(&inode_cmd);
 	}
 
-	bulkreq.lastip = &last;
-	bulkreq.icount = 1024; /* User-defined maybe!? */
-	bulkreq.ubuffer = &igroup;
-	bulkreq.ocount = &count;
-
-	for (;;) {
-		if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
-				&bulkreq)) {
-			perror("XFS_IOC_FSINUMBERS");
-			exitcode = 1;
-			return 0;
-		}
-
-		if (count == 0)
-			break;
-
-		lastgrp = count;
+	/* We are finding last inode in use */
+	lastino = get_last_inode();
+	if (!lastino) {
+		exitcode = 1;
+		return 0;
 	}
 
-	lastgrp--;
-	lastino = igroup[lastgrp].xi_startino +
-		  libxfs_highbit64(igroup[lastgrp].xi_allocmask);
-
 	if (verbose)
 		printf("%llu:%d\n", lastino,
 			lastino > XFS_MAXINUMBER_32 ? 64 : 32);
-- 
1.7.1

[PATCH 3/5] xfs_io: move inode command arg handling to top

[Eric Sandeen]

As it stands, collecting the inode number and testing args
validity is all tangled up; for example the test for
"-n" having no inode is buried in an else after a large
code block which handles something else.

Get inode number argument collection and testing out of the
way before doing anything else.

Clean up the error message if a non-numeric inode arg is given.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/open.c |   40 +++++++++++++++++++++-------------------
 1 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/io/open.c b/io/open.c
index c8c82b7..ba8b243 100644
--- a/io/open.c
+++ b/io/open.c
@@ -813,7 +813,7 @@ inode_f(
 {
 	__s32			count = 0;
 	__u64			lastino = 0;
-	__u64			userino = 0;
+	__u64			userino = NULLFSINO;
 	char			*p;
 	int			c;
 	int			verbose = 0;
@@ -835,27 +835,32 @@ inode_f(
 		}
 	}
 
-	/*
-	 * Inode number can be passed with or without extra arguments, so we
-	 * should handle inode numbers passed by user out of getopt()
-	 */
+	/* Last arg (if present) should be an inode number */
 	if (optind < argc) {
-
-		if (ret_next) {
-			cmd = XFS_IOC_FSBULKSTAT;
-		} else {
-			if ((argc > 2) && !verbose)
-				return command_usage(&inode_cmd);
-			else
-				cmd = XFS_IOC_FSBULKSTAT_SINGLE;
-		}
-
 		userino = strtoull(argv[optind], &p, 10);
 		if ((*p != '\0')) {
-			printf(_("[num] must be a numeric value\n"));
+			printf(_("%s is not a numeric inode value\n"),
+				argv[optind]);
 			exitcode = 1;
 			return 0;
 		}
+		optind++;
+	}
+
+	/* Extra junk? */
+	if (optind < argc)
+		return command_usage(&inode_cmd);
+
+	/* -n option requires an inode number */
+	if (ret_next && userino == NULLFSINO)
+		return command_usage(&inode_cmd);
+
+	if (userino != NULLFSINO) {
+
+		if (ret_next)	/* get next inode */
+			cmd = XFS_IOC_FSBULKSTAT;
+		else		/* get this inode */
+			cmd = XFS_IOC_FSBULKSTAT_SINGLE;
 
 		bulkreq.lastip = &userino;
 		bulkreq.icount = 1;
@@ -885,9 +890,6 @@ inode_f(
 			printf("%llu\n", userino);
 		return 0;
 
-	/* -n option must not be used stand alone */
-	} else if (ret_next) {
-		return command_usage(&inode_cmd);
 	}
 
 	/* We are finding last inode in use */
-- 
1.7.1

[PATCH 4/5] xfs_io: refactor inode command

[Eric Sandeen]

The inode_f function is a bit convoluted; the default
find-last-inode case appears at the end, there are several return
points, we print the same basic information using 2 different variables
in 2 different locations depending on the mode we're in, the
"inode not found" was a printf & exit in the middle of the function,
 etc.

Move the default case up to the top so it's more obvious, not
buried.

Make a new var, result_ino, which holds whatever we want to print
regardless of the mode, and then handle all the output at the end.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/open.c |   65 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/io/open.c b/io/open.c
index ba8b243..7d952a0 100644
--- a/io/open.c
+++ b/io/open.c
@@ -812,7 +812,7 @@ inode_f(
 	  char			**argv)
 {
 	__s32			count = 0;
-	__u64			lastino = 0;
+	__u64			result_ino = 0;
 	__u64			userino = NULLFSINO;
 	char			*p;
 	int			c;
@@ -855,8 +855,14 @@ inode_f(
 	if (ret_next && userino == NULLFSINO)
 		return command_usage(&inode_cmd);
 
-	if (userino != NULLFSINO) {
-
+	if (userino == NULLFSINO) {
+		/* We are finding last inode in use */
+		result_ino = get_last_inode();
+		if (!result_ino) {
+			exitcode = 1;
+			return 0;
+		}
+	} else {
 		if (ret_next)	/* get next inode */
 			cmd = XFS_IOC_FSBULKSTAT;
 		else		/* get this inode */
@@ -868,43 +874,32 @@ inode_f(
 		bulkreq.ocount = &count;
 
 		if (xfsctl(file->name, file->fd, cmd, &bulkreq)) {
-			if (errno == EINVAL) {
-				if (!ret_next)
-					printf("0\n");
+			if (!ret_next && errno == EINVAL) {
+				/* Not in use */
+				result_ino = 0;
 			} else {
 				perror("xfsctl");
+				exitcode = 1;
+				return 0;
 			}
-			exitcode = 1;
-			return 0;
-		}
-
-		if (ret_next)
-			userino = bstat.bs_ino;
-
-		if (verbose)
-			printf("%llu:%d\n",
-			       userino,
-			       userino > XFS_MAXINUMBER_32 ? 64 : 32);
-		else
-			/* Inode in use */
-			printf("%llu\n", userino);
-		return 0;
-
-	}
-
-	/* We are finding last inode in use */
-	lastino = get_last_inode();
-	if (!lastino) {
-		exitcode = 1;
-		return 0;
+		} else if (ret_next)	/* The next inode in use */
+			result_ino = bstat.bs_ino;
+		else			/* The inode we asked about */
+			result_ino = userino;
+	}
+
+	if (verbose && result_ino) {
+		/* Requested verbose and we have an answer */
+		printf("%llu:%d\n", result_ino,
+			result_ino > XFS_MAXINUMBER_32 ? 64 : 32);
+	} else if (userino == NULLFSINO) {
+		/* Just checking 32 or 64 bit presence, non-verbose */
+		printf("%d\n", result_ino > XFS_MAXINUMBER_32 ? 1 : 0);
+	} else {
+		/* We asked about a specific inode, non-verbose */
+		printf("%llu\n", result_ino);
 	}
 
-	if (verbose)
-		printf("%llu:%d\n", lastino,
-			lastino > XFS_MAXINUMBER_32 ? 64 : 32);
-	else
-		printf("%d\n", lastino > XFS_MAXINUMBER_32 ? 1 : 0);
-
 	return 0;
 }
 
-- 
1.7.1

[PATCH 5/5] xfs_io: fix inode command with "-n" for bogus inode

[Eric Sandeen]

If we ask for the next allocated inode after a number for which
no other inode exists, the bulkstat returns success, but with
count == 0.  If we ignore this fact, we print a garbage result
from bstat.bs_ino in this case, so fix it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/open.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/io/open.c b/io/open.c
index 7d952a0..4279062 100644
--- a/io/open.c
+++ b/io/open.c
@@ -882,10 +882,16 @@ inode_f(
 				exitcode = 1;
 				return 0;
 			}
-		} else if (ret_next)	/* The next inode in use */
-			result_ino = bstat.bs_ino;
-		else			/* The inode we asked about */
+		} else if (ret_next) {
+			/* The next inode in use, or 0 if none */
+			if (*bulkreq.ocount)
+				result_ino = bstat.bs_ino;
+			else
+				result_ino = 0;
+		} else {
+			/* The inode we asked about */
 			result_ino = userino;
+		}
 	}
 
 	if (verbose && result_ino) {
-- 
1.7.1