Re: [PATCH 05/19] mkfs: factor boolean option parsing

Eric Sandeen
2016-04-07T02:48:52Z

On 3/24/16 6:15 AM, jtulak@redhat.com wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> UPDATE:
> - add a forgotten getbool update to FINOBT
> - change -m crc and ftype to enable by specifying (instead of toggle off)
>
> Many of the options passed to mkfs have boolean options (0 or 1) and
> all hand roll the same code and validity checks. Factor these out
> into a common function.
>
> Note that the lazy-count option is now changed to match other
> booleans in that if you don't specify a value, it reverts to the
> default value (on) rather than throwing an error.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 109 +++++++++++++++++++++++---------------------------------
> 1 file changed, 44 insertions(+), 65 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 4811d77..9394bd3 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c

...

> @@ -1451,12 +1459,8 @@ main(
> sb_feat.attr_version = c;
> break;
> case I_PROJID32BIT:
> - if (!value || *value == '\0')
> - value = "0";
> - c = getnum(value, 0, 0, false);
> - if (c < 0 || c > 1)
> - illegal(value, "i projid32bit");
> - sb_feat.projid16bit = c ? false : true;
> + sb_feat.projid16bit = !getbool(value,
> + "i projid32bit", false);
> break;
> case I_SPINODES:
> if (!value || *value == '\0')

I_SPINODES now needs the getbool() treatment as well.

...

> @@ -1631,28 +1625,20 @@ main(
>
> switch (getsubopt(&p, (constpp)mopts, &value)) {
> case M_CRC:
> - if (!value || *value == '\0')
> - reqval('m', mopts, M_CRC);
> - c = getnum(value, 0, 0, false);
> - if (c < 0 || c > 1)
> - illegal(value, "m crc");
> - if (c && nftype) {
> + sb_feat.crcs_enabled = getbool(
> + value, "m crc", true);
> + if (sb_feat.crcs_enabled && nftype) {
> fprintf(stderr,
> -_("cannot specify both crc and ftype\n"));
> +_("cannot specify both -m crc=1 and -n ftype\n"));
> usage();
> }
> - sb_feat.crcs_enabled = c ? true : false;
> - if (c)
> + if (sb_feat.crcs_enabled)
> sb_feat.dirftype = true;
> break;
> case M_FINOBT:
> - if (!value || *value == '\0')
> - reqval('m', mopts, M_CRC);

^^^ whoops ;)
Well that bug goes away now ;)

> - c = atoi(value);
> - if (c < 0 || c > 1)
> - illegal(value, "m finobt");
> sb_feat.finobtflag = true;
> - sb_feat.finobt = c;
> + sb_feat.finobt = getbool(
> + value, "m finobt", true);
> break;
> case M_UUID:
> if (!value || *value == '\0')

Other than SPINODES this one looks ok to me.

-Eric