[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 at 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) {
>