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

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:01:54PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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