On 3/24/16 6:15 AM, jtulak@xxxxxxxxxx wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> CHANGELOG:
> o Remove unused argument of check_opt.
> o Add a comment to explain a new member of opt_params struct.
> o A stray chunk moved from the following patch to this one.
>
> So that string options are correctly detected for conflicts and
> respecification, add a getstr() function that modifies the option
> tables appropriately.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan Tulak <jtulak@xxxxxxxxxx>
Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> mkfs/xfs_mkfs.c | 143 ++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 78 insertions(+), 65 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d119580..9261ed5 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -83,6 +83,10 @@ unsigned int sectorsize;
> * Do not set this flag when definning a subopt. It is used to remeber that
> * this subopt was already seen, for example for conflicts detection.
> *
> + * str_seen INTERNAL
> + * Do not set. It is used internally for respecification, when some options
> + * has to be parsed twice - at first as a string, then later as a number.
> + *
> * convert OPTIONAL
> * A flag signalling whether the user-given value can use suffixes.
> * If you want to allow the use of user-friendly values like 13k, 42G,
> @@ -124,6 +128,7 @@ struct opt_params {
> struct subopt_param {
> int index;
> bool seen;
> + bool str_seen;
> bool convert;
> bool is_power_2;
> int conflicts[MAX_CONFLICTS];
> @@ -1470,14 +1475,17 @@ illegal_option(
> usage();
> }
>
> -static long long
> -getnum(
> - const char *str,
> +/*
> + * Check for conflicts and option respecification.
> + */
> +static void
> +check_opt(
> struct opt_params *opts,
> - int index)
> + int index,
> + bool str_seen)
> {
> - struct subopt_param *sp = &opts->subopt_params[index];
> - long long c;
> + struct subopt_param *sp = &opts->subopt_params[index];
> + int i;
>
> if (sp->index != index) {
> fprintf(stderr,
> @@ -1486,22 +1494,47 @@ getnum(
> reqval(opts->name, (char **)opts->subopts, index);
> }
>
> - /* check for respecification of the option */
> - if (sp->seen)
> - respec(opts->name, (char **)opts->subopts, index);
> - sp->seen = true;
> + /*
> + * Check for respecification of the option. This is more complex than it
> + * seems because some options are parsed twice - once as a string during
> + * input parsing, then later the string is passed to getnum for
> + * conversion into a number and bounds checking. Hence the two variables
> + * used to track the different uses based on the @str parameter passed
> + * to us.
> + */
> + if (!str_seen) {
> + if (sp->seen)
> + respec(opts->name, (char **)opts->subopts, index);
> + sp->seen = true;
> + } else {
> + if (sp->str_seen)
> + respec(opts->name, (char **)opts->subopts, index);
> + sp->str_seen = true;
> + }
>
> /* check for conflicts with the option */
> - for (c = 0; c < MAX_CONFLICTS; c++) {
> - int conflict_opt = sp->conflicts[c];
> + for (i = 0; i < MAX_CONFLICTS; i++) {
> + int conflict_opt = sp->conflicts[i];
>
> if (conflict_opt == LAST_CONFLICT)
> break;
> - if (opts->subopt_params[conflict_opt].seen)
> + if (opts->subopt_params[conflict_opt].seen ||
> + opts->subopt_params[conflict_opt].str_seen)
> conflict(opts->name, (char **)opts->subopts,
> conflict_opt, index);
> }
> +}
>
> +static long long
> +getnum(
> + const char *str,
> + struct opt_params *opts,
> + int index)
> +{
> + struct subopt_param *sp = &opts->subopt_params[index];
> + long long c;
> +
> + check_opt(opts, index, false);
> /* empty strings might just return a default value */
> if (!str || *str == '\0') {
> if (sp->defaultval == SUBOPT_NEEDS_VAL)
> @@ -1543,6 +1576,26 @@ getnum(
> return c;
> }
>
> +/*
> + * Option is a string - do all the option table work, and check there
> + * is actually an option string. Otherwise we don't do anything with the string
> + * here - validation will be done later when the string is converted to a value
> + * or used as a file/device path.
> + */
> +static char *
> +getstr(
> + char *str,
> + struct opt_params *opts,
> + int index)
> +{
> + check_opt(opts, index, true);
> +
> + /* empty strings for string options are not valid */
> + if (!str || *str == '\0')
> + reqval(opts->name, (char **)opts->subopts, index);
> + return str;
> +}
> +
> int
> main(
> int argc,
> @@ -1738,18 +1791,10 @@ main(
> xi.dcreat = 1;
> break;
> case D_NAME:
> - if (!value || *value == '\0')
> - reqval('d', subopts, D_NAME);
> - if (xi.dname)
> - respec('d', subopts, D_NAME);
> - xi.dname = value;
> + xi.dname = getstr(value, &dopts, D_NAME);
> break;
> case D_SIZE:
> - if (!value || *value == '\0')
> - reqval('d', subopts, D_SIZE);
> - if (dsize)
> - respec('d', subopts, D_SIZE);
> - dsize = value;
> + dsize = getstr(value, &dopts, D_SIZE);
> break;
> case D_SUNIT:
> dsunit = getnum(value, &dopts, D_SUNIT);
> @@ -1890,18 +1935,10 @@ main(
> break;
> case L_NAME:
> case L_DEV:
> - if (laflag)
> - conflict('l', subopts, L_AGNUM, L_DEV);
> - if (liflag)
> - conflict('l', subopts, L_INTERNAL, L_DEV);
> - if (!value || *value == '\0')
> - reqval('l', subopts, L_NAME);
> - if (xi.logname)
> - respec('l', subopts, L_NAME);
> + logfile = getstr(value, &lopts, L_NAME);
> + xi.logname = logfile;
> ldflag = 1;
> loginternal = 0;
> - logfile = value;
> - xi.logname = value;
> break;
> case L_VERSION:
> sb_feat.log_version =
> @@ -1909,12 +1946,7 @@ main(
> lvflag = 1;
> break;
> case L_SIZE:
> - if (!value || *value == '\0')
> - reqval('l', subopts, L_SIZE);
> - if (logsize)
> - respec('l', subopts, L_SIZE);
> - logsize = value;
> - lsflag = 1;
> + logsize = getstr(value, &lopts, L_SIZE);
> break;
> case L_SECTLOG:
> lsectorlog = getnum(value, &lopts,
> @@ -2000,10 +2032,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> nsflag = 1;
> break;
> case N_VERSION:
> - if (!value || *value == '\0')
> - reqval('n', subopts, N_VERSION);
> - if (nvflag)
> - respec('n', subopts, N_VERSION);
> + value = getstr(value, &nopts, N_VERSION);
> if (!strcasecmp(value, "ci")) {
> /* ASCII CI mode */
> sb_feat.nci = true;
> @@ -2052,11 +2081,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> switch (getsubopt(&p, (constpp)subopts,
> &value)) {
> case R_EXTSIZE:
> - if (!value || *value == '\0')
> - reqval('r', subopts, R_EXTSIZE);
> - if (rtextsize)
> - respec('r', subopts, R_EXTSIZE);
> - rtextsize = value;
> + rtextsize = getstr(value, &ropts,
> + R_EXTSIZE);
> break;
> case R_FILE:
> xi.risfile = getnum(value, &ropts,
> @@ -2066,18 +2092,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> break;
> case R_NAME:
> case R_DEV:
> - if (!value || *value == '\0')
> - reqval('r', subopts, R_NAME);
> - if (xi.rtname)
> - respec('r', subopts, R_NAME);
> - xi.rtname = value;
> + xi.rtname = getstr(value, &ropts,
> + R_NAME);
> break;
> case R_SIZE:
> - if (!value || *value == '\0')
> - reqval('r', subopts, R_SIZE);
> - if (rtsize)
> - respec('r', subopts, R_SIZE);
> - rtsize = value;
> + rtsize = getstr(value, &ropts, R_SIZE);
> break;
> case R_NOALIGN:
> norsflag = getnum(value, &ropts,
> @@ -2137,13 +2156,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> fprintf(stderr, _("extra arguments\n"));
> usage();
> } else if (argc - optind == 1) {
> - dfile = xi.volname = argv[optind];
> - if (xi.dname) {
> - fprintf(stderr,
> - _("cannot specify both %s and -d name=%s\n"),
> - xi.volname, xi.dname);
> - usage();
> - }
> + dfile = xi.volname = getstr(argv[optind], &dopts, D_NAME);
> } else
> dfile = xi.dname;
>
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
