Re: [PATCH 12/17] mkfs: merge getnum

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:02:01PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> getnum() is now only called by getnum_checked(). Move the two
> together into a single getnum() function and change all the callers
> back to getnum().
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  include/xfs_mkfs.h |   4 +-
>  mkfs/proto.c       |  20 ++++++
>  mkfs/xfs_mkfs.c    | 202 ++++++++++++++++++++++++-----------------------------
>  3 files changed, 113 insertions(+), 113 deletions(-)
> 
> diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
> index f5639b0..a4312e1 100644
> --- a/include/xfs_mkfs.h
> +++ b/include/xfs_mkfs.h
> @@ -57,8 +57,8 @@
>  #define XFS_NOMULTIDISK_AGLOG		2	/* 4 AGs */
>  #define XFS_MULTIDISK_AGCOUNT		(1 << XFS_MULTIDISK_AGLOG)
>  
> -extern long long getnum(const char *str, unsigned int blocksize,
> -			unsigned int sectorsize, bool convert);
> +extern long long cvtnum(unsigned int blksize, unsigned int sectsize,
> +			const char *str);
>  
>  /* proto.c */
>  extern char *setup_proto (char *fname);
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index a2a61e0..1e763a0 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -43,6 +43,26 @@ static long filesize(int fd);
>  	((uint)(MKFS_BLOCKRES_INODE + XFS_DA_NODE_MAXDEPTH + \
>  	(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) - 1) + (rb)))
>  
> +static long long
> +getnum(
> +	const char	*str,
> +	unsigned int	blksize,
> +	unsigned int	sectsize,
> +	bool		convert)
> +{
> +	long long	i;
> +	char		*sp;
> +
> +	if (convert)
> +		return cvtnum(blksize, sectsize, str);
> +
> +	i = strtoll(str, &sp, 0);
> +	if (i == 0 && sp == str)
> +		return -1LL;
> +	if (*sp != '\0')
> +		return -1LL; /* trailing garbage */
> +	return i;
> +}
>  
>  char *
>  setup_proto(
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 4fc1f34..e52fd4e 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -49,9 +49,6 @@ static void respec(char opt, char *tab[], int idx);
>  static void unknown(char opt, char *s);
>  static int  ispow2(unsigned int i);
>  
> -static long long cvtnum(unsigned int blocksize,
> -			unsigned int sectorsize, const char *s);
> -
>  /*
>   * The configured block and sector sizes are defined as global variables so
>   * that they don't need to be passed to functions that require them.
> @@ -1330,27 +1327,6 @@ sb_set_features(
>  
>  }
>  
> -long long
> -getnum(
> -	const char	*str,
> -	unsigned int	blksize,
> -	unsigned int	sectsize,
> -	bool		convert)
> -{
> -	long long	i;
> -	char		*sp;
> -
> -	if (convert)
> -		return cvtnum(blksize, sectsize, str);
> -
> -	i = strtoll(str, &sp, 0);
> -	if (i == 0 && sp == str)
> -		return -1LL;
> -	if (*sp != '\0')
> -		return -1LL; /* trailing garbage */
> -	return i;
> -}
> -
>  static __attribute__((noreturn)) void
>  illegal_option(
>  	const char	*value,
> @@ -1363,8 +1339,8 @@ illegal_option(
>  	usage();
>  }
>  
> -static int
> -getnum_checked(
> +static long long
> +getnum(
>  	const char		*str,
>  	struct opt_params	*opts,
>  	int			index)
> @@ -1384,13 +1360,32 @@ getnum_checked(
>  		respec(opts->name, (char **)opts->subopts, index);
>  	sp->seen = true;
>  
> +	/* empty strings might just return a default value */
>  	if (!str || *str == '\0') {
>  		if (sp->defaultval == SUBOPT_NEEDS_VAL)
>  			reqval(opts->name, (char **)opts->subopts, index);
>  		return sp->defaultval;
>  	}
>  
> -	c = getnum(str, blocksize, sectorsize, sp->convert);
> +	/*
> +	 * Some values are pure numbers, others can have suffixes that define
> +	 * the units of the number. Those get passed to cvtnum(), otherwise we
> +	 * convert it ourselves to guarantee there is no trailing garbage in the
> +	 * number.
> +	 */
> +	if (sp->convert)
> +		c = cvtnum(blocksize, sectorsize, str);
> +	else {
> +		char		*sp;
> +
> +		c = strtoll(str, &sp, 0);
> +		if (c == 0 && sp == str)
> +			illegal_option(str, opts, index);
> +		if (*sp != '\0')
> +			illegal_option(str, opts, index);
> +	}
> +

Hrm, why not continue to reuse this code? E.g., give the getnum() from
proto.c a better name and call that. Otherwise, this seems fine.

Brian

> +	/* Validity check the result. */
>  	if (c < sp->minval || c > sp->maxval)
>  		illegal_option(str, opts, index);
>  	if (sp->is_power_2 && !ispow2(c))
> @@ -1556,8 +1551,7 @@ main(
>  					if (bsflag)
>  						conflict('b', subopts, B_SIZE,
>  							 B_LOG);
> -					blocklog = getnum_checked(value, &bopts,
> -								  B_LOG);
> +					blocklog = getnum(value, &bopts, B_LOG);
>  					blocksize = 1 << blocklog;
>  					blflag = 1;
>  					break;
> @@ -1565,8 +1559,8 @@ main(
>  					if (blflag)
>  						conflict('b', subopts, B_LOG,
>  							 B_SIZE);
> -					blocksize = getnum_checked(value, &bopts,
> -								  B_SIZE);
> +					blocksize = getnum(value, &bopts,
> +							   B_SIZE);
>  					blocklog = libxfs_highbit32(blocksize);
>  					bsflag = 1;
>  					break;
> @@ -1584,18 +1578,17 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case D_AGCOUNT:
> -					agcount = getnum_checked(value, &dopts,
> -								 D_AGCOUNT);
> +					agcount = getnum(value, &dopts,
> +							 D_AGCOUNT);
>  					daflag = 1;
>  					break;
>  				case D_AGSIZE:
> -					agsize = getnum_checked(value, &dopts,
> -								 D_AGSIZE);
> +					agsize = getnum(value, &dopts, D_AGSIZE);
>  					dasize = 1;
>  					break;
>  				case D_FILE:
> -					xi.disfile = getnum_checked(value,
> -								&dopts, D_FILE);
> +					xi.disfile = getnum(value, &dopts,
> +							    D_FILE);
>  					if (xi.disfile && !Nflag)
>  						xi.dcreat = 1;
>  					break;
> @@ -1617,29 +1610,26 @@ main(
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SUNIT);
> -					dsunit = getnum_checked(value, &dopts,
> -								 D_SUNIT);
> +					dsunit = getnum(value, &dopts, D_SUNIT);
>  					break;
>  				case D_SWIDTH:
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SWIDTH);
> -					dswidth = getnum_checked(value, &dopts,
> -								 D_SWIDTH);
> +					dswidth = getnum(value, &dopts,
> +							 D_SWIDTH);
>  					break;
>  				case D_SU:
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SU);
> -					dsu = getnum_checked(value, &dopts,
> -								 D_SU);
> +					dsu = getnum(value, &dopts, D_SU);
>  					break;
>  				case D_SW:
>  					if (nodsflag)
>  						conflict('d', subopts, D_NOALIGN,
>  							 D_SW);
> -					dsw = getnum_checked(value, &dopts,
> -								 D_SW);
> +					dsw = getnum(value, &dopts, D_SW);
>  					break;
>  				case D_NOALIGN:
>  					if (dsu)
> @@ -1660,8 +1650,8 @@ main(
>  					if (ssflag)
>  						conflict('d', subopts, D_SECTSIZE,
>  							 D_SECTLOG);
> -					sectorlog = getnum_checked(value, &dopts,
> -								   D_SECTLOG);
> +					sectorlog = getnum(value, &dopts,
> +							   D_SECTLOG);
>  					sectorsize = 1 << sectorlog;
>  					slflag = 1;
>  					break;
> @@ -1669,28 +1659,27 @@ main(
>  					if (slflag)
>  						conflict('d', subopts, D_SECTLOG,
>  							 D_SECTSIZE);
> -					sectorsize = getnum_checked(value,
> -							&dopts, D_SECTSIZE);
> +					sectorsize = getnum(value, &dopts,
> +							    D_SECTSIZE);
>  					sectorlog =
>  						libxfs_highbit32(sectorsize);
>  					ssflag = 1;
>  					break;
>  				case D_RTINHERIT:
> -					c = getnum_checked(value, &dopts,
> -							   D_RTINHERIT);
> +					c = getnum(value, &dopts, D_RTINHERIT);
>  					if (c)
>  						fsx.fsx_xflags |=
>  							XFS_DIFLAG_RTINHERIT;
>  					break;
>  				case D_PROJINHERIT:
> -					fsx.fsx_projid = getnum_checked(value,
> -							&dopts, D_PROJINHERIT);
> +					fsx.fsx_projid = getnum(value, &dopts,
> +								D_PROJINHERIT);
>  					fsx.fsx_xflags |=
>  						XFS_DIFLAG_PROJINHERIT;
>  					break;
>  				case D_EXTSZINHERIT:
> -					fsx.fsx_extsize = getnum_checked(value,
> -							&dopts, D_EXTSZINHERIT);
> +					fsx.fsx_extsize = getnum(value, &dopts,
> +								 D_EXTSZINHERIT);
>  					fsx.fsx_xflags |=
>  						XFS_DIFLAG_EXTSZINHERIT;
>  					break;
> @@ -1708,8 +1697,8 @@ main(
>  				switch (getsubopt(&p, (constpp)subopts,
>  						  &value)) {
>  				case I_ALIGN:
> -					sb_feat.inode_align = getnum_checked(
> -							value, &iopts, I_ALIGN);
> +					sb_feat.inode_align = getnum(value,
> +								&iopts, I_ALIGN);
>  					break;
>  				case I_LOG:
>  					if (ipflag)
> @@ -1718,14 +1707,13 @@ main(
>  					if (isflag)
>  						conflict('i', subopts, I_SIZE,
>  							 I_LOG);
> -					inodelog = getnum_checked(value, &iopts,
> -								  I_LOG);
> +					inodelog = getnum(value, &iopts, I_LOG);
>  					isize = 1 << inodelog;
>  					ilflag = 1;
>  					break;
>  				case I_MAXPCT:
> -					imaxpct = getnum_checked(value, &iopts,
> -								 I_MAXPCT);
> +					imaxpct = getnum(value, &iopts,
> +							 I_MAXPCT);
>  					imflag = 1;
>  					break;
>  				case I_PERBLOCK:
> @@ -1735,8 +1723,8 @@ main(
>  					if (isflag)
>  						conflict('i', subopts, I_SIZE,
>  							 I_PERBLOCK);
> -					inopblock = getnum_checked(value, &iopts,
> -								   I_PERBLOCK);
> +					inopblock = getnum(value, &iopts,
> +							   I_PERBLOCK);
>  					ipflag = 1;
>  					break;
>  				case I_SIZE:
> @@ -1746,20 +1734,18 @@ main(
>  					if (ipflag)
>  						conflict('i', subopts, I_PERBLOCK,
>  							 I_SIZE);
> -					isize = getnum_checked(value, &iopts,
> -							       I_SIZE);
> +					isize = getnum(value, &iopts, I_SIZE);
>  					inodelog = libxfs_highbit32(isize);
>  					isflag = 1;
>  					break;
>  				case I_ATTR:
>  					sb_feat.attr_version =
> -						getnum_checked(value, &iopts,
> -							       I_ATTR);
> +						getnum(value, &iopts, I_ATTR);
>  					break;
>  				case I_PROJID32BIT:
>  					sb_feat.projid16bit =
> -						!getnum_checked(value, &iopts,
> -								I_PROJID32BIT);
> +						!getnum(value, &iopts,
> +							I_PROJID32BIT);
>  					break;
>  				case I_SPINODES:
>  					if (!value || *value == '\0')
> @@ -1784,16 +1770,15 @@ main(
>  				case L_AGNUM:
>  					if (ldflag)
>  						conflict('l', subopts, L_AGNUM, L_DEV);
> -					logagno = getnum_checked(value, &lopts,
> -								 L_AGNUM);
> +					logagno = getnum(value, &lopts, L_AGNUM);
>  					laflag = 1;
>  					break;
>  				case L_FILE:
>  					if (loginternal)
>  						conflict('l', subopts, L_INTERNAL,
>  							 L_FILE);
> -					xi.lisfile = getnum_checked(value,
> -								&lopts, L_FILE);
> +					xi.lisfile = getnum(value, &lopts,
> +							    L_FILE);
>  					if (xi.lisfile)
>  						xi.lcreat = 1;
>  					break;
> @@ -1804,18 +1789,16 @@ main(
>  						conflict('l', subopts, L_FILE,
>  							 L_INTERNAL);
>  
> -					loginternal = getnum_checked(value,
> -							&lopts, L_INTERNAL);
> +					loginternal = getnum(value, &lopts,
> +							     L_INTERNAL);
>  					liflag = 1;
>  					break;
>  				case L_SU:
> -					lsu = getnum_checked(value, &lopts,
> -								 L_SU);
> +					lsu = getnum(value, &lopts, L_SU);
>  					lsuflag = 1;
>  					break;
>  				case L_SUNIT:
> -					lsunit = getnum_checked(value, &lopts,
> -								 L_SUNIT);
> +					lsunit = getnum(value, &lopts, L_SUNIT);
>  					lsunitflag = 1;
>  					break;
>  				case L_NAME:
> @@ -1835,8 +1818,7 @@ main(
>  					break;
>  				case L_VERSION:
>  					sb_feat.log_version =
> -						getnum_checked(value, &lopts,
> -							       L_VERSION);
> +						getnum(value, &lopts, L_VERSION);
>  					lvflag = 1;
>  					break;
>  				case L_SIZE:
> @@ -1851,8 +1833,8 @@ main(
>  					if (lssflag)
>  						conflict('l', subopts, L_SECTSIZE,
>  							 L_SECTLOG);
> -					lsectorlog = getnum_checked(value,
> -							&lopts, L_SECTLOG);
> +					lsectorlog = getnum(value, &lopts,
> +							    L_SECTLOG);
>  					lsectorsize = 1 << lsectorlog;
>  					lslflag = 1;
>  					break;
> @@ -1860,15 +1842,15 @@ main(
>  					if (lslflag)
>  						conflict('l', subopts, L_SECTLOG,
>  							 L_SECTSIZE);
> -					lsectorsize = getnum_checked(value,
> -							&lopts, L_SECTSIZE);
> +					lsectorsize = getnum(value, &lopts,
> +							     L_SECTSIZE);
>  					lsectorlog =
>  						libxfs_highbit32(lsectorsize);
>  					lssflag = 1;
>  					break;
>  				case L_LAZYSBCNTR:
>  					sb_feat.lazy_sb_counters =
> -						getnum_checked(value, &lopts,
> +							getnum(value, &lopts,
>  							       L_LAZYSBCNTR);
>  					break;
>  				default:
> @@ -1891,8 +1873,7 @@ main(
>  						  &value)) {
>  				case M_CRC:
>  					sb_feat.crcs_enabled =
> -						getnum_checked(value, &mopts,
> -							       M_CRC);
> +						getnum(value, &mopts, M_CRC);
>  					if (sb_feat.crcs_enabled && nftype) {
>  						fprintf(stderr,
>  _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -1926,8 +1907,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					if (nsflag)
>  						conflict('n', subopts, N_SIZE,
>  							 N_LOG);
> -					dirblocklog = getnum_checked(value,
> -								&nopts, N_LOG);
> +					dirblocklog = getnum(value, &nopts,
> +							     N_LOG);
>  					dirblocksize = 1 << dirblocklog;
>  					nlflag = 1;
>  					break;
> @@ -1935,8 +1916,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					if (nlflag)
>  						conflict('n', subopts, N_LOG,
>  							 N_SIZE);
> -					dirblocksize = getnum_checked(value,
> -								&nopts, N_SIZE);
> +					dirblocksize = getnum(value, &nopts,
> +							      N_SIZE);
>  					dirblocklog =
>  						libxfs_highbit32(dirblocksize);
>  					nsflag = 1;
> @@ -1951,9 +1932,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  						sb_feat.nci = true;
>  					} else {
>  						sb_feat.dir_version =
> -							getnum_checked(value,
> -								&nopts,
> -								N_VERSION);
> +							getnum(value, &nopts,
> +							       N_VERSION);
>  					}
>  					nvflag = 1;
>  					break;
> @@ -1963,8 +1943,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 = getnum_checked(value,
> -								&nopts, N_FTYPE);
> +					sb_feat.dirftype = getnum(value, &nopts,
> +								  N_FTYPE);
>  					nftype = 1;
>  					break;
>  				default:
> @@ -2002,8 +1982,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					rtextsize = value;
>  					break;
>  				case R_FILE:
> -					xi.risfile = getnum_checked(value,
> -								&ropts, R_FILE);
> +					xi.risfile = getnum(value, &ropts,
> +							    R_FILE);
>  					if (xi.risfile)
>  						xi.rcreat = 1;
>  					break;
> @@ -2043,8 +2023,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					if (ssflag || lssflag)
>  						conflict('s', subopts,
>  							 S_SECTSIZE, S_SECTLOG);
> -					sectorlog = getnum_checked(value, &sopts,
> -								   S_SECTLOG);
> +					sectorlog = getnum(value, &sopts,
> +							   S_SECTLOG);
>  					lsectorlog = sectorlog;
>  					sectorsize = 1 << sectorlog;
>  					lsectorsize = sectorsize;
> @@ -2055,8 +2035,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  					if (slflag || lslflag)
>  						conflict('s', subopts, S_SECTLOG,
>  							 S_SECTSIZE);
> -					sectorsize = getnum_checked(value,
> -							&sopts, S_SECTSIZE);
> +					sectorsize = getnum(value, &sopts,
> +							    S_SECTSIZE);
>  					lsectorsize = sectorsize;
>  					sectorlog =
>  						libxfs_highbit32(sectorsize);
> @@ -2291,7 +2271,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (dsize) {
>  		__uint64_t dbytes;
>  
> -		dbytes = getnum_checked(dsize, &dopts, D_SIZE);
> +		dbytes = getnum(dsize, &dopts, D_SIZE);
>  		if (dbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal data length %lld, not a multiple of %d\n"),
> @@ -2328,7 +2308,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (logsize) {
>  		__uint64_t logbytes;
>  
> -		logbytes = getnum_checked(logsize, &lopts, L_SIZE);
> +		logbytes = getnum(logsize, &lopts, L_SIZE);
>  		if (logbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal log length %lld, not a multiple of %d\n"),
> @@ -2350,7 +2330,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (rtsize) {
>  		__uint64_t rtbytes;
>  
> -		rtbytes = getnum_checked(rtsize, &ropts, R_SIZE);
> +		rtbytes = getnum(rtsize, &ropts, R_SIZE);
>  		if (rtbytes % XFS_MIN_BLOCKSIZE) {
>  			fprintf(stderr,
>  			_("illegal rt length %lld, not a multiple of %d\n"),
> @@ -2370,7 +2350,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	if (rtextsize) {
>  		__uint64_t rtextbytes;
>  
> -		rtextbytes = getnum_checked(rtextsize, &ropts, R_EXTSIZE);
> +		rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
>  		if (rtextbytes % blocksize) {
>  			fprintf(stderr,
>  		_("illegal rt extent size %lld, not a multiple of %d\n"),
> @@ -3466,8 +3446,8 @@ unknown(
>  
>  long long
>  cvtnum(
> -	unsigned int	blocksize,
> -	unsigned int	sectorsize,
> +	unsigned int	blksize,
> +	unsigned int	sectsize,
>  	const char	*s)
>  {
>  	long long	i;
> @@ -3480,9 +3460,9 @@ cvtnum(
>  		return i;
>  
>  	if (*sp == 'b' && sp[1] == '\0')
> -		return i * blocksize;
> +		return i * blksize;
>  	if (*sp == 's' && sp[1] == '\0')
> -		return i * sectorsize;
> +		return i * sectsize;
>  
>  	if (*sp == 'k' && sp[1] == '\0')
>  		return 1024LL * i;
> -- 
> 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