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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Gao Xiang <hsiangkao@redhat.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>, Eric Sandeen <sandeen@redhat.com>
Subject: Re: [PATCH] mkfs.xfs: introduce sunit/swidth validation helper
Date: Mon, 3 Aug 2020 08:26:09 -0700	[thread overview]
Message-ID: <20200803152609.GA67818@magnolia> (raw)
In-Reply-To: <20200803125018.16718-1-hsiangkao@redhat.com>

On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote:
> Currently stripe unit/width 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>
> ---
> 
> This patch follows Darrick's original suggestion [1], yet I'm
> not sure if I'm doing the right thing or if something is still
> missing (e.g the meaning of six(ish) places)... So post it
> right now...
> 
> TBH, especially all these naming and the helper location (whether
> in topology.c)...plus, click a dislike on calc_stripe_factors()
> itself...
> 
> (Hopefully hear some advice about this... Thanks!)
> 
> [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia
> 
>  libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  libfrog/topology.h | 15 ++++++++++++++
>  mkfs/xfs_mkfs.c    | 48 ++++++++++++++++++++++----------------------
>  3 files changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..cf56fb03 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -174,6 +174,41 @@ out:
>  	return ret;
>  }
>  
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(

libfrog functions (and enums) should be prefixed with libfrog, not xfs.

LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.}

> +	int	sectorsize,
> +	int 	*sup,

Errant space between "int" and "*sup".

> +	int	*swp)

Strange that a validator function has out parameters...

Also, uh, .... full names, please.

	int	*sunitp,
	int	*swidthp)

(I'm vaguely wondering why we use signed ints here vs. unsigned, but
that isn't critical...)

> +{
> +	int sunit = *sup, swidth = *swp;
> +
> +	if (sectorsize) {
> +		long long	big_swidth;
> +
> +		if (sunit % sectorsize)
> +			return XFS_STRIPE_RET_SUNIT_MISALIGN;
> +
> +		sunit = (int)BTOBBT(sunit);

Hmm.  On input, *sup is in units of bytes, but on output it can be in
units of 512b blocks?  That is very surprising...

> +		big_swidth = (long long)sunit * swidth;
> +
> +		if (big_swidth > INT_MAX)
> +			return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
> +		swidth = big_swidth;
> +	}
> +	if ((sunit && !swidth) || (!sunit && swidth))
> +		return XFS_STRIPE_RET_PARTIAL_VALID;
> +
> +	if (sunit > swidth)
> +		return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
> +
> +	if (sunit && (swidth % sunit))
> +		return XFS_STRIPE_RET_SWIDTH_MISALIGN;
> +
> +	*sup = sunit;

...especially since in the !sectorsize case we don't change it at all.

> +	*swp = swidth;
> +	return XFS_STRIPE_RET_OK;
> +}
> +
>  static void blkid_get_topology(
>  	const char	*device,
>  	int		*sunit,
> @@ -229,6 +264,21 @@ static void blkid_get_topology(
>  	 */
>  	*sunit = *sunit >> 9;
>  	*swidth = *swidth >> 9;
> +	switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
> +	case XFS_STRIPE_RET_OK:
> +		break;
> +	case XFS_STRIPE_RET_PARTIAL_VALID:
> +		fprintf(stderr,
> +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
> +				progname, BBTOB(*sunit), BBTOB(*swidth));

Needs a "/* fallthrough */" comment here.

> +	default:

Why don't we warn about receiving garbage geometry that produces
MISALIGN or OVERFLOW?

> +		/*
> +		 * 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,
> diff --git a/libfrog/topology.h b/libfrog/topology.h
> index 6fde868a..e8be26b2 100644
> --- a/libfrog/topology.h
> +++ b/libfrog/topology.h
> @@ -36,4 +36,19 @@ extern int
>  check_overwrite(
>  	const char	*device);
>  
> +enum xfs_stripe_retcode {
> +	XFS_STRIPE_RET_OK = 0,
> +	XFS_STRIPE_RET_SUNIT_MISALIGN,
> +	XFS_STRIPE_RET_SWIDTH_OVERFLOW,
> +	XFS_STRIPE_RET_PARTIAL_VALID,
> +	XFS_STRIPE_RET_SUNIT_TOO_LARGE,
> +	XFS_STRIPE_RET_SWIDTH_MISALIGN,
> +};
> +
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(
> +	int	sectorsize,
> +	int 	*sup,

Errant space between "int" and "*sup".

> +	int	*swp);
> +
>  #endif	/* __LIBFROG_TOPOLOGY_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280..a3d6032c 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,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> +		dsunit = dsu;
> +		dswidth = dsw;
> +		error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);

I thought this function returned an enum?

> +		switch(error) {
> +		case XFS_STRIPE_RET_SUNIT_MISALIGN:
>  			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) {
> +			break;
> +		case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
>  			fprintf(stderr,
> -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> -				big_dswidth, dsunit);
> +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),

Why change this message?

> +				dsw, dsunit);
>  			usage();
> +			break;
>  		}
> -		dswidth = big_dswidth;
> +	} else {
> +		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> +	if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
> +	    error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
>  		fprintf(stderr,
>  _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  			dswidth, dsunit);
>  		usage();
>  	}
>  
> +	if (error) {
> +		fprintf(stderr,
> +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);

Invalid how?  We know the exact reason, so we should say so.

--D

> +		usage();
> +	}
> +
>  	/* If sunit & swidth were manually specified as 0, same as noalign */
>  	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
>  	    !dsunit && !dswidth)
> @@ -2328,18 +2337,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) {
> -- 
> 2.18.1
> 