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

[Eric Sandeen <sandeen@xxxxxxxxxxx>]

On 3/24/16 6:15 AM, jtulak@xxxxxxxxxx wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Jan Tulak <jtulak@xxxxxxxxxx>
> ---
>  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