[PATCH] mkfs: better error with incorrect b/s value suffix usage

[PATCH] mkfs: better error with incorrect b/s value suffix usage

[Jan Tulak]

If user writes a value using b or s suffix without explicitly stating the size
of blocks or sectors, mkfs ends with a not helpful error about the value being
too small. It happens because we read the physical geometry after all options
are parsed.

So, tell the user exactly what is wrong with the input.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---

While adding entries into the mkfs input test, I found this issue, though it
may be only a documentation thing. When I give some option block/sector suffix
without specifying explicitly its size (for example, -l su=10b without -b
size=4096), I get an error that value 10b is too small.  Of course it is,
because, at the time, mkfs did not read physical geometry yet, so blocksize is
0. And 10*0 = 0.

I think that this is not something we need to change, but it should be better
documented. Maybe not manpage (where it can be overlooked if not written to
every option using the size and it might be that it already is somewhere down
there), but an error message should warn the user in case of using b or s
suffix incorrectly.

I'm open to suggestions for a better solution, though.

Cheers,
Jan
---
 mkfs/xfs_mkfs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 955dcfd..2870f7b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3615,9 +3615,21 @@ cvtnum(
 		return -1LL;
 
 	if (*sp == 'b')
-		return i * blksize;
+		if (!blksize) {
+			fprintf(stderr,
+_("Blocksize must be explicitly provided when using 'b' suffix.\n"));
+			usage();
+		} else {
+			return i * blksize;
+		}
 	if (*sp == 's')
-		return i * sectsize;
+		if (!sectsize) {
+			fprintf(stderr,
+_("Sectorsize must be explicitly provided when using 's' suffix.\n"));
+			usage();
+		} else {
+			return i * sectsize;
+		}
 
 	c = tolower(*sp);
 	switch (c) {
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2] mkfs: better error with incorrect b/s value suffix usage

[Jan Tulak]

If user writes a value using b or s suffix without explicitly stating the size
of blocks or sectors, mkfs ends with a not helpful error about the value being
too small. It happens because we read the physical geometry after all options
are parsed.

So, tell the user exactly what is wrong with the input.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---

While adding entries into the mkfs input test, I found this issue, though it
may be only a documentation thing. When I give some option block/sector suffix
without specifying explicitly its size (for example, -l su=10b without -b
size=4096), I get an error that value 10b is too small.  Of course it is,
because, at the time, mkfs did not read physical geometry yet, so blocksize is
0. And 10*0 = 0.

I think that this is not something we need to change, but it should be better
documented. Maybe not manpage (where it can be overlooked if not written to
every option using the size and it might be that it already is somewhere down
there), but an error message should warn the user in case of using b or s
suffix incorrectly.

I'm open to suggestions for a better solution, though.

UPDATE:
Add { and } to fix a gcc warning about ambigious else branch.

Cheers,
Jan
---
 mkfs/xfs_mkfs.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index ed7800f..455bf11 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3614,10 +3614,24 @@ cvtnum(
 	if (sp[1] != '\0')
 		return -1LL;
 
-	if (*sp == 'b')
-		return i * blksize;
-	if (*sp == 's')
-		return i * sectsize;
+	if (*sp == 'b') {
+		if (!blksize) {
+			fprintf(stderr,
+_("Blocksize must be explicitly provided when using 'b' suffix.\n"));
+			usage();
+		} else {
+			return i * blksize;
+		}
+	}
+	if (*sp == 's') {
+		if (!sectsize) {
+			fprintf(stderr,
+_("Sectorsize must be explicitly provided when using 's' suffix.\n"));
+			usage();
+		} else {
+			return i * sectsize;
+		}
+	}
 
 	c = tolower(*sp);
 	switch (c) {
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] mkfs: better error with incorrect b/s value suffix usage

[Eric Sandeen]

On 6/9/16 8:27 AM, Jan Tulak wrote:
> If user writes a value using b or s suffix without explicitly stating the size
> of blocks or sectors, mkfs ends with a not helpful error about the value being
> too small. It happens because we read the physical geometry after all options
> are parsed.

It's probably worth putting an example of the current error in the commitlog,
i.e.:

# mkfs.xfs -dfile,name=fsfile,size=128m -l su=10b -b
Illegal value 10b for -l su option. value is too small
...


> So, tell the user exactly what is wrong with the input.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> 
> While adding entries into the mkfs input test, I found this issue, though it
> may be only a documentation thing. When I give some option block/sector suffix
> without specifying explicitly its size (for example, -l su=10b without -b
> size=4096), I get an error that value 10b is too small.  Of course it is,
> because, at the time, mkfs did not read physical geometry yet, so blocksize is
> 0. And 10*0 = 0.
> 
> I think that this is not something we need to change, but it should be better
> documented. Maybe not manpage (where it can be overlooked if not written to
> every option using the size and it might be that it already is somewhere down
> there), but an error message should warn the user in case of using b or s
> suffix incorrectly.
> 
> I'm open to suggestions for a better solution, though.
> 
> UPDATE:
> Add { and } to fix a gcc warning about ambigious else branch.
> 
> Cheers,
> Jan
> ---
>  mkfs/xfs_mkfs.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index ed7800f..455bf11 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3614,10 +3614,24 @@ cvtnum(
>  	if (sp[1] != '\0')
>  		return -1LL;
>  
> -	if (*sp == 'b')
> -		return i * blksize;
> -	if (*sp == 's')
> -		return i * sectsize;
> +	if (*sp == 'b') {
> +		if (!blksize) {
> +			fprintf(stderr,
> +_("Blocksize must be explicitly provided when using 'b' suffix.\n"));

I'd fix the warning text a little bit, because:

# mkfs.xfs -dfile,name=fsfile,size=128m -l su=10b -b size=4k
Blocksize must be explicitly provided when using 'b' suffix.

But I *did* provide it!  No fair! ;)

Perhaps:

+_("Blocksize must be specified prior to using the 'b' suffix.\n"));

and

+_("Sectorsize must be specified prior to using the 's' suffix.\n"));

?

(old mkfs said "blocksize not available yet." which is a bit cryptic
as well, but at least we clearly have the same behavior now, just
different messages.)

-Eric

> +			usage();
> +		} else {
> +			return i * blksize;
> +		}
> +	}
> +	if (*sp == 's') {
> +		if (!sectsize) {
> +			fprintf(stderr,
> +_("Sectorsize must be explicitly provided when using 's' suffix.\n"));
> +			usage();
> +		} else {
> +			return i * sectsize;
> +		}
> +	}
>  
>  	c = tolower(*sp);
>  	switch (c) {
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3] mkfs: better error with incorrect b/s value suffix usage

[Jan Tulak]

If user writes a value using b or s suffix without explicitly stating the size
of blocks or sectors, mkfs ends with a not helpful error about the value being
too small. It happens because we read the physical geometry after all options
are parsed.

So, tell the user exactly what is wrong with the input.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---

While adding entries into the mkfs input test, I found this issue, though it
may be only a documentation thing. When I give some option block/sector suffix
without specifying explicitly its size (for example, -l su=10b without -b
size=4096), I get an error that value 10b is too small.  Of course it is,
because, at the time, mkfs did not read physical geometry yet, so blocksize is
0. And 10*0 = 0.

I think that this is not something we need to change, but it should be better
documented. Maybe not manpage (where it can be overlooked if not written to
every option using the size and it might be that it already is somewhere down
there), but an error message should warn the user in case of using b or s
suffix incorrectly.

I'm open to suggestions for a better solution, though.

UPDATE:
Changed error message. Thanks Eric, this sounds better. :-)

Cheers,
Jan

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
 mkfs/xfs_mkfs.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index ed7800f..6059d91 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3614,10 +3614,24 @@ cvtnum(
 	if (sp[1] != '\0')
 		return -1LL;
 
-	if (*sp == 'b')
-		return i * blksize;
-	if (*sp == 's')
-		return i * sectsize;
+	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) {
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs