Re: [PATCH 07/17] mkfs: structify input parameter passing

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:01:56PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Passing large number of parameters around to number conversion
> functions is painful. Add a structure to encapsulate the constant
> parameters that are passed, and convert getnum_checked to use it.
> 
> This is the first real step towards a table driven option parser.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 602 ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 392 insertions(+), 210 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6f0aa55..d3d1e11 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -52,145 +52,322 @@ static int  ispow2(unsigned int i);
>  static long long cvtnum(unsigned int blocksize,
>  			unsigned int sectorsize, const char *s);
>  
...
>  
> -char	*iopts[] = {
> +
> +const struct opt_params iopts = {
> +	.name = 'i',
> +	.subopts = {
>  #define	I_ALIGN		0
> -	"align",
> +		"align",
>  #define	I_LOG		1
> -	"log",
> +		"log",
>  #define	I_MAXPCT	2
> -	"maxpct",
> +		"maxpct",
>  #define	I_PERBLOCK	3
> -	"perblock",
> +		"perblock",
>  #define	I_SIZE		4
> -	"size",
> +		"size",
>  #define	I_ATTR		5
> -	"attr",
> +		"attr",
>  #define	I_PROJID32BIT	6
>  	"projid32bit",
>  #define I_SPINODES	7
>  	"sparse",
>  	NULL

Nit... the last few strings in the above aren't aligned properly. Looks
like they weren't touched, so perhaps just an artifact of forward
porting the series.

> +	},
> +	.subopt_params = {
> +		{ .index = I_ALIGN,
> +		},
> +		{ .index = I_LOG,
> +		  .minval = XFS_DINODE_MIN_LOG,
> +		  .maxval = XFS_DINODE_MAX_LOG,
> +		},
> +		{ .index = I_MAXPCT,
> +		},
> +		{ .index = I_PERBLOCK,
> +		},
> +		{ .index = I_SIZE,
> +		},
> +		{ .index = I_ATTR,
> +		},
> +		{ .index = I_PROJID32BIT,
> +		},

Missing I_SPINODES here.

> +	},
>  };
>  
...
> +
>  static int
>  getnum_checked(
>  	const char	*str,
> -	long long	min_val,
> -	long long	max_val,
> -	const char	*illegal_str,
> -	char		reqval_char,
> -	char		*reqval_opts[],
> -	int		reqval_optind)
> +	const struct opt_params	*opts,
> +	int		index)
>  {
>  	long long	c;
>  
>  	if (!str || *str == '\0')
> -		reqval(reqval_char, reqval_opts, reqval_optind);
> +		reqval(opts->name, (char **)opts->subopts, index);
>  
>  	c = getnum(str, 0, 0, false);
> -	if (c < min_val || c > max_val)
> -		illegal(str, illegal_str);
> +	if (c < opts->subopt_params[index].minval ||
> +	    c > opts->subopt_params[index].maxval)
> +		illegal_option(str, opts, index);

Some kind of error, assert or updated logic to ensure minval and maxval
are specified would be nice here so failures to use the correct
validation function (or to specify the values) are more obvious.

>  	return c;
>  }
>  
...
> @@ -1662,9 +1842,11 @@ main(
>  		case 'm':
>  			p = optarg;
>  			while (*p != '\0') {
> +				char	**subopts = (char **)mopts.subopts;
>  				char	*value;
>  
> -				switch (getsubopt(&p, (constpp)mopts, &value)) {
> +				switch (getsubopt(&p, (constpp)subopts,
> +						  &value)) {
>  				case M_CRC:
>  					sb_feat.crcs_enabled = getbool(
>  							value, "m crc", false);
> @@ -1678,7 +1860,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					break;
>  				case M_FINOBT:
>  					if (!value || *value == '\0')
> -						reqval('m', mopts, M_CRC);
> +						reqval('m', (char**)mopts.subopts, M_CRC);

Can probably use subopts here..?

Brian

>  					c = atoi(value);
>  					if (c < 0 || c > 1)
>  						illegal(value, "m finobt");
> @@ -1692,30 +1874,29 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  		case 'n':
>  			p = optarg;
>  			while (*p != '\0') {
> +				char	**subopts = (char **)nopts.subopts;
>  				char	*value;
>  
> -				switch (getsubopt(&p, (constpp)nopts, &value)) {
> +				switch (getsubopt(&p, (constpp)subopts,
> +						  &value)) {
>  				case N_LOG:
>  					if (nlflag)
> -						respec('n', nopts, N_LOG);
> +						respec('n', subopts, N_LOG);
>  					if (nsflag)
> -						conflict('n', nopts, N_SIZE,
> +						conflict('n', subopts, N_SIZE,
>  							 N_LOG);
>  					dirblocklog = getnum_checked(value,
> -							XFS_MIN_REC_DIRSIZE,
> -							XFS_MAX_BLOCKSIZE_LOG,
> -							"n log", 'n', nopts,
> -							N_LOG);
> +								&nopts, N_LOG);
>  					dirblocksize = 1 << dirblocklog;
>  					nlflag = 1;
>  					break;
>  				case N_SIZE:
>  					if (!value || *value == '\0')
> -						reqval('n', nopts, N_SIZE);
> +						reqval('n', subopts, N_SIZE);
>  					if (nsflag)
> -						respec('n', nopts, N_SIZE);
> +						respec('n', subopts, N_SIZE);
>  					if (nlflag)
> -						conflict('n', nopts, N_LOG,
> +						conflict('n', subopts, N_LOG,
>  							 N_SIZE);
>  					dirblocksize = getnum(value, blocksize,
>  							      sectorsize, true);
> @@ -1728,9 +1909,9 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					break;
>  				case N_VERSION:
>  					if (!value || *value == '\0')
> -						reqval('n', nopts, N_VERSION);
> +						reqval('n', subopts, N_VERSION);
>  					if (nvflag)
> -						respec('n', nopts, N_VERSION);
> +						respec('n', subopts, N_VERSION);
>  					if (!strcasecmp(value, "ci")) {
>  						/* ASCII CI mode */
>  						sb_feat.nci = true;
> @@ -1745,7 +1926,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					break;
>  				case N_FTYPE:
>  					if (nftype)
> -						respec('n', nopts, N_FTYPE);
> +						respec('n', subopts, N_FTYPE);
>  					if (sb_feat.crcs_enabled) {
>  						fprintf(stderr,
>  _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -1777,14 +1958,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  		case 'r':
>  			p = optarg;
>  			while (*p != '\0') {
> +				char	**subopts = (char **)ropts.subopts;
>  				char	*value;
>  
> -				switch (getsubopt(&p, (constpp)ropts, &value)) {
> +				switch (getsubopt(&p, (constpp)subopts,
> +						  &value)) {
>  				case R_EXTSIZE:
>  					if (!value || *value == '\0')
> -						reqval('r', ropts, R_EXTSIZE);
> +						reqval('r', subopts, R_EXTSIZE);
>  					if (rtextsize)
> -						respec('r', ropts, R_EXTSIZE);
> +						respec('r', subopts, R_EXTSIZE);
>  					rtextsize = value;
>  					break;
>  				case R_FILE:
> @@ -1796,16 +1979,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  				case R_NAME:
>  				case R_DEV:
>  					if (!value || *value == '\0')
> -						reqval('r', ropts, R_NAME);
> +						reqval('r', subopts, R_NAME);
>  					if (xi.rtname)
> -						respec('r', ropts, R_NAME);
> +						respec('r', subopts, R_NAME);
>  					xi.rtname = value;
>  					break;
>  				case R_SIZE:
>  					if (!value || *value == '\0')
> -						reqval('r', ropts, R_SIZE);
> +						reqval('r', subopts, R_SIZE);
>  					if (rtsize)
> -						respec('r', ropts, R_SIZE);
> +						respec('r', subopts, R_SIZE);
>  					rtsize = value;
>  					break;
>  				case R_NOALIGN:
> @@ -1819,21 +2002,20 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  		case 's':
>  			p = optarg;
>  			while (*p != '\0') {
> +				char	**subopts = (char **)sopts.subopts;
>  				char	*value;
>  
> -				switch (getsubopt(&p, (constpp)sopts, &value)) {
> +				switch (getsubopt(&p, (constpp)subopts,
> +						  &value)) {
>  				case S_LOG:
>  				case S_SECTLOG:
>  					if (slflag || lslflag)
> -						respec('s', sopts, S_SECTLOG);
> +						respec('s', subopts, S_SECTLOG);
>  					if (ssflag || lssflag)
> -						conflict('s', sopts, S_SECTSIZE,
> -							 S_SECTLOG);
> -					sectorlog = getnum_checked(value,
> -							XFS_MIN_SECTORSIZE_LOG,
> -							XFS_MAX_SECTORSIZE_LOG,
> -							"s sectlog", 's', sopts,
> -							S_SECTLOG);
> +						conflict('s', subopts,
> +							 S_SECTSIZE, S_SECTLOG);
> +					sectorlog = getnum_checked(value, &sopts,
> +								   S_SECTLOG);
>  					lsectorlog = sectorlog;
>  					sectorsize = 1 << sectorlog;
>  					lsectorsize = sectorsize;
> @@ -1842,11 +2024,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  				case S_SIZE:
>  				case S_SECTSIZE:
>  					if (!value || *value == '\0')
> -						reqval('s', sopts, S_SECTSIZE);
> +						reqval('s', subopts, S_SECTSIZE);
>  					if (ssflag || lssflag)
> -						respec('s', sopts, S_SECTSIZE);
> +						respec('s', subopts, S_SECTSIZE);
>  					if (slflag || lslflag)
> -						conflict('s', sopts, S_SECTLOG,
> +						conflict('s', subopts, S_SECTLOG,
>  							 S_SECTSIZE);
>  					sectorsize = getnum(value, blocksize,
>  							    sectorsize, true);
> -- 
> 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