xfs_grow: Remove xflag and iflag to reduce redundant temporary variables.

Commit Message

wuyifeng (C) Dec. 14, 2023, 2:41 a.m. UTC

Both xflag and iflag are log flags. We can use the bits of lflag to
indicate all log flags, which is a small code reconstruction.

Signed-off-by: Wu YiFeng <[e-mail]>
---
 growfs/xfs_growfs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Christoph Hellwig Dec. 14, 2023, 5 a.m. UTC | #1

On Thu, Dec 14, 2023 at 10:41:34AM +0800, wuyifeng (C) wrote:
> Both xflag and iflag are log flags. We can use the bits of lflag to
> indicate all log flags, which is a small code reconstruction.

I don't really see much of an upside here.  This now requires me to
go out of the function to figure out what the flags means, and it adds
overly long lines making reading the code harder.

Darrick J. Wong Dec. 14, 2023, 5:07 p.m. UTC | #2

On Wed, Dec 13, 2023 at 09:00:39PM -0800, Christoph Hellwig wrote:
> On Thu, Dec 14, 2023 at 10:41:34AM +0800, wuyifeng (C) wrote:
> > Both xflag and iflag are log flags. We can use the bits of lflag to
> > indicate all log flags, which is a small code reconstruction.
> 
> I don't really see much of an upside here.  This now requires me to
> go out of the function to figure out what the flags means, and it adds
> overly long lines making reading the code harder.

Also, lflags is a bitset, so what does (LOG_EXT2INT|LOG_INT2EXT) mean?

--D

Patch

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index 683961f6..5fb1a9d2 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -8,6 +8,10 @@ 
 #include "libfrog/paths.h"
 #include "libfrog/fsgeom.h"

+#define LOG_GROW	(1<<0)	/* -l flag: grow log section */
+#define LOG_EXT2INT	(1<<1)	/* -i flag: convert log from external to internal format */
+#define LOG_INT2EXT	(1<<2)	/* -x flag: convert log from internal to external format */
+
 static void
 usage(void)
 {
@@ -45,7 +49,6 @@  main(int argc, char **argv)
 	long			esize;	/* new rt extent size */
 	int			ffd;	/* mount point file descriptor */
 	struct xfs_fsop_geom	geo;	/* current fs geometry */
-	int			iflag;	/* -i flag */
 	int			isint;	/* log is currently internal */
 	int			lflag;	/* -l flag */
 	long long		lsize;	/* new log size in fs blocks */
@@ -55,7 +58,6 @@  main(int argc, char **argv)
 	struct xfs_fsop_geom	ngeo;	/* new fs geometry */
 	int			rflag;	/* -r flag */
 	long long		rsize;	/* new rt size in fs blocks */
-	int			xflag;	/* -x flag */
 	char			*fname;	/* mount point name */
 	char			*datadev; /* data device name */
 	char			*logdev;  /*  log device name */
@@ -72,7 +74,7 @@  main(int argc, char **argv)

 	maxpct = esize = 0;
 	dsize = lsize = rsize = 0LL;
-	aflag = dflag = iflag = lflag = mflag = nflag = rflag = xflag = 0;
+	aflag = dflag = lflag = mflag = nflag = rflag = 0;

 	while ((c = getopt(argc, argv, "dD:e:ilL:m:np:rR:t:xV")) != EOF) {
 		switch (c) {
@@ -87,13 +89,13 @@  main(int argc, char **argv)
 			rflag = 1;
 			break;
 		case 'i':
-			lflag = iflag = 1;
+			lflag |= LOG_EXT2INT;
 			break;
 		case 'L':
 			lsize = strtoll(optarg, NULL, 10);
 			fallthrough;
 		case 'l':
-			lflag = 1;
+			lflag |= LOG_GROW;
 			break;
 		case 'm':
 			mflag = 1;
@@ -115,7 +117,7 @@  main(int argc, char **argv)
 			mtab_file = optarg;
 			break;
 		case 'x':
-			lflag = xflag = 1;
+			lflag |= LOG_INT2EXT;
 			break;
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
@@ -124,9 +126,7 @@  main(int argc, char **argv)
 			usage();
 		}
 	}
-	if (argc - optind != 1)
-		usage();
-	if (iflag && xflag)
+	if (argc - optind != 1 || ((lflag & LOG_EXT2INT) && (lflag & LOG_INT2EXT)))
 		usage();
 	if (dflag + lflag + rflag + mflag == 0)
 		aflag = 1;
@@ -323,9 +323,9 @@  _("[EXPERIMENTAL] try to shrink unused space %lld, old size is %lld\n"),

 		if (!lsize)
 			lsize = dlsize / (geo.blocksize / BBSIZE);
-		if (iflag)
+		if (lflag & LOG_EXT2INT)
 			in.isint = 1;
-		else if (xflag)
+		else if (lflag & LOG_INT2EXT)
 			in.isint = 0;
 		else
 			in.isint = xi.logBBsize == 0;