Re: [PATCH 1/2] mkfs: use cvtnum from libfrog

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move the checks for zero block/sector size to the libfrog code
> and return -1LL as an invalid value instead. Catch the invalid
> value in mkfs and error out there instead of inside cvtnum.
> 
> Also rename the libfrog block/sector size variables so they don't
> shadow the mkfs global variables of the same name.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  libfrog/convert.c | 12 +++++---
>  mkfs/xfs_mkfs.c   | 71 ++++-------------------------------------------
>  2 files changed, 14 insertions(+), 69 deletions(-)
> 
> diff --git a/libfrog/convert.c b/libfrog/convert.c
> index 8d4d4077b331..b5f3fc1238dd 100644
> --- a/libfrog/convert.c
> +++ b/libfrog/convert.c
> @@ -182,8 +182,8 @@ cvt_u16(
>  
>  long long
>  cvtnum(
> -	size_t		blocksize,
> -	size_t		sectorsize,
> +	size_t		blksize,
> +	size_t		sectsize,
>  	char		*s)
>  {
>  	long long	i;
> @@ -202,9 +202,13 @@ cvtnum(
>  	c = tolower(*sp);
>  	switch (c) {
>  	case 'b':
> -		return i * blocksize;
> +		if (!blksize)
> +			return -1LL;
> +		return i * blksize;
>  	case 's':
> -		return i * sectorsize;
> +		if (!sectsize)
> +			return -1LL;
> +		return i * sectsize;
>  	case 'k':
>  		return KILOBYTES(i);
>  	case 'm':
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 0adaa65d19f8..04063ca5b2c7 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -942,69 +942,6 @@ unknown(
>  	usage();
>  }
>  
> -long long
> -cvtnum(
> -	unsigned int	blksize,
> -	unsigned int	sectsize,
> -	const char	*s)
> -{
> -	long long	i;
> -	char		*sp;
> -	int		c;
> -
> -	i = strtoll(s, &sp, 0);
> -	if (i == 0 && sp == s)
> -		return -1LL;
> -	if (*sp == '\0')
> -		return i;
> -
> -	if (sp[1] != '\0')
> -		return -1LL;
> -
> -	if (*sp == 'b') {
> -		if (!blksize) {
> -			fprintf(stderr,
> -_("Blocksize must be provided prior to using 'b' suffix.\n"));
> -			usage();
> -		} else {
> -			return i * blksize;
> -		}
> -	}
> -	if (*sp == 's') {
> -		if (!sectsize) {
> -			fprintf(stderr,
> -_("Sectorsize must be specified prior to using 's' suffix.\n"));

Hmm, so this message is replaced with "Not a valid value or illegal suffix"?

That's not anywhere near as helpful as the old message... maybe we
should have this set errno or something so that callers can distinguish
between "you sent garbled input" vs. "you need to set up
blocksize /sectsize"... ?

--D

> -			usage();
> -		} else {
> -			return i * sectsize;
> -		}
> -	}
> -
> -	c = tolower(*sp);
> -	switch (c) {
> -	case 'e':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'p':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 't':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'g':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'm':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'k':
> -		return i * 1024LL;
> -	default:
> -		break;
> -	}
> -	return -1LL;
> -}
> -
>  static void
>  check_device_type(
>  	const char	*name,
> @@ -1347,9 +1284,13 @@ getnum(
>  	 * convert it ourselves to guarantee there is no trailing garbage in the
>  	 * number.
>  	 */
> -	if (sp->convert)
> +	if (sp->convert) {
>  		c = cvtnum(blocksize, sectorsize, str);
> -	else {
> +		if (c == -1LL) {
> +			illegal_option(str, opts, index,
> +				_("Not a valid value or illegal suffix"));
> +		}
> +	} else {
>  		char		*str_end;
>  
>  		c = strtoll(str, &str_end, 0);
> -- 
> 2.23.0.rc1
>