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

Brian Foster
2015-06-26T17:17:25Z

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