[PATCH v3] mkfs.xfs: introduce sunit/swidth validation helper

[Gao Xiang <hsiangkao@redhat.com>]

Currently stripe unit/swidth checking logic is all over xfsprogs.
So, refactor the same code snippet into a single validation helper
xfs_validate_stripe_factors(), including:
 - integer overflows of either value
 - sunit and swidth alignment wrt sector size
 - if either sunit or swidth are zero, both should be zero
 - swidth must be a multiple of sunit

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---

changes since v2:
 - try to cover xfs_validate_sb_common() case as well suggested by Eric,
   so move to libxfs as an attempt for review;

 - use an static inline helper in xfs_sb.h so compilers can do their
   best to eliminate unneeded branches / rearrange code according to
   each individual caller;

 - get back to use "int error" expression since it's compatible with
   "enum xfs_stripeval" and it can be laterly reused for future uses
   in these callers instead of introducing another error code variables;

just for review/comments for now, if it looks almost fine, I'd like to
go further to stage up related functions to kernel.

 libfrog/topology.c | 15 +++++++++++
 libxfs/xfs_sb.c    | 32 ++++++++++++++++--------
 libxfs/xfs_sb.h    | 54 ++++++++++++++++++++++++++++++++++++++++
 mkfs/xfs_mkfs.c    | 62 ++++++++++++++++++++++++----------------------
 po/pl.po           |  4 +--
 5 files changed, 124 insertions(+), 43 deletions(-)

diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9..554afbfc 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -187,6 +187,7 @@ static void blkid_get_topology(
 	blkid_probe pr;
 	unsigned long val;
 	struct stat statbuf;
+	int error;
 
 	/* can't get topology info from a file */
 	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
@@ -230,6 +231,20 @@ static void blkid_get_topology(
 	*sunit = *sunit >> 9;
 	*swidth = *swidth >> 9;
 
+	error = xfs_validate_stripe_factors(0, sunit, swidth);
+	if (error) {
+		fprintf(stderr,
+_("%s: Volume reports invalid sunit (%d bytes) and swidth (%d bytes) %s, ignoring.\n"),
+			progname, BBTOB(*sunit), BBTOB(*swidth),
+			xfs_stripeval_str[error]);
+		/*
+		 * if firmware is broken, just give up and set both to zero,
+		 * we can't trust information from this device.
+		 */
+		*sunit = 0;
+		*swidth = 0;
+	}
+
 	if (blkid_topology_get_alignment_offset(tp) != 0) {
 		fprintf(stderr,
 			_("warning: device is not properly aligned %s\n"),
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index d37d60b3..0c0f5f90 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -210,6 +210,15 @@ xfs_validate_sb_write(
 	return 0;
 }
 
+const char *xfs_stripeval_str[] = {
+	"OK",
+	"SUNIT_MISALIGN",
+	"SWIDTH_OVERFLOW",
+	"PARTIAL_VALID",
+	"SUNIT_TOO_LARGE",
+	"SWIDTH_MISALIGN",
+};
+
 /* Check the validity of the SB. */
 STATIC int
 xfs_validate_sb_common(
@@ -220,6 +229,8 @@ xfs_validate_sb_common(
 	struct xfs_dsb		*dsb = bp->b_addr;
 	uint32_t		agcount = 0;
 	uint32_t		rem;
+	int			sunit, swidth;
+	int			error;
 
 	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
 		xfs_warn(mp, "bad magic number");
@@ -357,21 +368,20 @@ xfs_validate_sb_common(
 		}
 	}
 
-	if (sbp->sb_unit) {
-		if (!xfs_sb_version_hasdalign(sbp) ||
-		    sbp->sb_unit > sbp->sb_width ||
-		    (sbp->sb_width % sbp->sb_unit) != 0) {
-			xfs_notice(mp, "SB stripe unit sanity check failed");
-			return -EFSCORRUPTED;
-		}
-	} else if (xfs_sb_version_hasdalign(sbp)) {
+	sunit = sbp->sb_unit;
+	swidth = sbp->sb_width;
+
+	if (!sunit ^ !xfs_sb_version_hasdalign(sbp)) {
 		xfs_notice(mp, "SB stripe alignment sanity check failed");
 		return -EFSCORRUPTED;
-	} else if (sbp->sb_width) {
-		xfs_notice(mp, "SB stripe width sanity check failed");
-		return -EFSCORRUPTED;
 	}
 
+	error = xfs_validate_stripe_factors(0, &sunit, &swidth);
+	if (error) {
+		xfs_notice(mp, "SB stripe sanity check failed %s",
+				xfs_stripeval_str[error]);
+		return -EFSCORRUPTED;
+	}
 
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a..c4524bbc 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -41,5 +41,59 @@ extern int	xfs_sb_read_secondary(struct xfs_mount *mp,
 extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
 				struct xfs_trans *tp, xfs_agnumber_t agno,
 				struct xfs_buf **bpp);
+enum xfs_stripeval {
+	XFS_STRIPEVAL_OK = 0,
+	XFS_STRIPEVAL_SUNIT_MISALIGN,
+	XFS_STRIPEVAL_SWIDTH_OVERFLOW,
+	XFS_STRIPEVAL_PARTIAL_VALID,
+	XFS_STRIPEVAL_SUNIT_TOO_LARGE,
+	XFS_STRIPEVAL_SWIDTH_MISALIGN,
+};
+
+extern const char *xfs_stripeval_str[];
+
+/*
+ * This accepts either
+ *  - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
+ * or
+ *  - (sectersize == 0) sunit / swidth (in 512B sectors)
+ * and return sunit/swidth in sectors.
+ */
+static inline enum xfs_stripeval
+xfs_validate_stripe_factors(
+	int	sectorsize,
+	int	*sunitp,
+	int	*swidthp)
+{
+	int	sunit = *sunitp;
+	int	swidth = *swidthp;
+
+	if (sectorsize) {
+		long long	big_swidth;
+
+		if (sunit % sectorsize)
+			return XFS_STRIPEVAL_SUNIT_MISALIGN;
+
+		sunit = (int)BTOBBT(sunit);
+		big_swidth = (long long)sunit * swidth;
+
+		if (big_swidth > INT_MAX)
+			return XFS_STRIPEVAL_SWIDTH_OVERFLOW;
+		swidth = big_swidth;
+	}
+
+	if ((sunit && !swidth) || (!sunit && swidth))
+		return XFS_STRIPEVAL_PARTIAL_VALID;
+
+	if (sunit > swidth)
+		return XFS_STRIPEVAL_SUNIT_TOO_LARGE;
+
+	if (sunit && (swidth % sunit))
+		return XFS_STRIPEVAL_SWIDTH_MISALIGN;
+
+	*sunitp = sunit;
+	*swidthp = swidth;
+	return XFS_STRIPEVAL_OK;
+}
 
 #endif	/* __XFS_SB_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280..8fdf17d7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2255,7 +2255,6 @@ calc_stripe_factors(
 	struct cli_params	*cli,
 	struct fs_topology	*ft)
 {
-	long long int	big_dswidth;
 	int		dsunit = 0;
 	int		dswidth = 0;
 	int		lsunit = 0;
@@ -2263,6 +2262,7 @@ calc_stripe_factors(
 	int		dsw = 0;
 	int		lsu = 0;
 	bool		use_dev = false;
+	int		error;
 
 	if (cli_opt_set(&dopts, D_SUNIT))
 		dsunit = cli->dsunit;
@@ -2289,29 +2289,40 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		if (dsu % cfg->sectorsize) {
-			fprintf(stderr,
-_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
-			usage();
-		}
-
-		dsunit  = (int)BTOBBT(dsu);
-		big_dswidth = (long long int)dsunit * dsw;
-		if (big_dswidth > INT_MAX) {
-			fprintf(stderr,
-_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
-				big_dswidth, dsunit);
-			usage();
-		}
-		dswidth = big_dswidth;
+		dsunit = dsu;
+		dswidth = dsw;
+		error = xfs_validate_stripe_factors(cfg->sectorsize,
+				&dsunit, &dswidth);
+	} else {
+		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
 	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
+	switch (error) {
+	case XFS_STRIPEVAL_OK:
+		break;
+	case XFS_STRIPEVAL_SUNIT_MISALIGN:
+		fprintf(stderr,
+_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
+		usage();
+		break;
+	case XFS_STRIPEVAL_SWIDTH_OVERFLOW:
+		fprintf(stderr,
+_("data stripe width (%d) is too large of a multiple of the data stripe unit (%d)\n"),
+			dsw, dsunit);
+		usage();
+		break;
+	case XFS_STRIPEVAL_PARTIAL_VALID:
+	case XFS_STRIPEVAL_SWIDTH_MISALIGN:
 		fprintf(stderr,
 _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
 			dswidth, dsunit);
 		usage();
+		break;
+	default:
+		fprintf(stderr,
+_("invalid data stripe unit (%d), width (%d) %s\n"),
+			dsunit, dswidth, xfs_stripeval_str[error]);
+		usage();
 	}
 
 	/* If sunit & swidth were manually specified as 0, same as noalign */
@@ -2328,18 +2339,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
 
 	/* if no stripe config set, use the device default */
 	if (!dsunit) {
-		/* Ignore nonsense from device.  XXX add more validation */
-		if (ft->dsunit && ft->dswidth == 0) {
-			fprintf(stderr,
-_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
-				progname, BBTOB(ft->dsunit));
-			ft->dsunit = 0;
-			ft->dswidth = 0;
-		} else {
-			dsunit = ft->dsunit;
-			dswidth = ft->dswidth;
-			use_dev = true;
-		}
+		dsunit = ft->dsunit;
+		dswidth = ft->dswidth;
+		use_dev = true;
 	} else {
 		/* check and warn if user-specified alignment is sub-optimal */
 		if (ft->dsunit && ft->dsunit != dsunit) {
diff --git a/po/pl.po b/po/pl.po
index 87109f6b..02d2258f 100644
--- a/po/pl.po
+++ b/po/pl.po
@@ -9085,10 +9085,10 @@ msgstr "su danych musi być wielokrotnością rozmiaru sektora (%d)\n"
 #: .././mkfs/xfs_mkfs.c:2267
 #, c-format
 msgid ""
-"data stripe width (%lld) is too large of a multiple of the data stripe unit "
+"data stripe width (%d) is too large of a multiple of the data stripe unit "
 "(%d)\n"
 msgstr ""
-"szerokość pasa danych (%lld) jest zbyt dużą wielokrotnością jednostki pasa "
+"szerokość pasa danych (%d) jest zbyt dużą wielokrotnością jednostki pasa "
 "danych (%d)\n"
 
 #: .././mkfs/xfs_mkfs.c:2276
-- 
2.18.1