Re: [PATCH 10/17] mkfs: add respecification detection to generic parsing

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:01:59PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Add flags to the generic input parameter tables so that
> respecification can be detected in a generic manner.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 68 +++++++++++++++------------------------------------------
>  1 file changed, 18 insertions(+), 50 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 332c491..0bd8998 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -57,15 +57,17 @@ static long long cvtnum(unsigned int blocksize,
>  struct opt_params {
>  	const char	name;
>  	const char	*subopts[MAX_SUBOPTS];
> +
>  	struct subopt_param {
>  		int		index;
> +		bool		seen;
>  		long long	minval;
>  		long long	maxval;
>  		long long	defaultval;
>  	}		subopt_params[MAX_SUBOPTS];
>  };
>  
> -const struct opt_params bopts = {
> +struct opt_params bopts = {
>  	.name = 'b',
>  	.subopts = {
>  #define	B_LOG		0
> @@ -88,7 +90,7 @@ const struct opt_params bopts = {
>  	},
>  };
>  
> -const struct opt_params dopts = {
> +struct opt_params dopts = {
>  	.name = 'd',
>  	.subopts = {
>  #define	D_AGCOUNT	0
> @@ -197,7 +199,7 @@ const struct opt_params dopts = {
>  };
>  
>  
> -const struct opt_params iopts = {
> +struct opt_params iopts = {
>  	.name = 'i',
>  	.subopts = {
>  #define	I_ALIGN		0
> @@ -257,7 +259,7 @@ const struct opt_params iopts = {
>  	},
>  };
>  
> -const struct opt_params lopts = {
> +struct opt_params lopts = {
>  	.name = 'l',
>  	.subopts = {
>  #define	L_AGNUM		0
> @@ -342,7 +344,7 @@ const struct opt_params lopts = {
>  	},
>  };
>  
> -const struct opt_params nopts = {
> +struct opt_params nopts = {
>  	.name = 'n',
>  	.subopts = {
>  #define	N_LOG		0
> @@ -379,7 +381,7 @@ const struct opt_params nopts = {
>  	},
>  };
>  
> -const struct opt_params ropts = {
> +struct opt_params ropts = {
>  	.name = 'r',
>  	.subopts = {
>  #define	R_EXTSIZE	0
> @@ -418,7 +420,7 @@ const struct opt_params ropts = {
>  	},
>  };
>  
> -const struct opt_params sopts = {
> +struct opt_params sopts = {

It would be nice to avoid all this noise by dropping the const
specifiers from the earlier patch. Otherwise, looks fine:

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

>  	.name = 's',
>  	.subopts = {
>  #define	S_LOG		0
> @@ -455,7 +457,7 @@ const struct opt_params sopts = {
>  	},
>  };
>  
> -const struct opt_params mopts = {
> +struct opt_params mopts = {
>  	.name = 'm',
>  	.subopts = {
>  #define	M_CRC		0
> @@ -1192,7 +1194,6 @@ struct sb_feat_args {
>  	int	dir_version;
>  	int	spinodes;
>  	int	finobt;
> -	bool	finobtflag;
>  	bool	inode_align;
>  	bool	nci;
>  	bool	lazy_sb_counters;
> @@ -1323,10 +1324,10 @@ illegal_option(
>  static int
>  getnum_checked(
>  	const char		*str,
> -	const struct opt_params	*opts,
> +	struct opt_params	*opts,
>  	int			index)
>  {
> -	const struct subopt_param *sp = &opts->subopt_params[index];
> +	struct subopt_param *sp = &opts->subopt_params[index];
>  	long long		c;
>  
>  	if (sp->index != index) {
> @@ -1336,6 +1337,11 @@ getnum_checked(
>  		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;
> +
>  	if (!str || *str == '\0') {
>  		if (sp->defaultval == SUBOPT_NEEDS_VAL)
>  			reqval(opts->name, (char **)opts->subopts, index);
> @@ -1446,7 +1452,6 @@ main(
>  	struct fs_topology	ft;
>  	struct sb_feat_args	sb_feat = {
>  		.finobt = 1,
> -		.finobtflag = false,
>  		.spinodes = 0,
>  		.log_version = 2,
>  		.attr_version = 2,
> @@ -1506,8 +1511,6 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case B_LOG:
> -					if (blflag)
> -						respec('b', subopts, B_LOG);
>  					if (bsflag)
>  						conflict('b', subopts, B_SIZE,
>  							 B_LOG);
> @@ -1546,9 +1549,6 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case D_AGCOUNT:
> -					if (daflag)
> -						respec('d', subopts, D_AGCOUNT);
> -
>  					agcount = getnum_checked(value, &dopts,
>  								 D_AGCOUNT);
>  					daflag = 1;
> @@ -1585,8 +1585,6 @@ main(
>  					dsize = value;
>  					break;
>  				case D_SUNIT:
> -					if (dsunit)
> -						respec('d', subopts, D_SUNIT);
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SUNIT);
> @@ -1594,8 +1592,6 @@ main(
>  								 D_SUNIT);
>  					break;
>  				case D_SWIDTH:
> -					if (dswidth)
> -						respec('d', subopts, D_SWIDTH);
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SWIDTH);
> @@ -1616,8 +1612,6 @@ main(
>  						illegal(value, "d su");
>  					break;
>  				case D_SW:
> -					if (dsw)
> -						respec('d', subopts, D_SW);
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SW);
> @@ -1640,8 +1634,6 @@ main(
>  					nodsflag = 1;
>  					break;
>  				case D_SECTLOG:
> -					if (slflag)
> -						respec('d', subopts, D_SECTLOG);
>  					if (ssflag)
>  						conflict('d', subopts, D_SECTSIZE,
>  							 D_SECTLOG);
> @@ -1704,8 +1696,6 @@ main(
>  							value, &iopts, I_ALIGN);
>  					break;
>  				case I_LOG:
> -					if (ilflag)
> -						respec('i', subopts, I_LOG);
>  					if (ipflag)
>  						conflict('i', subopts, I_PERBLOCK,
>  							 I_LOG);
> @@ -1718,8 +1708,6 @@ main(
>  					ilflag = 1;
>  					break;
>  				case I_MAXPCT:
> -					if (imflag)
> -						respec('i', subopts, I_MAXPCT);
>  					imaxpct = getnum_checked(value, &iopts,
>  								 I_MAXPCT);
>  					imflag = 1;
> @@ -1728,8 +1716,6 @@ main(
>  					if (ilflag)
>  						conflict('i', subopts, I_LOG,
>  							 I_PERBLOCK);
> -					if (ipflag)
> -						respec('i', subopts, I_PERBLOCK);
>  					if (isflag)
>  						conflict('i', subopts, I_SIZE,
>  							 I_PERBLOCK);
> @@ -1746,8 +1732,6 @@ main(
>  					if (ipflag)
>  						conflict('i', subopts, I_PERBLOCK,
>  							 I_SIZE);
> -					if (isflag)
> -						respec('i', subopts, I_SIZE);
>  					isize = getnum_checked(value, &iopts,
>  							       I_SIZE);
>  					if (!ispow2(isize))
> @@ -1786,8 +1770,6 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case L_AGNUM:
> -					if (laflag)
> -						respec('l', subopts, L_AGNUM);
>  					if (ldflag)
>  						conflict('l', subopts, L_AGNUM, L_DEV);
>  					logagno = getnum_checked(value, &lopts,
> @@ -1809,8 +1791,6 @@ main(
>  					if (xi.lisfile)
>  						conflict('l', subopts, L_FILE,
>  							 L_INTERNAL);
> -					if (liflag)
> -						respec('l', subopts, L_INTERNAL);
>  
>  					loginternal = getnum_checked(value,
>  							&lopts, L_INTERNAL);
> @@ -1828,8 +1808,6 @@ main(
>  					lsuflag = 1;
>  					break;
>  				case L_SUNIT:
> -					if (lsunit)
> -						respec('l', subopts, L_SUNIT);
>  					lsunit = getnum_checked(value, &lopts,
>  								 L_SUNIT);
>  					lsunitflag = 1;
> @@ -1850,8 +1828,6 @@ main(
>  					xi.logname = value;
>  					break;
>  				case L_VERSION:
> -					if (lvflag)
> -						respec('l', subopts, L_VERSION);
>  					sb_feat.log_version =
>  						getnum_checked(value, &lopts,
>  							       L_VERSION);
> @@ -1866,8 +1842,6 @@ main(
>  					lsflag = 1;
>  					break;
>  				case L_SECTLOG:
> -					if (lslflag)
> -						respec('l', subopts, L_SECTLOG);
>  					if (lssflag)
>  						conflict('l', subopts, L_SECTSIZE,
>  							 L_SECTLOG);
> @@ -1950,8 +1924,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case N_LOG:
> -					if (nlflag)
> -						respec('n', subopts, N_LOG);
>  					if (nsflag)
>  						conflict('n', subopts, N_SIZE,
>  							 N_LOG);
> @@ -1994,8 +1966,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					nvflag = 1;
>  					break;
>  				case N_FTYPE:
> -					if (nftype)
> -						respec('n', subopts, N_FTYPE);
>  					if (sb_feat.crcs_enabled) {
>  						fprintf(stderr,
>  _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -2078,8 +2048,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  						  &value)) {
>  				case S_LOG:
>  				case S_SECTLOG:
> -					if (slflag || lslflag)
> -						respec('s', subopts, S_SECTLOG);
>  					if (ssflag || lssflag)
>  						conflict('s', subopts,
>  							 S_SECTSIZE, S_SECTLOG);
> @@ -2296,7 +2264,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
>  		 * tried to use crc=0,finobt=1, then issue a warning before
>  		 * turning them off.
>  		 */
> -		if (sb_feat.finobt && sb_feat.finobtflag) {
> +		if (sb_feat.finobt && mopts.subopt_params[M_FINOBT].seen) {
>  			fprintf(stderr,
>  _("warning: finobt not supported without CRC support, disabled.\n"));
>  			sb_feat.finobt = 0;
> -- 
> 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