All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: "Jan Ťulák" <jtulak@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>, xfs@oss.sgi.com
Subject: Re: [PATCH 08/17] mkfs: getbool is redundant
Date: Fri, 26 Jun 2015 13:17:01 -0400	[thread overview]
Message-ID: <20150626171701.GD40750@bfoster.bfoster> (raw)
In-Reply-To: <1434711726-13092-9-git-send-email-jtulak@redhat.com>

On Fri, Jun 19, 2015 at 01:01:57PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> getbool() can be replaced with getnum_checked with appropriate
> min/max values set for the boolean variables.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 147 ++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 106 insertions(+), 41 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d3d1e11..3803779 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -53,6 +53,7 @@ static long long cvtnum(unsigned int blocksize,
>  			unsigned int sectorsize, const char *s);
>  
>  #define MAX_SUBOPTS	16
> +#define SUBOPT_NEEDS_VAL	(-1LL)
>  struct opt_params {
>  	const char	name;
>  	const char	*subopts[MAX_SUBOPTS];
> @@ -60,6 +61,7 @@ struct opt_params {
>  		int		index;
>  		long long	minval;
>  		long long	maxval;
> +		long long	defaultval;
>  	}		subopt_params[MAX_SUBOPTS];
>  };
>  
> @@ -76,10 +78,12 @@ const struct opt_params bopts = {
>  		{ .index = B_LOG,
>  		  .minval = XFS_MIN_BLOCKSIZE_LOG,
>  		  .maxval = XFS_MAX_BLOCKSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = B_SIZE,
>  		  .minval = XFS_MIN_BLOCKSIZE,
>  		  .maxval = XFS_MAX_BLOCKSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  	},
>  };
> @@ -121,38 +125,55 @@ const struct opt_params dopts = {
>  	},
>  	.subopt_params = {
>  		{ .index = D_AGCOUNT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_FILE,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,
>  		},
>  		{ .index = D_NAME,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SUNIT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SWIDTH,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_AGSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SU,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SW,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SECTLOG,
>  		  .minval = XFS_MIN_SECTORSIZE_LOG,
>  		  .maxval = XFS_MAX_SECTORSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_SECTSIZE,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_NOALIGN,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_RTINHERIT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},

I don't think the above two require values.

>  		{ .index = D_PROJINHERIT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = D_EXTSZINHERIT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  	},
>  };
> @@ -181,20 +202,31 @@ const struct opt_params iopts = {
>  	},
>  	.subopt_params = {
>  		{ .index = I_ALIGN,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,
>  		},
>  		{ .index = I_LOG,
>  		  .minval = XFS_DINODE_MIN_LOG,
>  		  .maxval = XFS_DINODE_MAX_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_MAXPCT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_PERBLOCK,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_SIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_ATTR,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = I_PROJID32BIT,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,

Need to handle sparse here too once it's added to the previous patch.

>  		},
>  	},
>  };
> @@ -230,32 +262,50 @@ const struct opt_params lopts = {
>  	},
>  	.subopt_params = {
>  		{ .index = L_AGNUM,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_INTERNAL,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,
>  		},
>  		{ .index = L_SIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_VERSION,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SUNIT,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SU,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_DEV,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SECTLOG,
>  		  .minval = XFS_MIN_SECTORSIZE_LOG,
>  		  .maxval = XFS_MAX_SECTORSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_SECTSIZE,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_FILE,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,
>  		},
>  		{ .index = L_NAME,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = L_LAZYSBCNTR,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,
>  		},
>  	},
>  };
> @@ -277,14 +327,20 @@ const struct opt_params nopts = {
>  		{ .index = N_LOG,
>  		  .minval = XFS_MIN_REC_DIRSIZE,
>  		  .maxval = XFS_MAX_BLOCKSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = N_SIZE,
>  		  .minval = 1 << XFS_MIN_REC_DIRSIZE,
>  		  .maxval = XFS_MAX_BLOCKSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = N_VERSION,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = N_FTYPE,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 0,
>  		},
>  	},
>  };
> @@ -308,16 +364,22 @@ const struct opt_params ropts = {
>  	},
>  	.subopt_params = {
>  		{ .index = R_EXTSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_SIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_DEV,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_FILE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,

This should have min/max/default similar to D_FILE, yes?

>  		},
>  		{ .index = R_NAME,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_NOALIGN,
> +		  .defaultval = SUBOPT_NEEDS_VAL,

Not sure about this one, either. There's no value according to the man
page.

>  		},
>  	},
>  };
> @@ -339,18 +401,22 @@ const struct opt_params sopts = {
>  		{ .index = S_LOG,
>  		  .minval = XFS_MIN_SECTORSIZE_LOG,
>  		  .maxval = XFS_MAX_SECTORSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = S_SECTLOG,
>  		  .minval = XFS_MIN_SECTORSIZE_LOG,
>  		  .maxval = XFS_MAX_SECTORSIZE_LOG,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = S_SIZE,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = S_SECTSIZE,
>  		  .minval = XFS_MIN_SECTORSIZE,
>  		  .maxval = XFS_MAX_SECTORSIZE,
> +		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  	},
>  };
> @@ -366,6 +432,9 @@ const struct opt_params mopts = {
>  	},
>  	.subopt_params = {
>  		{ .index = M_CRC,
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 0,
>  		},
>  	},

How about M_FINOBT?

Brian

>  };
> @@ -1205,22 +1274,6 @@ 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;
> -}
> -
>  static __attribute__((noreturn)) void
>  illegal_option(
>  	const char	*value,
> @@ -1235,18 +1288,28 @@ illegal_option(
>  
>  static int
>  getnum_checked(
> -	const char	*str,
> +	const char		*str,
>  	const struct opt_params	*opts,
> -	int		index)
> +	int			index)
>  {
> -	long long	c;
> +	const struct subopt_param *sp = &opts->subopt_params[index];
> +	long long		c;
>  
> -	if (!str || *str == '\0')
> +	if (sp->index != index) {
> +		fprintf(stderr,
> +	("Developer screwed up option parsing (%d/%d)! Please report!\n"),
> +			sp->index, index);
>  		reqval(opts->name, (char **)opts->subopts, index);
> +	}
> +
> +	if (!str || *str == '\0') {
> +		if (sp->defaultval == SUBOPT_NEEDS_VAL)
> +			reqval(opts->name, (char **)opts->subopts, index);
> +		return sp->defaultval;
> +	}
>  
>  	c = getnum(str, 0, 0, false);
> -	if (c < opts->subopt_params[index].minval ||
> -	    c > opts->subopt_params[index].maxval)
> +	if (c < sp->minval || c > sp->maxval)
>  		illegal_option(str, opts, index);
>  	return c;
>  }
> @@ -1470,8 +1533,8 @@ main(
>  					dasize = 1;
>  					break;
>  				case D_FILE:
> -					xi.disfile = getbool(value, "d file",
> -							     true);
> +					xi.disfile = getnum_checked(value,
> +								&dopts, D_FILE);
>  					if (xi.disfile && !Nflag)
>  						xi.dcreat = 1;
>  					break;
> @@ -1613,8 +1676,8 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case I_ALIGN:
> -					sb_feat.inode_align = getbool(
> -							value, "i align", true);
> +					sb_feat.inode_align = getnum_checked(
> +							value, &iopts, I_ALIGN);
>  					break;
>  				case I_LOG:
>  					if (ilflag)
> @@ -1684,8 +1747,9 @@ main(
>  					sb_feat.attr_version = c;
>  					break;
>  				case I_PROJID32BIT:
> -					sb_feat.projid16bit = !getbool(value,
> -							"i projid32bit", false);
> +					sb_feat.projid16bit =
> +						!getnum_checked(value, &iopts,
> +								I_PROJID32BIT);
>  					break;
>  				case I_SPINODES:
>  					if (!value || *value == '\0')
> @@ -1723,8 +1787,8 @@ main(
>  					if (loginternal)
>  						conflict('l', subopts, L_INTERNAL,
>  							 L_FILE);
> -					xi.lisfile = getbool(value, "l file",
> -							     true);
> +					xi.lisfile = getnum_checked(value,
> +								&lopts, L_FILE);
>  					if (xi.lisfile)
>  						xi.lcreat = 1;
>  					break;
> @@ -1737,8 +1801,8 @@ main(
>  					if (liflag)
>  						respec('l', subopts, L_INTERNAL);
>  
> -					loginternal = getbool(value,
> -							"l internal", true);
> +					loginternal = getnum_checked(value,
> +							&lopts, L_INTERNAL);
>  					liflag = 1;
>  					break;
>  				case L_SU:
> @@ -1825,9 +1889,9 @@ main(
>  					lssflag = 1;
>  					break;
>  				case L_LAZYSBCNTR:
> -					sb_feat.lazy_sb_counters = getbool(
> -							value, "l lazy-count",
> -							true);
> +					sb_feat.lazy_sb_counters =
> +						getnum_checked(value, &lopts,
> +							       L_LAZYSBCNTR);
>  					break;
>  				default:
>  					unknown('l', value);
> @@ -1848,8 +1912,9 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case M_CRC:
> -					sb_feat.crcs_enabled = getbool(
> -							value, "m crc", false);
> +					sb_feat.crcs_enabled =
> +						getnum_checked(value, &mopts,
> +							       M_CRC);
>  					if (sb_feat.crcs_enabled && nftype) {
>  						fprintf(stderr,
>  _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -1932,8 +1997,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  _("cannot specify both -m crc=1 and -n ftype\n"));
>  						usage();
>  					}
> -					sb_feat.dirftype = getbool(value,
> -							     "n ftype", false);
> +					sb_feat.dirftype = getnum_checked(value,
> +								&nopts, N_FTYPE);
>  					nftype = 1;
>  					break;
>  				default:
> @@ -1971,8 +2036,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					rtextsize = value;
>  					break;
>  				case R_FILE:
> -					xi.risfile = getbool(value,
> -							     "r file", true);
> +					xi.risfile = getnum_checked(value,
> +								&ropts, R_FILE);
>  					if (xi.risfile)
>  						xi.rcreat = 1;
>  					break;
> -- 
> 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

  reply	other threads:[~2015-06-26 17:17 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-19 11:01 [PATCH 00/17] mkfs: sanitise input parameters Jan Ťulák
2015-06-19 11:01 ` [PATCH 01/17] xfsprogs: use common code for multi-disk detection Jan Ťulák
2015-06-19 11:10   ` Christoph Hellwig
2015-06-19 11:51     ` Jan Tulak
2015-06-25 19:37   ` Brian Foster
2015-07-02 12:47     ` Jan Tulak
2015-07-02 14:14       ` Brian Foster
2015-07-02 23:05         ` Dave Chinner
2015-07-03 13:22           ` Brian Foster
2015-07-08 16:14           ` Jan Tulak
2015-07-09  0:45             ` Dave Chinner
2015-07-09  8:24               ` Jan Tulak
2015-07-03 10:06         ` Jan Tulak
2015-06-19 11:01 ` [PATCH 02/17] mkfs: sanitise ftype parameter values Jan Ťulák
2015-06-25 19:37   ` Brian Foster
2015-06-19 11:01 ` [PATCH 03/17] mkfs: Sanitise the superblock feature macros Jan Ťulák
2015-06-25 19:38   ` Brian Foster
2015-07-03  9:53     ` Jan Tulak
2015-07-03 13:24       ` Brian Foster
2015-06-19 11:01 ` [PATCH 04/17] mkfs: validate all input values Jan Ťulák
2015-06-25 19:38   ` Brian Foster
2015-06-19 11:01 ` [PATCH 05/17] mkfs: factor boolean option parsing Jan Ťulák
2015-06-25 19:38   ` Brian Foster
2015-06-19 11:01 ` [PATCH 06/17] mkfs: validate logarithmic parameters sanely Jan Ťulák
2015-06-26 17:16   ` Brian Foster
2015-06-19 11:01 ` [PATCH 07/17] mkfs: structify input parameter passing Jan Ťulák
2015-06-26 17:16   ` Brian Foster
2015-06-19 11:01 ` [PATCH 08/17] mkfs: getbool is redundant Jan Ťulák
2015-06-26 17:17   ` Brian Foster [this message]
2015-06-30  1:32     ` Dave Chinner
2015-06-19 11:01 ` [PATCH 09/17] mkfs: use getnum_checked for all ranged parameters Jan Ťulák
2015-06-26 17:17   ` Brian Foster
2015-06-19 11:01 ` [PATCH 10/17] mkfs: add respecification detection to generic parsing Jan Ťulák
2015-06-26 17:17   ` Brian Foster
2015-06-19 11:02 ` [PATCH 11/17] mkfs: table based parsing for converted parameters Jan Ťulák
2015-06-26 17:17   ` Brian Foster
2015-06-19 11:02 ` [PATCH 12/17] mkfs: merge getnum Jan Ťulák
2015-06-26 17:17   ` Brian Foster
2015-06-19 11:02 ` [PATCH 13/17] mkfs: encode conflicts into parsing table Jan Ťulák
2015-06-26 17:17   ` Brian Foster
2015-06-30  3:57     ` Dave Chinner
2015-06-30 11:27       ` Brian Foster
2015-07-01  8:30         ` Jan Tulak
2015-06-19 11:02 ` [PATCH 14/17] mkfs: add string options to generic parsing Jan Ťulák
2015-06-26 19:32   ` Brian Foster
2015-06-19 11:02 ` [PATCH 15/17] mkfs: don't treat files as though they are block devices Jan Ťulák
2015-06-26 19:32   ` Brian Foster
2015-06-19 11:02 ` [PATCH 16/17] mkfs fix: handling of files Jan Ťulák
2015-06-26 19:32   ` Brian Foster
2015-06-19 11:02 ` [PATCH 17/17] mkfs: move spinodes crc check Jan Ťulák
2015-06-26 19:32   ` Brian Foster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150626171701.GD40750@bfoster.bfoster \
    --to=bfoster@redhat.com \
    --cc=dchinner@redhat.com \
    --cc=jtulak@redhat.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.