[PATCH 2/2] mkfs: unify validation behavior for data, log and rt dev

[Zorro Lang <zlang@kernel.org>]

The current validation logic in validate_datadev, validate_logdev,
and validate_rtdev is inconsistent and confusing when checking device
sizes, particularly when handling file images.

This patch unifies the validation flow by categorizing devices into
two distinct cases: "regular file" and "block device". Validation is
now performed separately for each case across all three subvolumes to
ensure consistent behavior.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---

Hi,

validate_datadev, validate_logdev and validate_rtdev, these three functions
handle xi->*.size, cfg->*blocks, and cli->*size inconsistently while also
juggling xi->*.isfile status. Three functions ideally have similar validation
patterns, but instead of following a template, each function has its own
custom implementation, which invites bugs, maintenance overhead and inconsistent
behavior, especially for file images.

For example, mkfs.xfs works on an empty data file with -d size=xxx:

# mkfs.xfs -f -d name=/home/emptyfile,size=300m
meta-data=/home/emptyfile        isize=512    agcount=4, agsize=19200 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=1
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
         =                       exchange=1   metadir=0
data     =                       bsize=4096   blocks=76800, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
         =                       rgcount=0    rgsize=0 extents
         =                       zoned=0      start=0 reserved=0

But for log or rt, we got below weird errors:

# mkfs.xfs -f -l logdev=/home/emptyfile,size=128m /dev/pmem1
size 128m specified for log subvolume is too large, maximum is 0 blocks
...
# mkfs.xfs -f -r rtdev=/home/emptyfile,size=128m /dev/pmem1
Invalid zero length rt subvolume found
...

One said the "size=128m" is too large, maximum is 0 (??? due to the file
size is 0). The other one ignored the "size=128m", just complained the empty
file.

Thanks,
Zorro


 mkfs/xfs_mkfs.c | 115 ++++++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 43 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 9a93330f..5a2274ed 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3839,34 +3839,37 @@ validate_datadev(
 {
 	struct libxfs_init	*xi = cli->xi;
 
-	if (!xi->data.size) {
+	if (!xi->data.isfile) {
 		/*
 		 * if the device is a file, we can't validate the size here.
 		 * Instead, the file will be truncated to the correct length
 		 * later on. if it's not a file, we've got a dud device.
 		 */
-		if (!xi->data.isfile) {
+		if (!xi->data.size) {
 			fprintf(stderr, _("can't get size of data subvolume\n"));
 			usage();
-		} else {
-			if (!cli->dsize) {
+		}
+		if (cfg->dblocks) {
+			/* check the size fits into the underlying device */
+			if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
 				fprintf(stderr,
-_("Warning: Empty file needs a data subvolume size by -d size=<value> option\n"));
+_("size %s specified for data subvolume is too large, maximum is %lld blocks\n"),
+				        cli->dsize,
+				        (long long)DTOBT(xi->data.size, cfg->blocklog));
 				usage();
 			}
+		} else {
+			/* no user size, so use the full block device */
+			cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
 		}
-	} else if (cfg->dblocks) {
-		/* check the size fits into the underlying device */
-		if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
+	} else {
+		if (!cfg->dblocks && !xi->data.size) {
 			fprintf(stderr,
-_("size %s specified for data subvolume is too large, maximum is %lld blocks\n"),
-				cli->dsize,
-				(long long)DTOBT(xi->data.size, cfg->blocklog));
+_("Warning: Empty data file needs a data subvolume size by -d size=<value> option\n"));
 			usage();
+		} else if (xi->data.size && !cfg->dblocks) {
+			cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
 		}
-	} else {
-		/* no user size, so use the full block device */
-		cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
 	}
 
 	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS(cfg)) {
@@ -3925,19 +3928,31 @@ _("log size %lld too large for internal log\n"),
 		usage();
 	}
 
-	if (!cfg->logblocks) {
-		if (xi->log.size == 0) {
+	if (!xi->log.isfile) {
+		if (!xi->log.size) {
+			fprintf(stderr, _("can't get size of log subvolume\n"));
+			usage();
+		} else if (cfg->logblocks) {
+			/* check the size fits into the underlying device */
+			if (cfg->logblocks > DTOBT(xi->log.size, cfg->blocklog)) {
+				fprintf(stderr,
+_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
+				        cli->logsize,
+				        (long long)DTOBT(xi->log.size, cfg->blocklog));
+				usage();
+			}
+		} else {
+			/* no user size, so use the full block device */
+			cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
+		}
+	} else {
+		if (!cfg->logblocks && !xi->log.size) {
 			fprintf(stderr,
-_("unable to get size of the log subvolume.\n"));
+_("Warning: Empty log file needs a log subvolume size by -l size=<value> option\n"));
 			usage();
+		} else if (xi->log.size && !cfg->logblocks) {
+			cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
 		}
-		cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
-	} else if (cfg->logblocks > DTOBT(xi->log.size, cfg->blocklog)) {
-		fprintf(stderr,
-_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
-			cli->logsize,
-			(long long)DTOBT(xi->log.size, cfg->blocklog));
-		usage();
 	}
 
 	if (xi->log.bsize > cfg->lsectorsize) {
@@ -3968,31 +3983,45 @@ _("size specified for non-existent rt subvolume\n"));
 		cfg->rtbmblocks = 0;
 		return;
 	}
-	if (!xi->rt.size) {
-		fprintf(stderr, _("Invalid zero length rt subvolume found\n"));
-		usage();
-	}
 
-	if (cli->rtsize) {
-		if (cfg->rtblocks > DTOBT(xi->rt.size, cfg->blocklog)) {
-			fprintf(stderr,
+	if (!xi->rt.isfile) {
+		if (!xi->rt.size) {
+			fprintf(stderr, _("can't get size of realtime subvolume\n"));
+			usage();
+		}
+		if (cfg->rtblocks) {
+			/* check the size fits into the underlying device */
+			if (cfg->rtblocks > DTOBT(xi->rt.size, cfg->blocklog)) {
+				fprintf(stderr,
 _("size %s specified for rt subvolume is too large, maximum is %lld blocks\n"),
-				cli->rtsize,
-				(long long)DTOBT(xi->rt.size, cfg->blocklog));
+				        cli->rtsize,
+				        (long long)DTOBT(xi->rt.size, cfg->blocklog));
+				usage();
+			}
+		} else {
+			/* no user size, so use the full block device */
+			if (zt->rt.nr_zones) {
+				cfg->rtblocks = DTOBT(zt->rt.nr_zones * zt->rt.zone_capacity,
+				                      cfg->blocklog);
+			} else {
+				cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
+			}
+		}
+	} else {
+		if (!cfg->rtblocks && !xi->rt.size) {
+			fprintf(stderr,
+_("Warning: Empty rt file needs a rt subvolume size by -r size=<value> option\n"));
 			usage();
+		} else if (xi->rt.size && !cfg->rtblocks) {
+			cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
 		}
-		if (xi->rt.bsize > cfg->sectorsize) {
-			fprintf(stderr, _(
+	}
+
+	if (xi->rt.bsize > cfg->sectorsize) {
+		fprintf(stderr, _(
 "Warning: the realtime subvolume sector size %u is less than the sector size\n\
 reported by the device (%u).\n"),
-				cfg->sectorsize, xi->rt.bsize);
-		}
-	} else if (zt->rt.nr_zones) {
-		cfg->rtblocks = DTOBT(zt->rt.nr_zones * zt->rt.zone_capacity,
-				      cfg->blocklog);
-	} else {
-		/* grab volume size */
-		cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
+		        cfg->sectorsize, xi->rt.bsize);
 	}
 
 	cfg->rtextents = cfg->rtblocks / cfg->rtextblocks;
-- 
2.52.0