[RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Jan Tulak]

Followup of my "[xfsprogs] Do we need so many data types for user input?" email.

In the past, when mkfs was first written, it used atoi and
similar calls, so the variables were ints. However, the situation moved
since then and in course of the time, mkfs began to use other types too.

Clean and unify it. We don't need negative values anywhere in the code and
some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
type.

This patch changes variables declared at the beginning of main() + block/sectorsize, making only minimal changes. The following patch cleans some now-unnecessary type casts.

It would be nice to change types in some of the structures too, but
this might lead to changes outside of mkfs, so I'm skipping them for
this moment to keep it simple.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
 mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 92 insertions(+), 92 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 5aac4d1b..71382f70 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -39,8 +39,8 @@ static int  ispow2(unsigned int i);
  * The configured block and sector sizes are defined as global variables so
  * that they don't need to be passed to functions that require them.
  */
-unsigned int		blocksize;
-unsigned int		sectorsize;
+__uint64_t		blocksize;
+__uint64_t		sectorsize;
 
 #define MAX_SUBOPTS	16
 #define SUBOPT_NEEDS_VAL	(-1LL)
@@ -742,9 +742,9 @@ calc_stripe_factors(
 	int		dsectsz,
 	int		lsu,
 	int		lsectsz,
-	int		*dsunit,
-	int		*dswidth,
-	int		*lsunit)
+	__uint64_t	*dsunit,
+	__uint64_t	*dswidth,
+	__uint64_t	*lsunit)
 {
 	/* Handle data sunit/swidth options */
 	if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
@@ -769,32 +769,32 @@ calc_stripe_factors(
 			usage();
 		}
 
-		*dsunit  = (int)BTOBBT(dsu);
+		*dsunit  = BTOBBT(dsu);
 		*dswidth = *dsunit * dsw;
 	}
 
 	if (*dsunit && (*dswidth % *dsunit != 0)) {
 		fprintf(stderr,
-			_("data stripe width (%d) must be a multiple of the "
-			"data stripe unit (%d)\n"), *dswidth, *dsunit);
+			_("data stripe width (%lu) must be a multiple of the "
+			"data stripe unit (%lu)\n"), *dswidth, *dsunit);
 		usage();
 	}
 
 	/* Handle log sunit options */
 
 	if (lsu)
-		*lsunit = (int)BTOBBT(lsu);
+		*lsunit = BTOBBT(lsu);
 
 	/* verify if lsu/lsunit is a multiple block size */
 	if (lsu % blocksize != 0) {
 		fprintf(stderr,
-_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
+_("log stripe unit (%d) must be a multiple of the block size (%lu)\n"),
 		lsu, blocksize);
 		exit(1);
 	}
 	if ((BBTOB(*lsunit) % blocksize != 0)) {
 		fprintf(stderr,
-_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
+_("log stripe unit (%lu) must be a multiple of the block size (%lu)\n"),
 		BBTOB(*lsunit), blocksize);
 		exit(1);
 	}
@@ -917,7 +917,7 @@ fixup_internal_log_stripe(
 	int		sunit,
 	xfs_rfsblock_t	*logblocks,
 	int		blocklog,
-	int		*lalign)
+	__uint64_t	*lalign)
 {
 	if ((logstart % sunit) != 0) {
 		logstart = ((logstart + (sunit - 1))/sunit) * sunit;
@@ -1405,70 +1405,70 @@ main(
 	__uint64_t		agsize;
 	xfs_alloc_rec_t		*arec;
 	struct xfs_btree_block	*block;
-	int			blflag;
-	int			blocklog;
-	int			bsflag;
-	int			bsize;
+	__uint64_t		blflag;
+	__uint64_t		blocklog;
+	__uint64_t		bsflag;
+	__uint64_t		bsize;
 	xfs_buf_t		*buf;
-	int			c;
-	int			daflag;
-	int			dasize;
+	__uint64_t		c;
+	__uint64_t		daflag;
+	__uint64_t		dasize;
 	xfs_rfsblock_t		dblocks;
 	char			*dfile;
-	int			dirblocklog;
-	int			dirblocksize;
+	__uint64_t		dirblocklog;
+	__uint64_t		dirblocksize;
 	char			*dsize;
-	int			dsu;
-	int			dsw;
-	int			dsunit;
-	int			dswidth;
-	int			force_overwrite;
+	__uint64_t		dsu;
+	__uint64_t		dsw;
+	__uint64_t		dsunit;
+	__uint64_t		dswidth;
+	__uint64_t		force_overwrite;
 	struct fsxattr		fsx;
-	int			ilflag;
-	int			imaxpct;
-	int			imflag;
-	int			inodelog;
-	int			inopblock;
-	int			ipflag;
-	int			isflag;
-	int			isize;
+	__uint64_t		ilflag;
+	__uint64_t		imaxpct;
+	__uint64_t		imflag;
+	__uint64_t		inodelog;
+	__uint64_t		inopblock;
+	__uint64_t		ipflag;
+	__uint64_t		isflag;
+	__uint64_t		isize;
 	char			*label = NULL;
-	int			laflag;
-	int			lalign;
-	int			ldflag;
-	int			liflag;
+	__uint64_t		laflag;
+	__uint64_t		lalign;
+	__uint64_t		ldflag;
+	__uint64_t		liflag;
 	xfs_agnumber_t		logagno;
 	xfs_rfsblock_t		logblocks;
 	char			*logfile;
-	int			loginternal;
+	__uint64_t		loginternal;
 	char			*logsize;
 	xfs_fsblock_t		logstart;
-	int			lvflag;
-	int			lsflag;
-	int			lsuflag;
-	int			lsunitflag;
-	int			lsectorlog;
-	int			lsectorsize;
-	int			lslflag;
-	int			lssflag;
-	int			lsu;
-	int			lsunit;
-	int			min_logblocks;
+	__uint64_t		lvflag;
+	__uint64_t		lsflag;
+	__uint64_t		lsuflag;
+	__uint64_t		lsunitflag;
+	__uint64_t		lsectorlog;
+	__uint64_t		lsectorsize;
+	__uint64_t		lslflag;
+	__uint64_t		lssflag;
+	__uint64_t		lsu;
+	__uint64_t		lsunit;
+	__uint64_t		min_logblocks;
 	xfs_mount_t		*mp;
 	xfs_mount_t		mbuf;
 	xfs_extlen_t		nbmblocks;
-	int			nlflag;
-	int			nodsflag;
-	int			norsflag;
+	__uint64_t		nlflag;
+	__uint64_t		nodsflag;
+	__uint64_t		norsflag;
 	xfs_alloc_rec_t		*nrec;
-	int			nsflag;
-	int			nvflag;
-	int			Nflag;
-	int			discard = 1;
+	__uint64_t		nsflag;
+	__uint64_t		nvflag;
+	__uint64_t		Nflag;
+	__uint64_t		discard = 1;
 	char			*p;
 	char			*protofile;
 	char			*protostring;
-	int			qflag;
+	__uint64_t		qflag;
 	xfs_rfsblock_t		rtblocks;
 	xfs_extlen_t		rtextblocks;
 	xfs_rtblock_t		rtextents;
@@ -1476,13 +1476,13 @@ main(
 	char			*rtfile;
 	char			*rtsize;
 	xfs_sb_t		*sbp;
-	int			sectorlog;
+	__uint64_t		sectorlog;
 	__uint64_t		sector_mask;
-	int			slflag;
-	int			ssflag;
+	__uint64_t		slflag;
+	__uint64_t		ssflag;
 	__uint64_t		tmp_agsize;
 	uuid_t			uuid;
-	int			worst_freelist;
+	__uint64_t		worst_freelist;
 	libxfs_init_t		xi;
 	struct fs_topology	ft;
 	struct sb_feat_args	sb_feat = {
@@ -1946,7 +1946,7 @@ main(
 		blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
 	}
 	if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
-		fprintf(stderr, _("illegal block size %d\n"), blocksize);
+		fprintf(stderr, _("illegal block size %lu\n"), blocksize);
 		usage();
 	}
 	if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
@@ -2010,7 +2010,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
 
 		if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {
 			fprintf(stderr,
-_("specified blocksize %d is less than device physical sector size %d\n"),
+_("specified blocksize %lu is less than device physical sector size %d\n"),
 				blocksize, ft.psectorsize);
 			fprintf(stderr,
 _("switching to logical sector size %d\n"),
@@ -2031,21 +2031,21 @@ _("switching to logical sector size %d\n"),
 	if (sectorsize < XFS_MIN_SECTORSIZE ||
 	    sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
 		if (ssflag)
-			fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
+			fprintf(stderr, _("illegal sector size %lu\n"), sectorsize);
 		else
 			fprintf(stderr,
-_("block size %d cannot be smaller than logical sector size %d\n"),
+_("block size %lu cannot be smaller than logical sector size %d\n"),
 				blocksize, ft.lsectorsize);
 		usage();
 	}
 	if (sectorsize < ft.lsectorsize) {
-		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
+		fprintf(stderr, _("illegal sector size %lu; hw sector is %d\n"),
 			sectorsize, ft.lsectorsize);
 		usage();
 	}
 	if (lsectorsize < XFS_MIN_SECTORSIZE ||
 	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
-		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
+		fprintf(stderr, _("illegal log sector size %lu\n"), lsectorsize);
 		usage();
 	} else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
 		lsu = blocksize;
@@ -2151,7 +2151,7 @@ _("rmapbt not supported with realtime devices\n"));
 	if (nsflag || nlflag) {
 		if (dirblocksize < blocksize ||
 					dirblocksize > XFS_MAX_BLOCKSIZE) {
-			fprintf(stderr, _("illegal directory block size %d\n"),
+			fprintf(stderr, _("illegal directory block size %lu\n"),
 				dirblocksize);
 			usage();
 		}
@@ -2177,7 +2177,7 @@ _("rmapbt not supported with realtime devices\n"));
 		dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
 		if (dbytes % blocksize)
 			fprintf(stderr, _("warning: "
-	"data length %lld not a multiple of %d, truncated to %lld\n"),
+	"data length %lld not a multiple of %lu, truncated to %lld\n"),
 				(long long)dbytes, blocksize,
 				(long long)(dblocks << blocklog));
 	}
@@ -2209,7 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
 		logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
 		if (logbytes % blocksize)
 			fprintf(stderr,
-	_("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
+	_("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
 				(long long)logbytes, blocksize,
 				(long long)(logblocks << blocklog));
 	}
@@ -2226,7 +2226,7 @@ _("rmapbt not supported with realtime devices\n"));
 		rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
 		if (rtbytes % blocksize)
 			fprintf(stderr,
-	_("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
+	_("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
 				(long long)rtbytes, blocksize,
 				(long long)(rtblocks << blocklog));
 	}
@@ -2239,7 +2239,7 @@ _("rmapbt not supported with realtime devices\n"));
 		rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
 		if (rtextbytes % blocksize) {
 			fprintf(stderr,
-		_("illegal rt extent size %lld, not a multiple of %d\n"),
+		_("illegal rt extent size %lld, not a multiple of %lu\n"),
 				(long long)rtextbytes, blocksize);
 			usage();
 		}
@@ -2282,16 +2282,16 @@ _("rmapbt not supported with realtime devices\n"));
 	    isize > XFS_DINODE_MAX_SIZE) {
 		int	maxsz;
 
-		fprintf(stderr, _("illegal inode size %d\n"), isize);
+		fprintf(stderr, _("illegal inode size %lu\n"), isize);
 		maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
 			    XFS_DINODE_MAX_SIZE);
 		if (XFS_DINODE_MIN_SIZE == maxsz)
 			fprintf(stderr,
-			_("allowable inode size with %d byte blocks is %d\n"),
+			_("allowable inode size with %lu byte blocks is %d\n"),
 				blocksize, XFS_DINODE_MIN_SIZE);
 		else
 			fprintf(stderr,
-	_("allowable inode size with %d byte blocks is between %d and %d\n"),
+	_("allowable inode size with %lu byte blocks is between %d and %d\n"),
 				blocksize, XFS_DINODE_MIN_SIZE, maxsz);
 		exit(1);
 	}
@@ -2395,19 +2395,19 @@ _("rmapbt not supported with realtime devices\n"));
 
 	if (xi.dbsize > sectorsize) {
 		fprintf(stderr, _(
-"Warning: the data subvolume sector size %u is less than the sector size \n\
+"Warning: the data subvolume sector size %lu is less than the sector size \n\
 reported by the device (%u).\n"),
 			sectorsize, xi.dbsize);
 	}
 	if (!loginternal && xi.lbsize > lsectorsize) {
 		fprintf(stderr, _(
-"Warning: the log subvolume sector size %u is less than the sector size\n\
+"Warning: the log subvolume sector size %lu is less than the sector size\n\
 reported by the device (%u).\n"),
 			lsectorsize, xi.lbsize);
 	}
 	if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
 		fprintf(stderr, _(
-"Warning: the realtime subvolume sector size %u is less than the sector size\n\
+"Warning: the realtime subvolume sector size %lu is less than the sector size\n\
 reported by the device (%u).\n"),
 			sectorsize, xi.rtbsize);
 	}
@@ -2437,14 +2437,14 @@ reported by the device (%u).\n"),
 		if (dsunit) {
 			if (ft.dsunit && ft.dsunit != dsunit) {
 				fprintf(stderr,
-					_("%s: Specified data stripe unit %d "
+					_("%s: Specified data stripe unit %lu "
 					"is not the same as the volume stripe "
 					"unit %d\n"),
 					progname, dsunit, ft.dsunit);
 			}
 			if (ft.dswidth && ft.dswidth != dswidth) {
 				fprintf(stderr,
-					_("%s: Specified data stripe width %d "
+					_("%s: Specified data stripe width %lu "
 					"is not the same as the volume stripe "
 					"width %d\n"),
 					progname, dswidth, ft.dswidth);
@@ -2462,7 +2462,7 @@ reported by the device (%u).\n"),
 		 */
 		if (agsize % blocksize) {
 			fprintf(stderr,
-		_("agsize (%lld) not a multiple of fs blk size (%d)\n"),
+		_("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
 				(long long)agsize, blocksize);
 			usage();
 		}
@@ -2512,7 +2512,7 @@ reported by the device (%u).\n"),
 						(dblocks % agsize != 0);
 				if (dasize)
 					fprintf(stderr,
-				_("agsize rounded to %lld, swidth = %d\n"),
+				_("agsize rounded to %lld, swidth = %lu\n"),
 						(long long)agsize, dswidth);
 			} else {
 				if (nodsflag) {
@@ -2569,8 +2569,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
 			dsunit = dswidth = 0;
 		else {
 			fprintf(stderr,
-				_("%s: Stripe unit(%d) or stripe width(%d) is "
-				"not a multiple of the block size(%d)\n"),
+				_("%s: Stripe unit(%lu) or stripe width(%lu) is "
+				"not a multiple of the block size(%lu)\n"),
 				progname, BBTOB(dsunit), BBTOB(dswidth),
 				blocksize);
 			exit(1);
@@ -2610,7 +2610,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
 		/* Warn only if specified on commandline */
 		if (lsuflag || lsunitflag) {
 			fprintf(stderr,
-	_("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
+	_("log stripe unit (%lu bytes) is too large (maximum is 256KiB)\n"),
 				(lsunit * blocksize));
 			fprintf(stderr,
 	_("log stripe unit adjusted to 32KiB\n"));
@@ -2755,14 +2755,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
 
 	if (!qflag || Nflag) {
 		printf(_(
-		   "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
-		   "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
+		   "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
+		   "         =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
 		   "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
-		   "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
-		   "         =%-22s sunit=%-6u swidth=%u blks\n"
-		   "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
+		   "data     =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
+		   "         =%-22s sunit=%-6lu swidth=%lu blks\n"
+		   "naming   =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
 		   "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
-		   "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
+		   "         =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
 		   "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
 			dfile, isize, (long long)agcount, (long long)agsize,
 			"", sectorsize, sb_feat.attr_version,
-- 
2.12.1

[RFC PATCH 2/2] mkfs: remove long long type casts

[Jan Tulak]

We have uint64, why cast it to long long for prints? Just print it as
it is.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
 mkfs/xfs_mkfs.c | 144 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 72 insertions(+), 72 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 71382f70..885b5c0a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -900,9 +900,9 @@ fixup_log_stripe_unit(
 			}
 			*logblocks = tmp_logblocks;
 		} else {
-			fprintf(stderr, _("log size %lld is not a multiple "
+			fprintf(stderr, _("log size %lu is not a multiple "
 					  "of the log stripe unit %d\n"),
-				(long long) *logblocks, sunit);
+				*logblocks, sunit);
 			usage();
 		}
 	}
@@ -929,7 +929,7 @@ fixup_internal_log_stripe(
 	if (*logblocks > agsize - XFS_FSB_TO_AGBNO(mp, logstart)) {
 		fprintf(stderr,
 			_("Due to stripe alignment, the internal log size "
-			"(%lld) is too large.\n"), (long long) *logblocks);
+			"(%lu) is too large.\n"), *logblocks);
 		fprintf(stderr, _("Must fit within an allocation group.\n"));
 		usage();
 	}
@@ -941,20 +941,20 @@ validate_log_size(__uint64_t logblocks, int blocklog, int min_logblocks)
 {
 	if (logblocks < min_logblocks) {
 		fprintf(stderr,
-	_("log size %lld blocks too small, minimum size is %d blocks\n"),
-			(long long)logblocks, min_logblocks);
+	_("log size %lu blocks too small, minimum size is %d blocks\n"),
+			logblocks, min_logblocks);
 		usage();
 	}
 	if (logblocks > XFS_MAX_LOG_BLOCKS) {
 		fprintf(stderr,
-	_("log size %lld blocks too large, maximum size is %lld blocks\n"),
-			(long long)logblocks, XFS_MAX_LOG_BLOCKS);
+	_("log size %lu blocks too large, maximum size is %lld blocks\n"),
+			logblocks, XFS_MAX_LOG_BLOCKS);
 		usage();
 	}
 	if ((logblocks << blocklog) > XFS_MAX_LOG_BYTES) {
 		fprintf(stderr,
-	_("log size %lld bytes too large, maximum size is %lld bytes\n"),
-			(long long)(logblocks << blocklog), XFS_MAX_LOG_BYTES);
+	_("log size %lu bytes too large, maximum size is %lld bytes\n"),
+			(logblocks << blocklog), XFS_MAX_LOG_BYTES);
 		usage();
 	}
 }
@@ -990,43 +990,43 @@ validate_ag_geometry(
 {
 	if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
 		fprintf(stderr,
-	_("agsize (%lld blocks) too small, need at least %lld blocks\n"),
-			(long long)agsize,
-			(long long)XFS_AG_MIN_BLOCKS(blocklog));
+	_("agsize (%lu blocks) too small, need at least %lu blocks\n"),
+			agsize,
+			(__uint64_t)XFS_AG_MIN_BLOCKS(blocklog));
 		usage();
 	}
 
 	if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
 		fprintf(stderr,
-	_("agsize (%lld blocks) too big, maximum is %lld blocks\n"),
-			(long long)agsize,
-			(long long)XFS_AG_MAX_BLOCKS(blocklog));
+	_("agsize (%lu blocks) too big, maximum is %lu blocks\n"),
+			agsize,
+			(__uint64_t)XFS_AG_MAX_BLOCKS(blocklog));
 		usage();
 	}
 
 	if (agsize > dblocks) {
 		fprintf(stderr,
-	_("agsize (%lld blocks) too big, data area is %lld blocks\n"),
-			(long long)agsize, (long long)dblocks);
+	_("agsize (%lu blocks) too big, data area is %lu blocks\n"),
+			agsize, dblocks);
 			usage();
 	}
 
 	if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
 		fprintf(stderr,
-	_("too many allocation groups for size = %lld\n"),
-				(long long)agsize);
-		fprintf(stderr, _("need at most %lld allocation groups\n"),
-			(long long)(dblocks / XFS_AG_MIN_BLOCKS(blocklog) +
+	_("too many allocation groups for size = %lu\n"),
+				agsize);
+		fprintf(stderr, _("need at most %lu allocation groups\n"),
+			(__uint64_t) (dblocks / XFS_AG_MIN_BLOCKS(blocklog) +
 				(dblocks % XFS_AG_MIN_BLOCKS(blocklog) != 0)));
 		usage();
 	}
 
 	if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
 		fprintf(stderr,
-	_("too few allocation groups for size = %lld\n"), (long long)agsize);
+	_("too few allocation groups for size = %lu\n"), agsize);
 		fprintf(stderr,
-	_("need at least %lld allocation groups\n"),
-		(long long)(dblocks / XFS_AG_MAX_BLOCKS(blocklog) +
+	_("need at least %lu allocation groups\n"),
+		(__uint64_t)(dblocks / XFS_AG_MAX_BLOCKS(blocklog) +
 			(dblocks % XFS_AG_MAX_BLOCKS(blocklog) != 0)));
 		usage();
 	}
@@ -1038,9 +1038,9 @@ validate_ag_geometry(
 	if ( dblocks % agsize != 0 &&
 	     (dblocks % agsize < XFS_AG_MIN_BLOCKS(blocklog))) {
 		fprintf(stderr,
-	_("last AG size %lld blocks too small, minimum size is %lld blocks\n"),
-			(long long)(dblocks % agsize),
-			(long long)XFS_AG_MIN_BLOCKS(blocklog));
+	_("last AG size %lu blocks too small, minimum size is %lu blocks\n"),
+			(dblocks % agsize),
+			(__uint64_t)XFS_AG_MIN_BLOCKS(blocklog));
 		usage();
 	}
 
@@ -1049,8 +1049,8 @@ validate_ag_geometry(
 	 */
 	if (agcount > XFS_MAX_AGNUMBER + 1) {
 		fprintf(stderr,
-	_("%lld allocation groups is too many, maximum is %lld\n"),
-			(long long)agcount, (long long)XFS_MAX_AGNUMBER + 1);
+	_("%lu allocation groups is too many, maximum is %lu\n"),
+			agcount, (__uint64_t)XFS_MAX_AGNUMBER + 1);
 		usage();
 	}
 }
@@ -2170,16 +2170,16 @@ _("rmapbt not supported with realtime devices\n"));
 		dbytes = getnum(dsize, &dopts, D_SIZE);
 		if (dbytes % XFS_MIN_BLOCKSIZE) {
 			fprintf(stderr,
-			_("illegal data length %lld, not a multiple of %d\n"),
-				(long long)dbytes, XFS_MIN_BLOCKSIZE);
+			_("illegal data length %lu, not a multiple of %d\n"),
+				dbytes, XFS_MIN_BLOCKSIZE);
 			usage();
 		}
 		dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
 		if (dbytes % blocksize)
 			fprintf(stderr, _("warning: "
-	"data length %lld not a multiple of %lu, truncated to %lld\n"),
-				(long long)dbytes, blocksize,
-				(long long)(dblocks << blocklog));
+	"data length %lu not a multiple of %lu, truncated to %lu\n"),
+				dbytes, blocksize,
+				(__uint64_t)(dblocks << blocklog));
 	}
 	if (ipflag) {
 		inodelog = blocklog - libxfs_highbit32(inopblock);
@@ -2202,16 +2202,16 @@ _("rmapbt not supported with realtime devices\n"));
 		logbytes = getnum(logsize, &lopts, L_SIZE);
 		if (logbytes % XFS_MIN_BLOCKSIZE) {
 			fprintf(stderr,
-			_("illegal log length %lld, not a multiple of %d\n"),
-				(long long)logbytes, XFS_MIN_BLOCKSIZE);
+			_("illegal log length %lu, not a multiple of %d\n"),
+				logbytes, XFS_MIN_BLOCKSIZE);
 			usage();
 		}
 		logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
 		if (logbytes % blocksize)
 			fprintf(stderr,
-	_("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
-				(long long)logbytes, blocksize,
-				(long long)(logblocks << blocklog));
+	_("warning: log length %lu not a multiple of %lu, truncated to %lu\n"),
+				logbytes, blocksize,
+				(__uint64_t)(logblocks << blocklog));
 	}
 	if (rtsize) {
 		__uint64_t rtbytes;
@@ -2219,16 +2219,16 @@ _("rmapbt not supported with realtime devices\n"));
 		rtbytes = getnum(rtsize, &ropts, R_SIZE);
 		if (rtbytes % XFS_MIN_BLOCKSIZE) {
 			fprintf(stderr,
-			_("illegal rt length %lld, not a multiple of %d\n"),
-				(long long)rtbytes, XFS_MIN_BLOCKSIZE);
+			_("illegal rt length %lu, not a multiple of %d\n"),
+				rtbytes, XFS_MIN_BLOCKSIZE);
 			usage();
 		}
 		rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
 		if (rtbytes % blocksize)
 			fprintf(stderr,
-	_("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
-				(long long)rtbytes, blocksize,
-				(long long)(rtblocks << blocklog));
+	_("warning: rt length %lu not a multiple of %lu, truncated to %lu\n"),
+				rtbytes, blocksize,
+				(__uint64_t)(rtblocks << blocklog));
 	}
 	/*
 	 * If specified, check rt extent size against its constraints.
@@ -2239,8 +2239,8 @@ _("rmapbt not supported with realtime devices\n"));
 		rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
 		if (rtextbytes % blocksize) {
 			fprintf(stderr,
-		_("illegal rt extent size %lld, not a multiple of %lu\n"),
-				(long long)rtextbytes, blocksize);
+		_("illegal rt extent size %lu, not a multiple of %lu\n"),
+				rtextbytes, blocksize);
 			usage();
 		}
 		rtextblocks = (xfs_extlen_t)(rtextbytes >> blocklog);
@@ -2367,8 +2367,8 @@ _("rmapbt not supported with realtime devices\n"));
 	if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
 		fprintf(stderr,
 			_("size %s specified for data subvolume is too large, "
-			"maximum is %lld blocks\n"),
-			dsize, (long long)DTOBT(xi.dsize));
+			"maximum is %lu blocks\n"),
+			dsize, (__uint64_t)DTOBT(xi.dsize));
 		usage();
 	} else if (!dsize && xi.dsize > 0)
 		dblocks = DTOBT(xi.dsize);
@@ -2378,8 +2378,8 @@ _("rmapbt not supported with realtime devices\n"));
 	}
 	if (dblocks < XFS_MIN_DATA_BLOCKS) {
 		fprintf(stderr,
-	_("size %lld of data subvolume is too small, minimum %d blocks\n"),
-			(long long)dblocks, XFS_MIN_DATA_BLOCKS);
+	_("size %lu of data subvolume is too small, minimum %d blocks\n"),
+			dblocks, XFS_MIN_DATA_BLOCKS);
 		usage();
 	}
 
@@ -2415,8 +2415,8 @@ reported by the device (%u).\n"),
 	if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
 		fprintf(stderr,
 			_("size %s specified for rt subvolume is too large, "
-			"maximum is %lld blocks\n"),
-			rtsize, (long long)DTOBT(xi.rtsize));
+			"maximum is %lu blocks\n"),
+			rtsize, (__uint64_t)DTOBT(xi.rtsize));
 		usage();
 	} else if (!rtsize && xi.rtsize > 0)
 		rtblocks = DTOBT(xi.rtsize);
@@ -2462,8 +2462,8 @@ reported by the device (%u).\n"),
 		 */
 		if (agsize % blocksize) {
 			fprintf(stderr,
-		_("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
-				(long long)agsize, blocksize);
+		_("agsize (%lu) not a multiple of fs blk size (%lu)\n"),
+				agsize, blocksize);
 			usage();
 		}
 		agsize /= blocksize;
@@ -2512,8 +2512,8 @@ reported by the device (%u).\n"),
 						(dblocks % agsize != 0);
 				if (dasize)
 					fprintf(stderr,
-				_("agsize rounded to %lld, swidth = %lu\n"),
-						(long long)agsize, dswidth);
+				_("agsize rounded to %lu, swidth = %lu\n"),
+						agsize, dswidth);
 			} else {
 				if (nodsflag) {
 					dsunit = dswidth = 0;
@@ -2629,8 +2629,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
 		min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
 	if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
 		fprintf(stderr,
-_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
-			logsize, (long long)DTOBT(xi.logBBsize));
+_("size %s specified for log subvolume is too large, maximum is %lu blocks\n"),
+			logsize, (__uint64_t)DTOBT(xi.logBBsize));
 		usage();
 	} else if (!logsize && xi.logBBsize > 0) {
 		logblocks = DTOBT(xi.logBBsize);
@@ -2639,8 +2639,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
 			_("size specified for non-existent log subvolume\n"));
 		usage();
 	} else if (loginternal && logsize && logblocks >= dblocks) {
-		fprintf(stderr, _("size %lld too large for internal log\n"),
-			(long long)logblocks);
+		fprintf(stderr, _("size %lu too large for internal log\n"),
+			logblocks);
 		usage();
 	} else if (!loginternal && !xi.logdev) {
 		logblocks = 0;
@@ -2717,16 +2717,16 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
 		}
 		if (logblocks > agsize - libxfs_prealloc_blocks(mp)) {
 			fprintf(stderr,
-	_("internal log size %lld too large, must fit in allocation group\n"),
-				(long long)logblocks);
+	_("internal log size %lu too large, must fit in allocation group\n"),
+				logblocks);
 			usage();
 		}
 
 		if (laflag) {
 			if (logagno >= agcount) {
 				fprintf(stderr,
-		_("log ag number %d too large, must be less than %lld\n"),
-					logagno, (long long)agcount);
+		_("log ag number %d too large, must be less than %lu\n"),
+					logagno, agcount);
 				usage();
 			}
 		} else
@@ -2755,29 +2755,29 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
 
 	if (!qflag || Nflag) {
 		printf(_(
-		   "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
+		   "meta-data=%-22s isize=%-6lu agcount=%lu, agsize=%lu blks\n"
 		   "         =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
 		   "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
-		   "data     =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
+		   "data     =%-22s bsize=%-6lu blocks=%lu, imaxpct=%lu\n"
 		   "         =%-22s sunit=%-6lu swidth=%lu blks\n"
 		   "naming   =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
-		   "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
+		   "log      =%-22s bsize=%-6d blocks=%lu, version=%d\n"
 		   "         =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
-		   "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
-			dfile, isize, (long long)agcount, (long long)agsize,
+		   "realtime =%-22s extsz=%-6d blocks=%lu, rtextents=%lu\n"),
+			dfile, isize, agcount, agsize,
 			"", sectorsize, sb_feat.attr_version,
 				    !sb_feat.projid16bit,
 			"", sb_feat.crcs_enabled, sb_feat.finobt, sb_feat.spinodes,
 			sb_feat.rmapbt, sb_feat.reflink,
-			"", blocksize, (long long)dblocks, imaxpct,
+			"", blocksize, dblocks, imaxpct,
 			"", dsunit, dswidth,
 			sb_feat.dir_version, dirblocksize, sb_feat.nci,
 				sb_feat.dirftype,
-			logfile, 1 << blocklog, (long long)logblocks,
+			logfile, 1 << blocklog, logblocks,
 			sb_feat.log_version, "", lsectorsize, lsunit,
 				sb_feat.lazy_sb_counters,
 			rtfile, rtextblocks << blocklog,
-			(long long)rtblocks, (long long)rtextents);
+			rtblocks, rtextents);
 		if (Nflag)
 			exit(0);
 	}
-- 
2.12.1

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Jan Tulak]

On Thu, Apr 6, 2017 at 4:41 PM, Jan Tulak <jtulak@redhat.com> wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
>
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
>
> This patch changes variables declared at the beginning of main() + block/sectorsize, making only minimal changes. The following patch cleans some now-unnecessary type casts.
>
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 92 insertions(+), 92 deletions(-)
>

Public git tree for these changes:
https://github.com/jtulak/xfsprogs-dev/tree/github-uint

(It includes the other two small patches I submitted today too).

Jan

-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Darrick J. Wong]

On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
> 
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
> 
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
> 
> This patch changes variables declared at the beginning of main() +
> block/sectorsize, making only minimal changes. The following patch cleans
> some now-unnecessary type casts.
> 
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 92 insertions(+), 92 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5aac4d1b..71382f70 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -39,8 +39,8 @@ static int  ispow2(unsigned int i);
>   * The configured block and sector sizes are defined as global variables so
>   * that they don't need to be passed to functions that require them.
>   */
> -unsigned int		blocksize;
> -unsigned int		sectorsize;
> +__uint64_t		blocksize;
> +__uint64_t		sectorsize;
>  
>  #define MAX_SUBOPTS	16
>  #define SUBOPT_NEEDS_VAL	(-1LL)
> @@ -742,9 +742,9 @@ calc_stripe_factors(
>  	int		dsectsz,
>  	int		lsu,
>  	int		lsectsz,
> -	int		*dsunit,
> -	int		*dswidth,
> -	int		*lsunit)
> +	__uint64_t	*dsunit,
> +	__uint64_t	*dswidth,
> +	__uint64_t	*lsunit)

My, what big raid stripes you have! ;)

>  {
>  	/* Handle data sunit/swidth options */
>  	if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
> @@ -769,32 +769,32 @@ calc_stripe_factors(
>  			usage();
>  		}
>  
> -		*dsunit  = (int)BTOBBT(dsu);
> +		*dsunit  = BTOBBT(dsu);
>  		*dswidth = *dsunit * dsw;
>  	}
>  
>  	if (*dsunit && (*dswidth % *dsunit != 0)) {
>  		fprintf(stderr,
> -			_("data stripe width (%d) must be a multiple of the "
> -			"data stripe unit (%d)\n"), *dswidth, *dsunit);
> +			_("data stripe width (%lu) must be a multiple of the "
> +			"data stripe unit (%lu)\n"), *dswidth, *dsunit);
>  		usage();
>  	}
>  
>  	/* Handle log sunit options */
>  
>  	if (lsu)
> -		*lsunit = (int)BTOBBT(lsu);
> +		*lsunit = BTOBBT(lsu);
>  
>  	/* verify if lsu/lsunit is a multiple block size */
>  	if (lsu % blocksize != 0) {
>  		fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%d) must be a multiple of the block size (%lu)\n"),
>  		lsu, blocksize);
>  		exit(1);
>  	}
>  	if ((BBTOB(*lsunit) % blocksize != 0)) {
>  		fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%lu) must be a multiple of the block size (%lu)\n"),
>  		BBTOB(*lsunit), blocksize);
>  		exit(1);
>  	}
> @@ -917,7 +917,7 @@ fixup_internal_log_stripe(
>  	int		sunit,
>  	xfs_rfsblock_t	*logblocks,
>  	int		blocklog,
> -	int		*lalign)
> +	__uint64_t	*lalign)
>  {
>  	if ((logstart % sunit) != 0) {
>  		logstart = ((logstart + (sunit - 1))/sunit) * sunit;
> @@ -1405,70 +1405,70 @@ main(
>  	__uint64_t		agsize;
>  	xfs_alloc_rec_t		*arec;
>  	struct xfs_btree_block	*block;
> -	int			blflag;
> -	int			blocklog;
> -	int			bsflag;
> -	int			bsize;
> +	__uint64_t		blflag;

Why do we need 64 bits to store a boolean flag?  Shouldn't bool (or just
leaving the flags as int) be enough?

> +	__uint64_t		blocklog;
> +	__uint64_t		bsflag;
> +	__uint64_t		bsize;

I'm wondering why __uint64_t instead of uint64_t?

>  	xfs_buf_t		*buf;
> -	int			c;
> -	int			daflag;
> -	int			dasize;
> +	__uint64_t		c;
> +	__uint64_t		daflag;
> +	__uint64_t		dasize;
>  	xfs_rfsblock_t		dblocks;
>  	char			*dfile;
> -	int			dirblocklog;
> -	int			dirblocksize;
> +	__uint64_t		dirblocklog;
> +	__uint64_t		dirblocksize;
>  	char			*dsize;
> -	int			dsu;
> -	int			dsw;
> -	int			dsunit;
> -	int			dswidth;
> -	int			force_overwrite;
> +	__uint64_t		dsu;
> +	__uint64_t		dsw;
> +	__uint64_t		dsunit;
> +	__uint64_t		dswidth;
> +	__uint64_t		force_overwrite;
>  	struct fsxattr		fsx;
> -	int			ilflag;
> -	int			imaxpct;
> -	int			imflag;
> -	int			inodelog;
> -	int			inopblock;
> -	int			ipflag;
> -	int			isflag;
> -	int			isize;
> +	__uint64_t		ilflag;
> +	__uint64_t		imaxpct;
> +	__uint64_t		imflag;
> +	__uint64_t		inodelog;
> +	__uint64_t		inopblock;
> +	__uint64_t		ipflag;
> +	__uint64_t		isflag;
> +	__uint64_t		isize;
>  	char			*label = NULL;
> -	int			laflag;
> -	int			lalign;
> -	int			ldflag;
> -	int			liflag;
> +	__uint64_t		laflag;
> +	__uint64_t		lalign;
> +	__uint64_t		ldflag;
> +	__uint64_t		liflag;
>  	xfs_agnumber_t		logagno;
>  	xfs_rfsblock_t		logblocks;
>  	char			*logfile;
> -	int			loginternal;
> +	__uint64_t		loginternal;
>  	char			*logsize;
>  	xfs_fsblock_t		logstart;
> -	int			lvflag;
> -	int			lsflag;
> -	int			lsuflag;
> -	int			lsunitflag;
> -	int			lsectorlog;
> -	int			lsectorsize;
> -	int			lslflag;
> -	int			lssflag;
> -	int			lsu;
> -	int			lsunit;
> -	int			min_logblocks;
> +	__uint64_t		lvflag;
> +	__uint64_t		lsflag;
> +	__uint64_t		lsuflag;
> +	__uint64_t		lsunitflag;
> +	__uint64_t		lsectorlog;
> +	__uint64_t		lsectorsize;
> +	__uint64_t		lslflag;
> +	__uint64_t		lssflag;
> +	__uint64_t		lsu;
> +	__uint64_t		lsunit;
> +	__uint64_t		min_logblocks;
>  	xfs_mount_t		*mp;
>  	xfs_mount_t		mbuf;
>  	xfs_extlen_t		nbmblocks;
> -	int			nlflag;
> -	int			nodsflag;
> -	int			norsflag;
> +	__uint64_t		nlflag;
> +	__uint64_t		nodsflag;
> +	__uint64_t		norsflag;
>  	xfs_alloc_rec_t		*nrec;
> -	int			nsflag;
> -	int			nvflag;
> -	int			Nflag;
> -	int			discard = 1;
> +	__uint64_t		nsflag;
> +	__uint64_t		nvflag;
> +	__uint64_t		Nflag;
> +	__uint64_t		discard = 1;
>  	char			*p;
>  	char			*protofile;
>  	char			*protostring;
> -	int			qflag;
> +	__uint64_t		qflag;
>  	xfs_rfsblock_t		rtblocks;
>  	xfs_extlen_t		rtextblocks;
>  	xfs_rtblock_t		rtextents;
> @@ -1476,13 +1476,13 @@ main(
>  	char			*rtfile;
>  	char			*rtsize;
>  	xfs_sb_t		*sbp;
> -	int			sectorlog;
> +	__uint64_t		sectorlog;
>  	__uint64_t		sector_mask;
> -	int			slflag;
> -	int			ssflag;
> +	__uint64_t		slflag;
> +	__uint64_t		ssflag;
>  	__uint64_t		tmp_agsize;
>  	uuid_t			uuid;
> -	int			worst_freelist;
> +	__uint64_t		worst_freelist;
>  	libxfs_init_t		xi;
>  	struct fs_topology	ft;
>  	struct sb_feat_args	sb_feat = {
> @@ -1946,7 +1946,7 @@ main(
>  		blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
>  	}
>  	if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
> -		fprintf(stderr, _("illegal block size %d\n"), blocksize);
> +		fprintf(stderr, _("illegal block size %lu\n"), blocksize);

"illegal block size %"PRIu64"\n", because uint64_t can be unsigned long
long on 32bit.

--D

>  		usage();
>  	}
>  	if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> @@ -2010,7 +2010,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>  
>  		if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {
>  			fprintf(stderr,
> -_("specified blocksize %d is less than device physical sector size %d\n"),
> +_("specified blocksize %lu is less than device physical sector size %d\n"),
>  				blocksize, ft.psectorsize);
>  			fprintf(stderr,
>  _("switching to logical sector size %d\n"),
> @@ -2031,21 +2031,21 @@ _("switching to logical sector size %d\n"),
>  	if (sectorsize < XFS_MIN_SECTORSIZE ||
>  	    sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
>  		if (ssflag)
> -			fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> +			fprintf(stderr, _("illegal sector size %lu\n"), sectorsize);
>  		else
>  			fprintf(stderr,
> -_("block size %d cannot be smaller than logical sector size %d\n"),
> +_("block size %lu cannot be smaller than logical sector size %d\n"),
>  				blocksize, ft.lsectorsize);
>  		usage();
>  	}
>  	if (sectorsize < ft.lsectorsize) {
> -		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> +		fprintf(stderr, _("illegal sector size %lu; hw sector is %d\n"),
>  			sectorsize, ft.lsectorsize);
>  		usage();
>  	}
>  	if (lsectorsize < XFS_MIN_SECTORSIZE ||
>  	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
> -		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> +		fprintf(stderr, _("illegal log sector size %lu\n"), lsectorsize);
>  		usage();
>  	} else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>  		lsu = blocksize;
> @@ -2151,7 +2151,7 @@ _("rmapbt not supported with realtime devices\n"));
>  	if (nsflag || nlflag) {
>  		if (dirblocksize < blocksize ||
>  					dirblocksize > XFS_MAX_BLOCKSIZE) {
> -			fprintf(stderr, _("illegal directory block size %d\n"),
> +			fprintf(stderr, _("illegal directory block size %lu\n"),
>  				dirblocksize);
>  			usage();
>  		}
> @@ -2177,7 +2177,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
>  		if (dbytes % blocksize)
>  			fprintf(stderr, _("warning: "
> -	"data length %lld not a multiple of %d, truncated to %lld\n"),
> +	"data length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)dbytes, blocksize,
>  				(long long)(dblocks << blocklog));
>  	}
> @@ -2209,7 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
>  		if (logbytes % blocksize)
>  			fprintf(stderr,
> -	_("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
> +	_("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)logbytes, blocksize,
>  				(long long)(logblocks << blocklog));
>  	}
> @@ -2226,7 +2226,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
>  		if (rtbytes % blocksize)
>  			fprintf(stderr,
> -	_("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
> +	_("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)rtbytes, blocksize,
>  				(long long)(rtblocks << blocklog));
>  	}
> @@ -2239,7 +2239,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
>  		if (rtextbytes % blocksize) {
>  			fprintf(stderr,
> -		_("illegal rt extent size %lld, not a multiple of %d\n"),
> +		_("illegal rt extent size %lld, not a multiple of %lu\n"),
>  				(long long)rtextbytes, blocksize);
>  			usage();
>  		}
> @@ -2282,16 +2282,16 @@ _("rmapbt not supported with realtime devices\n"));
>  	    isize > XFS_DINODE_MAX_SIZE) {
>  		int	maxsz;
>  
> -		fprintf(stderr, _("illegal inode size %d\n"), isize);
> +		fprintf(stderr, _("illegal inode size %lu\n"), isize);
>  		maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
>  			    XFS_DINODE_MAX_SIZE);
>  		if (XFS_DINODE_MIN_SIZE == maxsz)
>  			fprintf(stderr,
> -			_("allowable inode size with %d byte blocks is %d\n"),
> +			_("allowable inode size with %lu byte blocks is %d\n"),
>  				blocksize, XFS_DINODE_MIN_SIZE);
>  		else
>  			fprintf(stderr,
> -	_("allowable inode size with %d byte blocks is between %d and %d\n"),
> +	_("allowable inode size with %lu byte blocks is between %d and %d\n"),
>  				blocksize, XFS_DINODE_MIN_SIZE, maxsz);
>  		exit(1);
>  	}
> @@ -2395,19 +2395,19 @@ _("rmapbt not supported with realtime devices\n"));
>  
>  	if (xi.dbsize > sectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the data subvolume sector size %u is less than the sector size \n\
> +"Warning: the data subvolume sector size %lu is less than the sector size \n\
>  reported by the device (%u).\n"),
>  			sectorsize, xi.dbsize);
>  	}
>  	if (!loginternal && xi.lbsize > lsectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the log subvolume sector size %u is less than the sector size\n\
> +"Warning: the log subvolume sector size %lu is less than the sector size\n\
>  reported by the device (%u).\n"),
>  			lsectorsize, xi.lbsize);
>  	}
>  	if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the realtime subvolume sector size %u is less than the sector size\n\
> +"Warning: the realtime subvolume sector size %lu is less than the sector size\n\
>  reported by the device (%u).\n"),
>  			sectorsize, xi.rtbsize);
>  	}
> @@ -2437,14 +2437,14 @@ reported by the device (%u).\n"),
>  		if (dsunit) {
>  			if (ft.dsunit && ft.dsunit != dsunit) {
>  				fprintf(stderr,
> -					_("%s: Specified data stripe unit %d "
> +					_("%s: Specified data stripe unit %lu "
>  					"is not the same as the volume stripe "
>  					"unit %d\n"),
>  					progname, dsunit, ft.dsunit);
>  			}
>  			if (ft.dswidth && ft.dswidth != dswidth) {
>  				fprintf(stderr,
> -					_("%s: Specified data stripe width %d "
> +					_("%s: Specified data stripe width %lu "
>  					"is not the same as the volume stripe "
>  					"width %d\n"),
>  					progname, dswidth, ft.dswidth);
> @@ -2462,7 +2462,7 @@ reported by the device (%u).\n"),
>  		 */
>  		if (agsize % blocksize) {
>  			fprintf(stderr,
> -		_("agsize (%lld) not a multiple of fs blk size (%d)\n"),
> +		_("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
>  				(long long)agsize, blocksize);
>  			usage();
>  		}
> @@ -2512,7 +2512,7 @@ reported by the device (%u).\n"),
>  						(dblocks % agsize != 0);
>  				if (dasize)
>  					fprintf(stderr,
> -				_("agsize rounded to %lld, swidth = %d\n"),
> +				_("agsize rounded to %lld, swidth = %lu\n"),
>  						(long long)agsize, dswidth);
>  			} else {
>  				if (nodsflag) {
> @@ -2569,8 +2569,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  			dsunit = dswidth = 0;
>  		else {
>  			fprintf(stderr,
> -				_("%s: Stripe unit(%d) or stripe width(%d) is "
> -				"not a multiple of the block size(%d)\n"),
> +				_("%s: Stripe unit(%lu) or stripe width(%lu) is "
> +				"not a multiple of the block size(%lu)\n"),
>  				progname, BBTOB(dsunit), BBTOB(dswidth),
>  				blocksize);
>  			exit(1);
> @@ -2610,7 +2610,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  		/* Warn only if specified on commandline */
>  		if (lsuflag || lsunitflag) {
>  			fprintf(stderr,
> -	_("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
> +	_("log stripe unit (%lu bytes) is too large (maximum is 256KiB)\n"),
>  				(lsunit * blocksize));
>  			fprintf(stderr,
>  	_("log stripe unit adjusted to 32KiB\n"));
> @@ -2755,14 +2755,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
>  
>  	if (!qflag || Nflag) {
>  		printf(_(
> -		   "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> -		   "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> +		   "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
> +		   "         =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
>  		   "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> -		   "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
> -		   "         =%-22s sunit=%-6u swidth=%u blks\n"
> -		   "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> +		   "data     =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
> +		   "         =%-22s sunit=%-6lu swidth=%lu blks\n"
> +		   "naming   =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
>  		   "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> -		   "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> +		   "         =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
>  		   "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
>  			dfile, isize, (long long)agcount, (long long)agsize,
>  			"", sectorsize, sb_feat.attr_version,
> -- 
> 2.12.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Eric Sandeen]

On 4/6/17 9:41 AM, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
> 
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
> 
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
> 
> This patch changes variables declared at the beginning of main() +
> block/sectorsize, making only minimal changes. The following patch
> cleans some now-unnecessary type casts.
> 
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.

This changelog doesn't really say why we'd want to make this change
in all of the variables you've modified.

For example, you're taking simple flags like qflag - which sure does look
like it should be a /bool/ - and making it 64 bits:


-	int			qflag;
+	__uint64_t		qflag;

why?

I don't understand what is improved by this sort of thing.  It is only
used as a simple flag, even after your other work, as far as I can tell.
There is no casting, no conversion, nothing ... Same goes for many other
variables changed by this patch.

If choosing proper types avoids casts back and forth in some places,
sure, that makes sense - pick the proper type, or the superset type
as needed for conversions -  but "consistency" is not a reason to turn
boolean flags into 64-bit variables, as far as I can tell...

Also, as Darrick mentioned I'm not sure why we'd use the __uint64_t
type instead of uint64_t in this code?

And, you've changed printf formats for these to %lu, but this will go all
to hell on a 32-bit build.  As Darrick mentioned, you'll need the funky
PRIu64 formatter stuff if you want to go this way.

I know you & Luis discussed this, but I'm just not getting it; "consistency"
alone is not a reason to make every variable 64 bits, IMHO.  If it serves
some greater purpose, please make that purpose more clear in the changelog...

-Eric


> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 92 insertions(+), 92 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5aac4d1b..71382f70 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -39,8 +39,8 @@ static int  ispow2(unsigned int i);
>   * The configured block and sector sizes are defined as global variables so
>   * that they don't need to be passed to functions that require them.
>   */
> -unsigned int		blocksize;
> -unsigned int		sectorsize;
> +__uint64_t		blocksize;
> +__uint64_t		sectorsize;
>  
>  #define MAX_SUBOPTS	16
>  #define SUBOPT_NEEDS_VAL	(-1LL)
> @@ -742,9 +742,9 @@ calc_stripe_factors(
>  	int		dsectsz,
>  	int		lsu,
>  	int		lsectsz,
> -	int		*dsunit,
> -	int		*dswidth,
> -	int		*lsunit)
> +	__uint64_t	*dsunit,
> +	__uint64_t	*dswidth,
> +	__uint64_t	*lsunit)
>  {
>  	/* Handle data sunit/swidth options */
>  	if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
> @@ -769,32 +769,32 @@ calc_stripe_factors(
>  			usage();
>  		}
>  
> -		*dsunit  = (int)BTOBBT(dsu);
> +		*dsunit  = BTOBBT(dsu);
>  		*dswidth = *dsunit * dsw;
>  	}
>  
>  	if (*dsunit && (*dswidth % *dsunit != 0)) {
>  		fprintf(stderr,
> -			_("data stripe width (%d) must be a multiple of the "
> -			"data stripe unit (%d)\n"), *dswidth, *dsunit);
> +			_("data stripe width (%lu) must be a multiple of the "
> +			"data stripe unit (%lu)\n"), *dswidth, *dsunit);
>  		usage();
>  	}
>  
>  	/* Handle log sunit options */
>  
>  	if (lsu)
> -		*lsunit = (int)BTOBBT(lsu);
> +		*lsunit = BTOBBT(lsu);
>  
>  	/* verify if lsu/lsunit is a multiple block size */
>  	if (lsu % blocksize != 0) {
>  		fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%d) must be a multiple of the block size (%lu)\n"),
>  		lsu, blocksize);
>  		exit(1);
>  	}
>  	if ((BBTOB(*lsunit) % blocksize != 0)) {
>  		fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%lu) must be a multiple of the block size (%lu)\n"),
>  		BBTOB(*lsunit), blocksize);
>  		exit(1);
>  	}
> @@ -917,7 +917,7 @@ fixup_internal_log_stripe(
>  	int		sunit,
>  	xfs_rfsblock_t	*logblocks,
>  	int		blocklog,
> -	int		*lalign)
> +	__uint64_t	*lalign)
>  {
>  	if ((logstart % sunit) != 0) {
>  		logstart = ((logstart + (sunit - 1))/sunit) * sunit;
> @@ -1405,70 +1405,70 @@ main(
>  	__uint64_t		agsize;
>  	xfs_alloc_rec_t		*arec;
>  	struct xfs_btree_block	*block;
> -	int			blflag;
> -	int			blocklog;
> -	int			bsflag;
> -	int			bsize;
> +	__uint64_t		blflag;
> +	__uint64_t		blocklog;
> +	__uint64_t		bsflag;
> +	__uint64_t		bsize;
>  	xfs_buf_t		*buf;
> -	int			c;
> -	int			daflag;
> -	int			dasize;
> +	__uint64_t		c;
> +	__uint64_t		daflag;
> +	__uint64_t		dasize;
>  	xfs_rfsblock_t		dblocks;
>  	char			*dfile;
> -	int			dirblocklog;
> -	int			dirblocksize;
> +	__uint64_t		dirblocklog;
> +	__uint64_t		dirblocksize;
>  	char			*dsize;
> -	int			dsu;
> -	int			dsw;
> -	int			dsunit;
> -	int			dswidth;
> -	int			force_overwrite;
> +	__uint64_t		dsu;
> +	__uint64_t		dsw;
> +	__uint64_t		dsunit;
> +	__uint64_t		dswidth;
> +	__uint64_t		force_overwrite;
>  	struct fsxattr		fsx;
> -	int			ilflag;
> -	int			imaxpct;
> -	int			imflag;
> -	int			inodelog;
> -	int			inopblock;
> -	int			ipflag;
> -	int			isflag;
> -	int			isize;
> +	__uint64_t		ilflag;
> +	__uint64_t		imaxpct;
> +	__uint64_t		imflag;
> +	__uint64_t		inodelog;
> +	__uint64_t		inopblock;
> +	__uint64_t		ipflag;
> +	__uint64_t		isflag;
> +	__uint64_t		isize;
>  	char			*label = NULL;
> -	int			laflag;
> -	int			lalign;
> -	int			ldflag;
> -	int			liflag;
> +	__uint64_t		laflag;
> +	__uint64_t		lalign;
> +	__uint64_t		ldflag;
> +	__uint64_t		liflag;
>  	xfs_agnumber_t		logagno;
>  	xfs_rfsblock_t		logblocks;
>  	char			*logfile;
> -	int			loginternal;
> +	__uint64_t		loginternal;
>  	char			*logsize;
>  	xfs_fsblock_t		logstart;
> -	int			lvflag;
> -	int			lsflag;
> -	int			lsuflag;
> -	int			lsunitflag;
> -	int			lsectorlog;
> -	int			lsectorsize;
> -	int			lslflag;
> -	int			lssflag;
> -	int			lsu;
> -	int			lsunit;
> -	int			min_logblocks;
> +	__uint64_t		lvflag;
> +	__uint64_t		lsflag;
> +	__uint64_t		lsuflag;
> +	__uint64_t		lsunitflag;
> +	__uint64_t		lsectorlog;
> +	__uint64_t		lsectorsize;
> +	__uint64_t		lslflag;
> +	__uint64_t		lssflag;
> +	__uint64_t		lsu;
> +	__uint64_t		lsunit;
> +	__uint64_t		min_logblocks;
>  	xfs_mount_t		*mp;
>  	xfs_mount_t		mbuf;
>  	xfs_extlen_t		nbmblocks;
> -	int			nlflag;
> -	int			nodsflag;
> -	int			norsflag;
> +	__uint64_t		nlflag;
> +	__uint64_t		nodsflag;
> +	__uint64_t		norsflag;
>  	xfs_alloc_rec_t		*nrec;
> -	int			nsflag;
> -	int			nvflag;
> -	int			Nflag;
> -	int			discard = 1;
> +	__uint64_t		nsflag;
> +	__uint64_t		nvflag;
> +	__uint64_t		Nflag;
> +	__uint64_t		discard = 1;
>  	char			*p;
>  	char			*protofile;
>  	char			*protostring;
> -	int			qflag;
> +	__uint64_t		qflag;
>  	xfs_rfsblock_t		rtblocks;
>  	xfs_extlen_t		rtextblocks;
>  	xfs_rtblock_t		rtextents;
> @@ -1476,13 +1476,13 @@ main(
>  	char			*rtfile;
>  	char			*rtsize;
>  	xfs_sb_t		*sbp;
> -	int			sectorlog;
> +	__uint64_t		sectorlog;
>  	__uint64_t		sector_mask;
> -	int			slflag;
> -	int			ssflag;
> +	__uint64_t		slflag;
> +	__uint64_t		ssflag;
>  	__uint64_t		tmp_agsize;
>  	uuid_t			uuid;
> -	int			worst_freelist;
> +	__uint64_t		worst_freelist;
>  	libxfs_init_t		xi;
>  	struct fs_topology	ft;
>  	struct sb_feat_args	sb_feat = {
> @@ -1946,7 +1946,7 @@ main(
>  		blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
>  	}
>  	if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
> -		fprintf(stderr, _("illegal block size %d\n"), blocksize);
> +		fprintf(stderr, _("illegal block size %lu\n"), blocksize);
>  		usage();
>  	}
>  	if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> @@ -2010,7 +2010,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>  
>  		if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {
>  			fprintf(stderr,
> -_("specified blocksize %d is less than device physical sector size %d\n"),
> +_("specified blocksize %lu is less than device physical sector size %d\n"),
>  				blocksize, ft.psectorsize);
>  			fprintf(stderr,
>  _("switching to logical sector size %d\n"),
> @@ -2031,21 +2031,21 @@ _("switching to logical sector size %d\n"),
>  	if (sectorsize < XFS_MIN_SECTORSIZE ||
>  	    sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
>  		if (ssflag)
> -			fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> +			fprintf(stderr, _("illegal sector size %lu\n"), sectorsize);
>  		else
>  			fprintf(stderr,
> -_("block size %d cannot be smaller than logical sector size %d\n"),
> +_("block size %lu cannot be smaller than logical sector size %d\n"),
>  				blocksize, ft.lsectorsize);
>  		usage();
>  	}
>  	if (sectorsize < ft.lsectorsize) {
> -		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> +		fprintf(stderr, _("illegal sector size %lu; hw sector is %d\n"),
>  			sectorsize, ft.lsectorsize);
>  		usage();
>  	}
>  	if (lsectorsize < XFS_MIN_SECTORSIZE ||
>  	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
> -		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> +		fprintf(stderr, _("illegal log sector size %lu\n"), lsectorsize);
>  		usage();
>  	} else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>  		lsu = blocksize;
> @@ -2151,7 +2151,7 @@ _("rmapbt not supported with realtime devices\n"));
>  	if (nsflag || nlflag) {
>  		if (dirblocksize < blocksize ||
>  					dirblocksize > XFS_MAX_BLOCKSIZE) {
> -			fprintf(stderr, _("illegal directory block size %d\n"),
> +			fprintf(stderr, _("illegal directory block size %lu\n"),
>  				dirblocksize);
>  			usage();
>  		}
> @@ -2177,7 +2177,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
>  		if (dbytes % blocksize)
>  			fprintf(stderr, _("warning: "
> -	"data length %lld not a multiple of %d, truncated to %lld\n"),
> +	"data length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)dbytes, blocksize,
>  				(long long)(dblocks << blocklog));
>  	}
> @@ -2209,7 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
>  		if (logbytes % blocksize)
>  			fprintf(stderr,
> -	_("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
> +	_("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)logbytes, blocksize,
>  				(long long)(logblocks << blocklog));
>  	}
> @@ -2226,7 +2226,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
>  		if (rtbytes % blocksize)
>  			fprintf(stderr,
> -	_("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
> +	_("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
>  				(long long)rtbytes, blocksize,
>  				(long long)(rtblocks << blocklog));
>  	}
> @@ -2239,7 +2239,7 @@ _("rmapbt not supported with realtime devices\n"));
>  		rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
>  		if (rtextbytes % blocksize) {
>  			fprintf(stderr,
> -		_("illegal rt extent size %lld, not a multiple of %d\n"),
> +		_("illegal rt extent size %lld, not a multiple of %lu\n"),
>  				(long long)rtextbytes, blocksize);
>  			usage();
>  		}
> @@ -2282,16 +2282,16 @@ _("rmapbt not supported with realtime devices\n"));
>  	    isize > XFS_DINODE_MAX_SIZE) {
>  		int	maxsz;
>  
> -		fprintf(stderr, _("illegal inode size %d\n"), isize);
> +		fprintf(stderr, _("illegal inode size %lu\n"), isize);
>  		maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
>  			    XFS_DINODE_MAX_SIZE);
>  		if (XFS_DINODE_MIN_SIZE == maxsz)
>  			fprintf(stderr,
> -			_("allowable inode size with %d byte blocks is %d\n"),
> +			_("allowable inode size with %lu byte blocks is %d\n"),
>  				blocksize, XFS_DINODE_MIN_SIZE);
>  		else
>  			fprintf(stderr,
> -	_("allowable inode size with %d byte blocks is between %d and %d\n"),
> +	_("allowable inode size with %lu byte blocks is between %d and %d\n"),
>  				blocksize, XFS_DINODE_MIN_SIZE, maxsz);
>  		exit(1);
>  	}
> @@ -2395,19 +2395,19 @@ _("rmapbt not supported with realtime devices\n"));
>  
>  	if (xi.dbsize > sectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the data subvolume sector size %u is less than the sector size \n\
> +"Warning: the data subvolume sector size %lu is less than the sector size \n\
>  reported by the device (%u).\n"),
>  			sectorsize, xi.dbsize);
>  	}
>  	if (!loginternal && xi.lbsize > lsectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the log subvolume sector size %u is less than the sector size\n\
> +"Warning: the log subvolume sector size %lu is less than the sector size\n\
>  reported by the device (%u).\n"),
>  			lsectorsize, xi.lbsize);
>  	}
>  	if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
>  		fprintf(stderr, _(
> -"Warning: the realtime subvolume sector size %u is less than the sector size\n\
> +"Warning: the realtime subvolume sector size %lu is less than the sector size\n\
>  reported by the device (%u).\n"),
>  			sectorsize, xi.rtbsize);
>  	}
> @@ -2437,14 +2437,14 @@ reported by the device (%u).\n"),
>  		if (dsunit) {
>  			if (ft.dsunit && ft.dsunit != dsunit) {
>  				fprintf(stderr,
> -					_("%s: Specified data stripe unit %d "
> +					_("%s: Specified data stripe unit %lu "
>  					"is not the same as the volume stripe "
>  					"unit %d\n"),
>  					progname, dsunit, ft.dsunit);
>  			}
>  			if (ft.dswidth && ft.dswidth != dswidth) {
>  				fprintf(stderr,
> -					_("%s: Specified data stripe width %d "
> +					_("%s: Specified data stripe width %lu "
>  					"is not the same as the volume stripe "
>  					"width %d\n"),
>  					progname, dswidth, ft.dswidth);
> @@ -2462,7 +2462,7 @@ reported by the device (%u).\n"),
>  		 */
>  		if (agsize % blocksize) {
>  			fprintf(stderr,
> -		_("agsize (%lld) not a multiple of fs blk size (%d)\n"),
> +		_("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
>  				(long long)agsize, blocksize);
>  			usage();
>  		}
> @@ -2512,7 +2512,7 @@ reported by the device (%u).\n"),
>  						(dblocks % agsize != 0);
>  				if (dasize)
>  					fprintf(stderr,
> -				_("agsize rounded to %lld, swidth = %d\n"),
> +				_("agsize rounded to %lld, swidth = %lu\n"),
>  						(long long)agsize, dswidth);
>  			} else {
>  				if (nodsflag) {
> @@ -2569,8 +2569,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  			dsunit = dswidth = 0;
>  		else {
>  			fprintf(stderr,
> -				_("%s: Stripe unit(%d) or stripe width(%d) is "
> -				"not a multiple of the block size(%d)\n"),
> +				_("%s: Stripe unit(%lu) or stripe width(%lu) is "
> +				"not a multiple of the block size(%lu)\n"),
>  				progname, BBTOB(dsunit), BBTOB(dswidth),
>  				blocksize);
>  			exit(1);
> @@ -2610,7 +2610,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  		/* Warn only if specified on commandline */
>  		if (lsuflag || lsunitflag) {
>  			fprintf(stderr,
> -	_("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
> +	_("log stripe unit (%lu bytes) is too large (maximum is 256KiB)\n"),
>  				(lsunit * blocksize));
>  			fprintf(stderr,
>  	_("log stripe unit adjusted to 32KiB\n"));
> @@ -2755,14 +2755,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
>  
>  	if (!qflag || Nflag) {
>  		printf(_(
> -		   "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> -		   "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> +		   "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
> +		   "         =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
>  		   "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> -		   "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
> -		   "         =%-22s sunit=%-6u swidth=%u blks\n"
> -		   "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> +		   "data     =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
> +		   "         =%-22s sunit=%-6lu swidth=%lu blks\n"
> +		   "naming   =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
>  		   "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> -		   "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> +		   "         =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
>  		   "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
>  			dfile, isize, (long long)agcount, (long long)agsize,
>  			"", sectorsize, sb_feat.attr_version,
> 

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Dave Chinner]

On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
> 
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
> 
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.

I'm with Darrick and Eric on this - it's not the right conversion to
make for all the reasons they've pointed out. Further, I think it's
the wrong direction to be working in.

What I originally intended the config option table to be used for
was to /replace all these config variables/ with config option table
lookups. We don't need tens of variables to say we certain options
set - once option parsing is complete we can just lookup the config
table and use the option value directly. i.e.  we need to work
towards removing all the variables, not try to make them pretty....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Jan Tulak]

On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>>
>> In the past, when mkfs was first written, it used atoi and
>> similar calls, so the variables were ints. However, the situation moved
>> since then and in course of the time, mkfs began to use other types too.
>>
>> Clean and unify it. We don't need negative values anywhere in the code and
>> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>> type.
>
> I'm with Darrick and Eric on this - it's not the right conversion to
> make for all the reasons they've pointed out. Further, I think it's
> the wrong direction to be working in.
>
> What I originally intended the config option table to be used for
> was to /replace all these config variables/ with config option table
> lookups. We don't need tens of variables to say we certain options
> set - once option parsing is complete we can just lookup the config
> table and use the option value directly. i.e.  we need to work
> towards removing all the variables, not try to make them pretty....
>

Removing them entirely is not as easy... Right now, there is this
thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
opts struct values" in the main:

(Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)

-       __uint64_t              agcount;
+       __uint64_t              *agcount;
...

+       /*
+        * Set up pointers, so we can use shorter names and to let gcc
+        * check the correct type. We don't want to inadvertently use an int as
+        * unsigned int and so on...
+        */
+       agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
...

The comment states, why the variables are still here and with this
code, it still makes sense to make (at least for some variables) the
transformation. I tried to come up with another way how to make a
short and type-safe access to the options without the variables, but
so far, I didn't got anywhere.

Clearly, we can't use a macro, because we need to access the value of
a member of a struct to decide what type the option is. The only thing
I could come up that seems remotely working is to make a set of
functions like these for every type:

int get_opt_int(opt, sub)
int set_opt_int(opt, sub, number)


It would require us to always remember what suffix we have to use and
it is not possible to do direct assignments with "="(that is why I
transformed the variables into pointers in the patchset). But at least
it could be possible to catch an incorrect type use easily, something
we couldn't do in the macro:

int get_opt_int(opt, sub)
{
  if (opts[opt].subopt_params[sub].type != INT)
       print an error and exit();
  return opts[opt].subopt_params[sub].value.int;
}

If you find this better than the pointers, I can change it and then
this unifying patch is rather useless. But if not, then it makes sense
to do something with the types now, rather than in the linked patch.
Ultimately, if we decide to have only one numeric type, then the type
safety is not much of an issue. Because you can't get it wrong and one
macro for a number and one for a string would suffice then.


So to the other points from Eric and Darrick:

>> -     int             *dsunit,
>> -     int             *dswidth,
>> -     int             *lsunit)
>> +     __uint64_t      *dsunit,
>> +     __uint64_t      *dswidth,
>> +     __uint64_t      *lsunit)
>
> My, what big raid stripes you have! ;)

Well, yes, 64 bits is not necessary here, but I would say that having
just one size of uint removes some (even if small) ambiguity, while
performance-wise, it won't do anything noticeable.


> For example, you're taking simple flags like qflag - which sure does look
> like it should be a /bool/ - and making it 64 bits:

and

>> -     int                     bsflag;
>> -     int                     bsize;
>> +     __uint64_t              blflag;
>
> Why do we need 64 bits to store a boolean flag?  Shouldn't bool (or just
> leaving the flags as int) be enough?

The same as above. I admit it is wasting of space, though, and in case
of booleans it  make more sense to turn them into a proper bool.

>
>> +     __uint64_t              blocklog;
>> +     __uint64_t              bsflag;
>> +     __uint64_t              bsize;
>
> I'm wondering why __uint64_t instead of uint64_t?

Because __uint64_t was already used for some numbers (e.g. agcount),
while uint64_t not, and it is defined in xfs_linux.h. So I thought
that it is safer to use xfs's own type, if it exists for some reason.

>> -             fprintf(stderr, _("illegal block size %d\n"), blocksize);
>> +             fprintf(stderr, _("illegal block size %lu\n"), blocksize);
>
> "illegal block size %"PRIu64"\n", because uint64_t can be unsigned long
> long on 32bit.

Mmm, ok, I'm making a note to have a 32bit VM at hand and try to at
least compile things there too. Then I would get a warning that would
force me to find the PRIu64 alternative...


> This changelog doesn't really say why we'd want to make this change
> in all of the variables you've modified.

I thought this as also a way to simplify the types in the opts
structure, ideally to just one numeric type and a string. So this is
in part how I picked most of the variables. And I added the rest
thinking: "Well, I'm changing most of them, so why keep the last few
ints in there? Let's convert them too."
But I now, after reading your comments, I think that adding a boolean
into the mix makes sense. Using 64 bits even where we won't need so
big numbers is not an issue, I would still use the 64 bits there, but
the boolean can be bool instead.

I hope I didn't forget anything and that it is understandable...

Jan

-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Dave Chinner]

On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
> >>
> >> In the past, when mkfs was first written, it used atoi and
> >> similar calls, so the variables were ints. However, the situation moved
> >> since then and in course of the time, mkfs began to use other types too.
> >>
> >> Clean and unify it. We don't need negative values anywhere in the code and
> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> >> type.
> >
> > I'm with Darrick and Eric on this - it's not the right conversion to
> > make for all the reasons they've pointed out. Further, I think it's
> > the wrong direction to be working in.
> >
> > What I originally intended the config option table to be used for
> > was to /replace all these config variables/ with config option table
> > lookups. We don't need tens of variables to say we certain options
> > set - once option parsing is complete we can just lookup the config
> > table and use the option value directly. i.e.  we need to work
> > towards removing all the variables, not try to make them pretty....
> >
> 
> Removing them entirely is not as easy... Right now, there is this
> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
> opts struct values" in the main:
> 
> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
> 
> -       __uint64_t              agcount;
> +       __uint64_t              *agcount;
> ...
> 
> +       /*
> +        * Set up pointers, so we can use shorter names and to let gcc
> +        * check the correct type. We don't want to inadvertently use an int as
> +        * unsigned int and so on...
> +        */
> +       agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;

That's .... an interesting interpretation....

What I intended was replacing all the uses of the agcount variable
with calls like:

	get_config_val(OPT_D, D_AGCOUNT)

[....]

> transformed the variables into pointers in the patchset). But at least
> it could be possible to catch an incorrect type use easily, something
> we couldn't do in the macro:
> 
> int get_opt_int(opt, sub)
> {
>   if (opts[opt].subopt_params[sub].type != INT)
>        print an error and exit();
>   return opts[opt].subopt_params[sub].value.int;
> }

Yes, but why do we need to add type checking like this? It seems to
me that you're trying to over-engineer a simple thing that does not
need to be complex. For options with integer/flag values:

/* return default value if nothing specified on the CLI */
static inline uint64_t
get_config_val(int opt, int subopt)
{
	if (!opts[opt].subopt_params[subopt].seen)
		return opts[opt].subopt_params[subopt].defaultval;
	return opts[opt].subopt_params[subopt].value;
}

And for options that are strings (e.g. device names):

static inline char *
get_config_str(int opt, int subopt)
{
	if (!opts[opt].subopt_params[subopt].str_seen)
		return NULL;
	return opts[opt].subopt_params[subopt].string;
}

That's all that is necessary. The config table is guaranteed to
contain valid default values, and by the time we get to checking
options we've done all the conflict/validity checking so we can
trust the config settings to be valid and just use them directly
like this.

> >> +     __uint64_t      *dswidth,
> >> +     __uint64_t      *lsunit)
> >
> > My, what big raid stripes you have! ;)
> 
> Well, yes, 64 bits is not necessary here, but I would say that having
> just one size of uint removes some (even if small) ambiguity, while
> performance-wise, it won't do anything noticeable.

However, it makes me cringe when I read code written like this. You
say it removes ambiguity but to me, after more than 20 years of C
coding using appropriate types for variable contents, using uint64_t
for all variables (especially those that don't require 64 bit types)
introduces ambiguity and raises questions about the code quality.

e.g.  declaring a flag as boolean means the author intended it to
only have two values (i.e. it's self documenting then intent of a
flag value!) whereas declaring them all as uint64_t makes me wonder
why the author of this code didn't know what the valid value range
for this variable is, why none of the reviewers cared either, what
happens if I put a really large value into the field instead of 0 or
1, if that was tested, etc. Using the right type removes all this
potential ambiguity in the use/value of the variables.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Jan Tulak]

On Sun, Apr 9, 2017 at 1:59 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
>> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>> >>
>> >> In the past, when mkfs was first written, it used atoi and
>> >> similar calls, so the variables were ints. However, the situation moved
>> >> since then and in course of the time, mkfs began to use other types too.
>> >>
>> >> Clean and unify it. We don't need negative values anywhere in the code and
>> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>> >> type.
>> >
>> > I'm with Darrick and Eric on this - it's not the right conversion to
>> > make for all the reasons they've pointed out. Further, I think it's
>> > the wrong direction to be working in.
>> >
>> > What I originally intended the config option table to be used for
>> > was to /replace all these config variables/ with config option table
>> > lookups. We don't need tens of variables to say we certain options
>> > set - once option parsing is complete we can just lookup the config
>> > table and use the option value directly. i.e.  we need to work
>> > towards removing all the variables, not try to make them pretty....
>> >
>>
>> Removing them entirely is not as easy... Right now, there is this
>> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
>> opts struct values" in the main:
>>
>> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
>>
>> -       __uint64_t              agcount;
>> +       __uint64_t              *agcount;
>> ...
>>
>> +       /*
>> +        * Set up pointers, so we can use shorter names and to let gcc
>> +        * check the correct type. We don't want to inadvertently use an int as
>> +        * unsigned int and so on...
>> +        */
>> +       agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
>
> That's .... an interesting interpretation....
>
> What I intended was replacing all the uses of the agcount variable
> with calls like:
>
>         get_config_val(OPT_D, D_AGCOUNT)
>
> [....]
>
>> transformed the variables into pointers in the patchset). But at least
>> it could be possible to catch an incorrect type use easily, something
>> we couldn't do in the macro:
>>
>> int get_opt_int(opt, sub)
>> {
>>   if (opts[opt].subopt_params[sub].type != INT)
>>        print an error and exit();
>>   return opts[opt].subopt_params[sub].value.int;
>> }
>
> Yes, but why do we need to add type checking like this? It seems to
> me that you're trying to over-engineer a simple thing that does not
> need to be complex. For options with integer/flag values:
>
> /* return default value if nothing specified on the CLI */
> static inline uint64_t
> get_config_val(int opt, int subopt)
> {
>         if (!opts[opt].subopt_params[subopt].seen)
>                 return opts[opt].subopt_params[subopt].defaultval;
>         return opts[opt].subopt_params[subopt].value;
> }
>
> And for options that are strings (e.g. device names):
>
> static inline char *
> get_config_str(int opt, int subopt)
> {
>         if (!opts[opt].subopt_params[subopt].str_seen)
>                 return NULL;
>         return opts[opt].subopt_params[subopt].string;
> }
>
> That's all that is necessary. The config table is guaranteed to
> contain valid default values, and by the time we get to checking
> options we've done all the conflict/validity checking so we can
> trust the config settings to be valid and just use them directly
> like this.
>

Yes, but to be able to do this, we have to remove the other numeric
types. As long we have int, uint, various sizes... then there isn't an
overarching data type that can be used, so we either lose part of the
value range, or we need multiple functions. If we have only the
uint64, then this works.

>> >> +     __uint64_t      *dswidth,
>> >> +     __uint64_t      *lsunit)
>> >
>> > My, what big raid stripes you have! ;)
>>
>> Well, yes, 64 bits is not necessary here, but I would say that having
>> just one size of uint removes some (even if small) ambiguity, while
>> performance-wise, it won't do anything noticeable.
>
> However, it makes me cringe when I read code written like this. You
> say it removes ambiguity but to me, after more than 20 years of C
> coding using appropriate types for variable contents, using uint64_t
> for all variables (especially those that don't require 64 bit types)
> introduces ambiguity and raises questions about the code quality.
>
> e.g.  declaring a flag as boolean means the author intended it to
> only have two values (i.e. it's self documenting then intent of a
> flag value!) whereas declaring them all as uint64_t makes me wonder
> why the author of this code didn't know what the valid value range
> for this variable is, why none of the reviewers cared either, what
> happens if I put a really large value into the field instead of 0 or
> 1, if that was tested, etc. Using the right type removes all this
> potential ambiguity in the use/value of the variables.....

The boolean flag is something I acknowledged in my previous email; it
indeed makes sense to keep that as a boolean. With other types though,
that goes against the idea of a single get_config_val().

Cheers,
Jan




-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()

[Jan Tulak]

On Mon, Apr 10, 2017 at 10:42 AM, Jan Tulak <jtulak@redhat.com> wrote:
> On Sun, Apr 9, 2017 at 1:59 AM, Dave Chinner <david@fromorbit.com> wrote:
>> On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
>>> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
>>> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>>> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>>> >>
>>> >> In the past, when mkfs was first written, it used atoi and
>>> >> similar calls, so the variables were ints. However, the situation moved
>>> >> since then and in course of the time, mkfs began to use other types too.
>>> >>
>>> >> Clean and unify it. We don't need negative values anywhere in the code and
>>> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>>> >> type.
>>> >
>>> > I'm with Darrick and Eric on this - it's not the right conversion to
>>> > make for all the reasons they've pointed out. Further, I think it's
>>> > the wrong direction to be working in.
>>> >
>>> > What I originally intended the config option table to be used for
>>> > was to /replace all these config variables/ with config option table
>>> > lookups. We don't need tens of variables to say we certain options
>>> > set - once option parsing is complete we can just lookup the config
>>> > table and use the option value directly. i.e.  we need to work
>>> > towards removing all the variables, not try to make them pretty....
>>> >
>>>
>>> Removing them entirely is not as easy... Right now, there is this
>>> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
>>> opts struct values" in the main:
>>>
>>> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
>>>
>>> -       __uint64_t              agcount;
>>> +       __uint64_t              *agcount;
>>> ...
>>>
>>> +       /*
>>> +        * Set up pointers, so we can use shorter names and to let gcc
>>> +        * check the correct type. We don't want to inadvertently use an int as
>>> +        * unsigned int and so on...
>>> +        */
>>> +       agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
>>
>> That's .... an interesting interpretation....
>>
>> What I intended was replacing all the uses of the agcount variable
>> with calls like:
>>
>>         get_config_val(OPT_D, D_AGCOUNT)
>>
>> [....]
>>
>>> transformed the variables into pointers in the patchset). But at least
>>> it could be possible to catch an incorrect type use easily, something
>>> we couldn't do in the macro:
>>>
>>> int get_opt_int(opt, sub)
>>> {
>>>   if (opts[opt].subopt_params[sub].type != INT)
>>>        print an error and exit();
>>>   return opts[opt].subopt_params[sub].value.int;
>>> }
>>
>> Yes, but why do we need to add type checking like this? It seems to
>> me that you're trying to over-engineer a simple thing that does not
>> need to be complex. For options with integer/flag values:
>>
>> /* return default value if nothing specified on the CLI */
>> static inline uint64_t
>> get_config_val(int opt, int subopt)
>> {
>>         if (!opts[opt].subopt_params[subopt].seen)
>>                 return opts[opt].subopt_params[subopt].defaultval;
>>         return opts[opt].subopt_params[subopt].value;
>> }
>>
>> And for options that are strings (e.g. device names):
>>
>> static inline char *
>> get_config_str(int opt, int subopt)
>> {
>>         if (!opts[opt].subopt_params[subopt].str_seen)
>>                 return NULL;
>>         return opts[opt].subopt_params[subopt].string;
>> }
>>
>> That's all that is necessary. The config table is guaranteed to
>> contain valid default values, and by the time we get to checking
>> options we've done all the conflict/validity checking so we can
>> trust the config settings to be valid and just use them directly
>> like this.
>>
>
> Yes, but to be able to do this, we have to remove the other numeric
> types. As long we have int, uint, various sizes... then there isn't an
> overarching data type that can be used, so we either lose part of the
> value range, or we need multiple functions. If we have only the
> uint64, then this works.
>
>>> >> +     __uint64_t      *dswidth,
>>> >> +     __uint64_t      *lsunit)
>>> >
>>> > My, what big raid stripes you have! ;)
>>>
>>> Well, yes, 64 bits is not necessary here, but I would say that having
>>> just one size of uint removes some (even if small) ambiguity, while
>>> performance-wise, it won't do anything noticeable.
>>
>> However, it makes me cringe when I read code written like this. You
>> say it removes ambiguity but to me, after more than 20 years of C
>> coding using appropriate types for variable contents, using uint64_t
>> for all variables (especially those that don't require 64 bit types)
>> introduces ambiguity and raises questions about the code quality.
>>
>> e.g.  declaring a flag as boolean means the author intended it to
>> only have two values (i.e. it's self documenting then intent of a
>> flag value!) whereas declaring them all as uint64_t makes me wonder
>> why the author of this code didn't know what the valid value range
>> for this variable is, why none of the reviewers cared either, what
>> happens if I put a really large value into the field instead of 0 or
>> 1, if that was tested, etc. Using the right type removes all this
>> potential ambiguity in the use/value of the variables.....
>
> The boolean flag is something I acknowledged in my previous email; it
> indeed makes sense to keep that as a boolean. With other types though,
> that goes against the idea of a single get_config_val().

Pinging this a bit. I changed all the flags to bool - these shouldn't
be an issue now. How about the other options like dswidth, though? Is
it ok if those are kept uint64?

And I'm not sure whether to use uint64_t or __uint64_t - as I wrote
before, I picked the __uint64_t because it was already used in this
file and is defined in xfs_linux.h, but I'm open to change it if it is
a legacy type and standard uint64_t is preferred...

Thanks.

Cheers,
Jan

-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me