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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
