Re: [PATCH v3 1/1] qemu_img: add cvtnum_full to print error reports

[Eric Blake <eblake@redhat.com>]

On 5/13/20 8:36 AM, 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_full to reduce code
> duplication and provide a single error message. Additionally, cvtnum now wraps
> cvtnum_full with the existing default range of 0 to MAX_INT64.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---

> -static int64_t cvtnum(const char *s)
> +static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
> +                           int64_t max)
>   {
>       int err;
> -    uint64_t value;
> -
> -    err = qemu_strtosz(s, NULL, &value);
> -    if (err < 0) {
> +    uint64_t res;
> +
> +    err = qemu_strtosz(value, NULL, &res);
> +    if (err < 0 && err != -ERANGE) {
> +        error_report("Invalid %s specified. You may use "
> +                     "k, M, G, T, P or E suffixes for ", name);
> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                     "petabytes and exabytes.");

The new common message is in terms of bytes, even though not all callers 
are specifically related to bytes.  I don't think it's a show-stopper, 
though, merely a quality of error message, and I don't have any ideas 
for how to improve it.

> @@ -4506,10 +4508,9 @@ static int img_dd_count(const char *arg,
>                           struct DdIo *in, struct DdIo *out,
>                           struct DdInfo *dd)
>   {
> -    dd->count = cvtnum(arg);
> +    dd->count = cvtnum("count", arg);
>   
>       if (dd->count < 0) {
> -        error_report("invalid number: '%s'", arg);

For example, the count argument to dd is not really about bytes, but 
repetitions.  So the error message is now more informative (what 
suffixes you can use) but also less accurate ("invalid number" was vague 
but at least correct).

I think the common code is worth the corner case regressions, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

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