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

[Eyal Moscovici <eyal.moscovici@oracle.com>]

On 07/05/2020 0:59, Eric Blake wrote:
> 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.
Sure, will fix.
>
>> @@ -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.
>
Great idea, I will create two functions.
>> +++ 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?
>
Thanks, yes.