Re: [PATCH v2 2/5] qemu_img: add error report to cvtnum

[Eric Blake <eblake@redhat.com>]

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> All calls to cvtnum check the return value and print the same error message more
> or less. And so error reporting moved to cvtnum to reduce duplicate code and
> provide a single error message.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>   qemu-img.c                 | 63 ++++++++++++++++----------------------
>   tests/qemu-iotests/049.out |  4 +--
>   2 files changed, 28 insertions(+), 39 deletions(-)
> 

> 
> -    err = qemu_strtosz(s, NULL, &value);
> -    if (err < 0) {
> +    err = qemu_strtosz(arg_value, NULL, &value);
> +    if (err < 0 && err != -ERANGE) {
> +        error_report("Invalid %s specified! You may use "
> +                     "k, M, G, T, P or E suffixes for ", arg_name);
> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                     "petabytes and exabytes.");
>          return err;
>      }
> -    if (value > INT64_MAX) {
> +    if (err == -ERANGE || value > INT64_MAX) {
> +        error_report("Invalid %s specified! Must be less than 8 EiB!",

Copied from our pre-existing errors, but why are we shouting at our 
user?  This would be a good time to s/!/./ to tone it down a bit.

> @@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
>   {
>       int64_t res;
>   
> -    res = cvtnum(arg);
> +    res = cvtnum("bs", arg);
>   
> -    if (res <= 0) {
> -        error_report("invalid number: '%s'", arg);
> +    if (res < 0) {
> +        return 1;
> +    } else if (res == 0) {
> +        error_report("Invalid bs specified! It cannot be 0.");

Maybe it's worth two functions:

int64_t cvtnum_full(const char *name, const char *value, int64_t min, 
int64_t max)

and then a common helper:

int64_t cvtnum(const char *name, const char *value) {
     return cvtnum_full(name, value, 0, INT64_MAX);
}

many existing callers remain with cvtnum(), but callers like this could 
be cvtnum("bs", arg, 1, INT64_MAX).  You'd still have to special-case 
other restrictions, such as whether a number must a power-of-2, but 
that's fewer places.

> +++ b/tests/qemu-iotests/049.out
> @@ -92,13 +92,13 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1649267441664 cluster_size=65536 l
>   == 3. Invalid sizes ==
>   
>   qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
> -qemu-img: Image size must be less than 8 EiB!
> +qemu-img: Invalid image size specified! Must be less than 8 EiB!

Nice that you checked for iotest fallout.  Is this really the only 
impacted test?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org