[1/2] mkfs: use cvtnum from libfrog
diff mbox series

Message ID 20190813051421.21137-2-david@fromorbit.com
State New
Headers show
Series
  • xfsprogs: "Fix" --disable-static option
Related show

Commit Message

Dave Chinner Aug. 13, 2019, 5:14 a.m. UTC
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(-)

Comments

Darrick J. Wong Aug. 13, 2019, 2:24 p.m. UTC | #1
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
>
Dave Chinner Aug. 13, 2019, 9:29 p.m. UTC | #2
On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > -	}
> > -	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"?

Actually, the error message is this:

# mkfs.xfs -f -b size=1b /dev/vdc
Invalid value 1b for -b size option. Not a valid value or illegal suffix

It does actually tell you what the value is, what option is wrong,
and the message shold be fairly clear that specifying the block size
in using a "blocks" suffix is illegal.

> 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"... ?

Actually, the error will only occur when you use -s size= or -b
size= options, as if they are not specified we use the default
values in mkfs and cvtnum is always called with a valid
blocksize/sectorsize pair. i.e. This error only triggers when validating
the base sector size/block size options because that occurs before
we set the global varibles mkfs will use for cvtnum....

It's a chicken-egg thing, and I figured the error message prefix
would be sufficient to point out the problem with the value suffic
used for these kinda unusual corner cases.

Cheers,

Dave.
Darrick J. Wong Aug. 14, 2019, 3:18 p.m. UTC | #3
On Wed, Aug 14, 2019 at 07:29:36AM +1000, Dave Chinner wrote:
> On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > -	}
> > > -	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"?
> 
> Actually, the error message is this:
> 
> # mkfs.xfs -f -b size=1b /dev/vdc
> Invalid value 1b for -b size option. Not a valid value or illegal suffix
> 
> It does actually tell you what the value is, what option is wrong,
> and the message shold be fairly clear that specifying the block size
> in using a "blocks" suffix is illegal.
> 
> > 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"... ?
> 
> Actually, the error will only occur when you use -s size= or -b
> size= options, as if they are not specified we use the default
> values in mkfs and cvtnum is always called with a valid
> blocksize/sectorsize pair. i.e. This error only triggers when validating
> the base sector size/block size options because that occurs before
> we set the global varibles mkfs will use for cvtnum....
> 
> It's a chicken-egg thing, and I figured the error message prefix
> would be sufficient to point out the problem with the value suffic
> used for these kinda unusual corner cases.

Heh, ok, carry on then. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Patch
diff mbox series

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"));
-			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);