Re: [PATCH 14/17] mkfs: add string options to generic parsing

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:02:03PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  mkfs/xfs_mkfs.c | 127 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 70 insertions(+), 57 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1d80188..6bfa73c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -68,6 +68,7 @@ struct opt_params {
>  	struct subopt_param {
>  		int		index;
>  		bool		seen;
> +		bool		str_seen;
>  		bool		convert;
>  		bool		is_power_2;
>  		int		conflicts[MAX_CONFLICTS];
> @@ -1412,14 +1413,15 @@ illegal_option(
>  	usage();
>  }
>  
> -static long long
> -getnum(
> +static void
> +check_opt(
>  	const char		*str,
>  	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,
> @@ -1428,21 +1430,46 @@ 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(str, opts, index, false);
>  	/* empty strings might just return a default value */
>  	if (!str || *str == '\0') {
>  		if (sp->defaultval == SUBOPT_NEEDS_VAL)
> @@ -1476,6 +1503,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(str, 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,
> @@ -1670,18 +1717,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);
> @@ -1822,18 +1861,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 =
> @@ -1841,12 +1872,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,
> @@ -1930,10 +1956,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;
> @@ -1982,11 +2005,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,
> @@ -1996,18 +2016,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 = 1;
> -- 
> 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