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

Consecutive error_report() calls each output a newline, which means your 
new output includes a trailing space.

> @@ -572,16 +584,8 @@ static int img_create(int argc, char **argv)
>       if (optind < argc) {
>           int64_t sval;
>   
> -        sval = cvtnum(argv[optind++]);
> +        sval = cvtnum("image size", argv[optind++]);
>           if (sval < 0) {
> -            if (sval == -ERANGE) {
> -                error_report("Image size must be less than 8 EiB!");
> -            } else {
> -                error_report("Invalid image size specified! You may use k, M, "
> -                      "G, T, P or E suffixes for ");
> -                error_report("kilobytes, megabytes, gigabytes, terabytes, "
> -                             "petabytes and exabytes.");
> -            }

True, that's what some of the old code was doing, but...

> +++ b/tests/qemu-iotests/049.out

>   
>   qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
> -qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
> +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for

where it gets hairy is that our iotests _intentionally_ strip trailing 
space before comparing to expected output, because it is such a pain to 
commit files with trailing spaces into the repository.  We're better off 
making the expected output precisely match what qemu-img actually 
outputs, which means using this as an opportunity to fix qemu-img to not 
output trailing space in the first place.

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