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

[Brian Foster <bfoster@xxxxxxxxxx>]

On Fri, Jun 19, 2015 at 01:01:54PM +0200, Jan ÅulÃk wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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. Similarly the -m
> crc and -n ftype options default to off rather than throwing an
> error.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan ÅulÃk <jtulak@xxxxxxxxxx>
> ---
>  mkfs/xfs_mkfs.c | 101 
> +++++++++++++++++++++++---------------------------------
>  1 file changed, 42 insertions(+), 59 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1a5e2f8..6b9e991 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -42,7 +42,7 @@ struct fs_topology {
>   * Prototypes for internal functions.
>   */
>  static void conflict(char opt, char *tab[], int oldidx, int newidx);
> -static void illegal(char *value, char *opt);
> +static void illegal(const char *value, const char *opt);
>  static __attribute__((noreturn)) void usage (void);
>  static __attribute__((noreturn)) void reqval(char opt, char *tab[], int idx);
>  static void respec(char opt, char *tab[], int idx);
> @@ -1028,6 +1028,21 @@ getnum(
>       return i;
>  }
>  
> +static bool
> +getbool(
> +     const char      *str,
> +     const char      *illegal_str,
> +     bool            default_val)
> +{
> +     long long       c;
> +
> +     if (!str || *str == '\0')
> +             return default_val;
> +     c = getnum(str, 0, 0, false);
> +     if (c < 0 || c > 1)
> +             illegal(str, illegal_str);
> +     return c ? true : false;
> +}
>  
>  int
>  main(
> @@ -1247,11 +1262,8 @@ main(
>                                       dasize = 1;
>                                       break;
>                               case D_FILE:
> -                                     if (!value || *value == '\0')
> -                                             value = "1";
> -                                     xi.disfile = getnum(value, 0, 0, false);
> -                                     if (xi.disfile < 0 || xi.disfile > 1)
> -                                             illegal(value, "d file");
> +                                     xi.disfile = getbool(value, "d file",
> +                                                          true);
>                                       if (xi.disfile && !Nflag)
>                                               xi.dcreat = 1;
>                                       break;
> @@ -1394,12 +1406,8 @@ main(
>  
>                               switch (getsubopt(&p, (constpp)iopts, &value)) {
>                               case I_ALIGN:
> -                                     if (!value || *value == '\0')
> -                                             break;
> -                                     c = getnum(value, 0, 0, false);
> -                                     if (c < 0 || c > 1)
> -                                             illegal(value, "i align");
> -                                     sb_feat.inode_align = c ? true : false;
> +                                     sb_feat.inode_align = getbool(
> +                                                     value, "i align", true);
>                                       break;
>                               case I_LOG:
>                                       if (!value || *value == '\0')
> @@ -1472,12 +1480,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')
> @@ -1510,20 +1514,15 @@ main(
>                                       laflag = 1;
>                                       break;
>                               case L_FILE:
> -                                     if (!value || *value == '\0')
> -                                             value = "1";
>                                       if (loginternal)
>                                               conflict('l', lopts, L_INTERNAL,
>                                                        L_FILE);
> -                                     xi.lisfile = getnum(value, 0, 0, false);
> -                                     if (xi.lisfile < 0 || xi.lisfile > 1)
> -                                             illegal(value, "l file");
> +                                     xi.lisfile = getbool(value, "l file",
> +                                                          true);
>                                       if (xi.lisfile)
>                                               xi.lcreat = 1;
>                                       break;
>                               case L_INTERNAL:
> -                                     if (!value || *value == '\0')
> -                                             value = "1";
>                                       if (ldflag)
>                                               conflict('l', lopts, 
> L_INTERNAL, L_DEV);
>                                       if (xi.lisfile)
> @@ -1531,9 +1530,9 @@ main(
>                                                        L_INTERNAL);
>                                       if (liflag)
>                                               respec('l', lopts, L_INTERNAL);
> -                                     loginternal = getnum(value, 0, 0, 
> false);
> -                                     if (loginternal < 0 || loginternal > 1)
> -                                             illegal(value, "l internal");
> +
> +                                     loginternal = getbool(value,
> +                                                     "l internal", true);
>                                       liflag = 1;
>                                       break;
>                               case L_SU:
> @@ -1623,14 +1622,9 @@ main(
>                                       lssflag = 1;
>                                       break;
>                               case L_LAZYSBCNTR:
> -                                     if (!value || *value == '\0')
> -                                             reqval('l', lopts,
> -                                                             L_LAZYSBCNTR);
> -                                     c = getnum(value, 0, 0, false);
> -                                     if (c < 0 || c > 1)
> -                                             illegal(value, "l lazy-count");
> -                                     sb_feat.lazy_sb_counters = c ? true
> -                                                                  : false;
> +                                     sb_feat.lazy_sb_counters = getbool(
> +                                                     value, "l lazy-count",
> +                                                     true);
>                                       break;
>                               default:
>                                       unknown('l', value);
> @@ -1649,18 +1643,14 @@ 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", false);

Hmm... so I know we have crc on by default now, but it seems a little
weird to me for '-m crc' to turn it off.

> +                                     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:

No getbool() update for finobt?

> @@ -1731,19 +1721,15 @@ _("cannot specify both crc and ftype\n"));
>                                       nvflag = 1;
>                                       break;
>                               case N_FTYPE:
> -                                     if (!value || *value == '\0')
> -                                             reqval('n', nopts, N_FTYPE);
>                                       if (nftype)
>                                               respec('n', nopts, N_FTYPE);
> -                                     c = getnum(value, 0, 0, false);
> -                                     if (c < 0 || c > 1)
> -                                             illegal(value, "n ftype");
>                                       if (sb_feat.crcs_enabled) {
>                                               fprintf(stderr,
> -_("cannot specify both crc and ftype\n"));
> +_("cannot specify both -m crc=1 and -n ftype\n"));
>                                               usage();
>                                       }
> -                                     sb_feat.dirftype = c ? true : false;
> +                                     sb_feat.dirftype = getbool(value,
> +                                                          "n ftype", false);

Similar weirdness here, IMO. Using '-m crc -n ftype' gives an fs without
either. The toggling behavior is consistent I suppose, but it seems
really nonintuitive to me. I would expect the act of specifying
something as an implicit request to enable, regardless of the
application specific defaults (that do change once in a while).

Brian

>                                       nftype = 1;
>                                       break;
>                               default:
> @@ -1779,11 +1765,8 @@ _("cannot specify both crc and ftype\n"));
>                                       rtextsize = value;
>                                       break;
>                               case R_FILE:
> -                                     if (!value || *value == '\0')
> -                                             value = "1";
> -                                     xi.risfile = getnum(value, 0, 0, false);
> -                                     if (xi.risfile < 0 || xi.risfile > 1)
> -                                             illegal(value, "r file");
> +                                     xi.risfile = getbool(value,
> +                                                          "r file", true);
>                                       if (xi.risfile)
>                                               xi.rcreat = 1;
>                                       break;
> @@ -3228,8 +3211,8 @@ conflict(
>  
>  static void
>  illegal(
> -     char            *value,
> -     char            *opt)
> +     const char      *value,
> +     const char      *opt)
>  {
>       fprintf(stderr, _("Illegal value %s for -%s option\n"), value, opt);
>       usage();
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs