Re: [PATCH 11/17] mkfs: table based parsing for converted parameters

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:02:00PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> All the parameters that can be passed as block or sector sizes need
> to be passed the block and sector sizes that they should be using
> for conversion. For parameter parsing, it is always the same two
> variables, so to make things easy just declare them as global
> variables so we can avoid needing to pass them to getnum_checked().
> 
> We also need to mark these parameters are requiring conversion so
> that we don't need to pass this information manually to
> getnum_checked(). Further, some of these options are required to
> have a power of 2 value, so add optional checking for that as well.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  mkfs/xfs_mkfs.c | 171 ++++++++++++++++++++++----------------------------------
>  1 file changed, 68 insertions(+), 103 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 0bd8998..4fc1f34 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -52,6 +52,13 @@ static int  ispow2(unsigned int i);
>  static long long cvtnum(unsigned int blocksize,
>  			unsigned int sectorsize, const char *s);
>  
> +/*
> + * 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;
> +
>  #define MAX_SUBOPTS	16
>  #define SUBOPT_NEEDS_VAL	(-1LL)
>  struct opt_params {
> @@ -61,6 +68,8 @@ struct opt_params {
>  	struct subopt_param {
>  		int		index;
>  		bool		seen;
> +		bool		convert;
> +		bool		is_power_2;
>  		long long	minval;
>  		long long	maxval;
>  		long long	defaultval;
> @@ -83,6 +92,8 @@ struct opt_params bopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = B_SIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_BLOCKSIZE,
>  		  .maxval = XFS_MAX_BLOCKSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -140,6 +151,9 @@ struct opt_params dopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SIZE,
> +		  .convert = true,
> +		  .minval = XFS_AG_MIN_BYTES,
> +		  .maxval = LLONG_MAX,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SUNIT,
> @@ -153,11 +167,15 @@ struct opt_params dopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_AGSIZE,
> +		  .convert = true,
>  		  .minval = XFS_AG_MIN_BYTES,
>  		  .maxval = XFS_AG_MAX_BYTES,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SU,
> +		  .convert = true,
> +		  .minval = 0,
> +		  .maxval = UINT_MAX,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SW,
> @@ -171,6 +189,8 @@ struct opt_params dopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SECTSIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -237,11 +257,13 @@ struct opt_params iopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_PERBLOCK,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_INODE_PERBLOCK,
>  		  .maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_SIZE,
> +		  .is_power_2 = true,
>  		  .minval = XFS_DINODE_MIN_SIZE,
>  		  .maxval = XFS_DINODE_MAX_SIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -300,6 +322,9 @@ struct opt_params lopts = {
>  		  .defaultval = 1,
>  		},
>  		{ .index = L_SIZE,
> +		  .convert = true,
> +		  .minval = 2 * 1024 * 1024LL,	/* XXX: XFS_MIN_LOG_BYTES */
> +		  .maxval = XFS_MAX_LOG_BYTES,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_VERSION,
> @@ -313,6 +338,9 @@ struct opt_params lopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SU,
> +		  .convert = true,
> +		  .minval = XLOG_MIN_RECORD_BSIZE,
> +		  .maxval = XLOG_MAX_RECORD_BSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_DEV,
> @@ -324,6 +352,8 @@ struct opt_params lopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SECTSIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -364,6 +394,8 @@ struct opt_params nopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = N_SIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = 1 << XFS_MIN_REC_DIRSIZE,
>  		  .maxval = XFS_MAX_BLOCKSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -400,9 +432,15 @@ struct opt_params ropts = {
>  	},
>  	.subopt_params = {
>  		{ .index = R_EXTSIZE,
> +		  .convert = true,
> +		  .minval = XFS_MIN_RTEXTSIZE,
> +		  .maxval = XFS_MAX_RTEXTSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_SIZE,
> +		  .convert = true,
> +		  .minval = 0,
> +		  .maxval = LLONG_MAX,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_DEV,
> @@ -445,11 +483,15 @@ struct opt_params sopts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = S_SIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = S_SECTSIZE,
> +		  .convert = true,
> +		  .is_power_2 = true,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -1291,15 +1333,15 @@ sb_set_features(
>  long long
>  getnum(
>  	const char	*str,
> -	unsigned int	blocksize,
> -	unsigned int	sectorsize,
> +	unsigned int	blksize,
> +	unsigned int	sectsize,
>  	bool		convert)
>  {
>  	long long	i;
>  	char		*sp;
>  
>  	if (convert)
> -		return cvtnum(blocksize, sectorsize, str);
> +		return cvtnum(blksize, sectsize, str);
>  
>  	i = strtoll(str, &sp, 0);
>  	if (i == 0 && sp == str)
> @@ -1348,9 +1390,11 @@ getnum_checked(
>  		return sp->defaultval;
>  	}
>  
> -	c = getnum(str, 0, 0, false);
> +	c = getnum(str, blocksize, sectorsize, sp->convert);
>  	if (c < sp->minval || c > sp->maxval)
>  		illegal_option(str, opts, index);
> +	if (sp->is_power_2 && !ispow2(c))
> +		illegal_option(str, opts, index);
>  	return c;
>  }
>  
> @@ -1368,7 +1412,6 @@ main(
>  	struct xfs_btree_block	*block;
>  	int			blflag;
>  	int			blocklog;
> -	unsigned int		blocksize;
>  	int			bsflag;
>  	int			bsize;
>  	xfs_buf_t		*buf;
> @@ -1441,7 +1484,6 @@ main(
>  	char			*rtsize;
>  	xfs_sb_t		*sbp;
>  	int			sectorlog;
> -	unsigned int		sectorsize;
>  	__uint64_t		sector_mask;
>  	int			slflag;
>  	int			ssflag;
> @@ -1520,18 +1562,11 @@ main(
>  					blflag = 1;
>  					break;
>  				case B_SIZE:
> -					if (!value || *value == '\0')
> -						reqval('b', subopts, B_SIZE);
> -					if (bsflag)
> -						respec('b', subopts, B_SIZE);
>  					if (blflag)
>  						conflict('b', subopts, B_LOG,
>  							 B_SIZE);
> -					blocksize = getnum(value, blocksize,
> -							sectorsize, true);
> -					if (blocksize <= 0 ||
> -					    !ispow2(blocksize))
> -						illegal(value, "b size");
> +					blocksize = getnum_checked(value, &bopts,
> +								  B_SIZE);
>  					blocklog = libxfs_highbit32(blocksize);
>  					bsflag = 1;
>  					break;
> @@ -1554,14 +1589,8 @@ main(
>  					daflag = 1;
>  					break;
>  				case D_AGSIZE:
> -					if (!value || *value == '\0')
> -						reqval('d', subopts, D_AGSIZE);
> -					if (dasize)
> -						respec('d', subopts, D_AGSIZE);
> -					agsize = getnum(value, blocksize,
> -							sectorsize, true);
> -					if ((__int64_t)agsize <= 0)
> -						illegal(value, "d agsize");
> +					agsize = getnum_checked(value, &dopts,
> +								 D_AGSIZE);
>  					dasize = 1;
>  					break;
>  				case D_FILE:
> @@ -1599,17 +1628,11 @@ main(
>  								 D_SWIDTH);
>  					break;
>  				case D_SU:
> -					if (!value || *value == '\0')
> -						reqval('d', subopts, D_SU);
> -					if (dsu)
> -						respec('d', subopts, D_SU);
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SU);
> -					dsu = getnum(value, blocksize,
> -						     sectorsize, true);
> -					if (dsu < 0)
> -						illegal(value, "d su");
> +					dsu = getnum_checked(value, &dopts,
> +								 D_SU);
>  					break;
>  				case D_SW:
>  					if (nodsflag)
> @@ -1643,18 +1666,11 @@ main(
>  					slflag = 1;
>  					break;
>  				case D_SECTSIZE:
> -					if (!value || *value == '\0')
> -						reqval('d', subopts, D_SECTSIZE);
> -					if (ssflag)
> -						respec('d', subopts, D_SECTSIZE);
>  					if (slflag)
>  						conflict('d', subopts, D_SECTLOG,
>  							 D_SECTSIZE);
> -					sectorsize = getnum(value, blocksize,
> -							    sectorsize, true);
> -					if (sectorsize <= 0 ||
> -					    !ispow2(sectorsize))
> -						illegal(value, "d sectsize");
> +					sectorsize = getnum_checked(value,
> +							&dopts, D_SECTSIZE);
>  					sectorlog =
>  						libxfs_highbit32(sectorsize);
>  					ssflag = 1;
> @@ -1721,8 +1737,6 @@ main(
>  							 I_PERBLOCK);
>  					inopblock = getnum_checked(value, &iopts,
>  								   I_PERBLOCK);
> -					if (!ispow2(inopblock))
> -						illegal(value, "i perblock");
>  					ipflag = 1;
>  					break;
>  				case I_SIZE:
> @@ -1734,8 +1748,6 @@ main(
>  							 I_SIZE);
>  					isize = getnum_checked(value, &iopts,
>  							       I_SIZE);
> -					if (!ispow2(isize))
> -						illegal(value, "i size");
>  					inodelog = libxfs_highbit32(isize);
>  					isflag = 1;
>  					break;
> @@ -1797,14 +1809,8 @@ main(
>  					liflag = 1;
>  					break;
>  				case L_SU:
> -					if (!value || *value == '\0')
> -						reqval('l', subopts, L_SU);
> -					if (lsu)
> -						respec('l', subopts, L_SU);
> -					lsu = getnum(value, blocksize,
> -						     sectorsize, true);
> -					if (lsu < 0)
> -						illegal(value, "l su");
> +					lsu = getnum_checked(value, &lopts,
> +								 L_SU);
>  					lsuflag = 1;
>  					break;
>  				case L_SUNIT:
> @@ -1851,18 +1857,11 @@ main(
>  					lslflag = 1;
>  					break;
>  				case L_SECTSIZE:
> -					if (!value || *value == '\0')
> -						reqval('l', subopts, L_SECTSIZE);
> -					if (lssflag)
> -						respec('l', subopts, L_SECTSIZE);
>  					if (lslflag)
>  						conflict('l', subopts, L_SECTLOG,
>  							 L_SECTSIZE);
> -					lsectorsize = getnum(value, blocksize,
> -							     sectorsize, true);
> -					if (lsectorsize <= 0 ||
> -					    !ispow2(lsectorsize))
> -						illegal(value, "l sectsize");
> +					lsectorsize = getnum_checked(value,
> +							&lopts, L_SECTSIZE);
>  					lsectorlog =
>  						libxfs_highbit32(lsectorsize);
>  					lssflag = 1;
> @@ -1933,18 +1932,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					nlflag = 1;
>  					break;
>  				case N_SIZE:
> -					if (!value || *value == '\0')
> -						reqval('n', subopts, N_SIZE);
> -					if (nsflag)
> -						respec('n', subopts, N_SIZE);
>  					if (nlflag)
>  						conflict('n', subopts, N_LOG,
>  							 N_SIZE);
> -					dirblocksize = getnum(value, blocksize,
> -							      sectorsize, true);
> -					if (dirblocksize <= 0 ||
> -					    !ispow2(dirblocksize))
> -						illegal(value, "n size");
> +					dirblocksize = getnum_checked(value,
> +								&nopts, N_SIZE);
>  					dirblocklog =
>  						libxfs_highbit32(dirblocksize);
>  					nsflag = 1;
> @@ -2060,18 +2052,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					break;
>  				case S_SIZE:
>  				case S_SECTSIZE:
> -					if (!value || *value == '\0')
> -						reqval('s', subopts, S_SECTSIZE);
> -					if (ssflag || lssflag)
> -						respec('s', subopts, S_SECTSIZE);
>  					if (slflag || lslflag)
>  						conflict('s', subopts, S_SECTLOG,
>  							 S_SECTSIZE);
> -					sectorsize = getnum(value, blocksize,
> -							    sectorsize, true);
> -					if (sectorsize <= 0 ||
> -					    !ispow2(sectorsize))
> -						illegal(value, "s sectsize");
> +					sectorsize = getnum_checked(value,
> +							&sopts, S_SECTSIZE);
>  					lsectorsize = sectorsize;
>  					sectorlog =
>  						libxfs_highbit32(sectorsize);
> @@ -2306,9 +2291,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (dsize) {
>  		__uint64_t dbytes;
>  
> -		dbytes = getnum(dsize, blocksize, sectorsize, true);
> -		if ((__int64_t)dbytes < 0)
> -			illegal(dsize, "d size");
> +		dbytes = getnum_checked(dsize, &dopts, D_SIZE);
>  		if (dbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal data length %lld, not a multiple of %d\n"),
> @@ -2345,9 +2328,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (logsize) {
>  		__uint64_t logbytes;
>  
> -		logbytes = getnum(logsize, blocksize, sectorsize, true);
> -		if ((__int64_t)logbytes < 0)
> -			illegal(logsize, "l size");
> +		logbytes = getnum_checked(logsize, &lopts, L_SIZE);
>  		if (logbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal log length %lld, not a multiple of %d\n"),
> @@ -2369,9 +2350,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (rtsize) {
>  		__uint64_t rtbytes;
>  
> -		rtbytes = getnum(rtsize, blocksize, sectorsize, true);
> -		if ((__int64_t)rtbytes < 0)
> -			illegal(rtsize, "r size");
> +		rtbytes = getnum_checked(rtsize, &ropts, R_SIZE);
>  		if (rtbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal rt length %lld, not a multiple of %d\n"),
> @@ -2391,27 +2370,13 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (rtextsize) {
>  		__uint64_t rtextbytes;
>  
> -		rtextbytes = getnum(rtextsize, blocksize, sectorsize, true);
> -		if ((__int64_t)rtextbytes < 0)
> -			illegal(rtsize, "r extsize");
> +		rtextbytes = getnum_checked(rtextsize, &ropts, R_EXTSIZE);
>  		if (rtextbytes % blocksize) {
>  			fprintf(stderr,
>  		_("illegal rt extent size %lld, not a multiple of %d\n"),
>  				(long long)rtextbytes, blocksize);
>  			usage();
>  		}
> -		if (rtextbytes > XFS_MAX_RTEXTSIZE) {
> -			fprintf(stderr,
> -				_("rt extent size %s too large, maximum %d\n"),
> -				rtextsize, XFS_MAX_RTEXTSIZE);
> -			usage();
> -		}
> -		if (rtextbytes < XFS_MIN_RTEXTSIZE) {
> -			fprintf(stderr,
> -				_("rt extent size %s too small, minimum %d\n"),
> -				rtextsize, XFS_MIN_RTEXTSIZE);
> -			usage();
> -		}
>  		rtextblocks = (xfs_extlen_t)(rtextbytes >> blocklog);
>  	} else {
>  		/*
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs