Re: [Qemu-devel] [PATCH 22/24] util/cutils: Return qemu_strtosz*() error and value separately

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/14/2017 04:26 AM, Markus Armbruster wrote:
>> This makes qemu_strtosz(), qemu_strtosz_mebi() and
>> qemu_strtosz_metric() similar to qemu_strtoi64(), except negative
>> values are rejected.
>
> Yay. It also opens the door to allowing them to return an unsigned 64
> bit value ;)
>
>> 
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Cc: Eduardo Habkost <address@hidden> (maintainer:X86)
>> Cc: Kevin Wolf <address@hidden> (supporter:Block layer core)
>> Cc: Max Reitz <address@hidden> (supporter:Block layer core)
>> Cc: address@hidden (open list:Block layer core)
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> @@ -1378,8 +1378,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>> QDict *qdict)
>>                  break;
>>              case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>>                  p.has_max_bandwidth = true;
>> -                valuebw = qemu_strtosz_mebi(valuestr, NULL);
>> -                if (valuebw < 0 || (size_t)valuebw != valuebw) {
>> +                ret = qemu_strtosz_mebi(valuestr, NULL, &valuebw);
>> +                if (ret < 0 || (size_t)valuebw != valuebw) {
>
> Question: do we want a wrapper that takes a maximum, as in:
>
> ret = qemu_strtosz_capped(valuestr, NULL, 'M', SIZE_MAX, &valuebw);
>
> so that the caller doesn't have to check (size_t)valuebw != valuebw ?

Maybe.

It's a more interesting question for qemu_strtou64(), actually.  The
obvious

    err = qemu_strtou64(str, NULL, 0, &value);
    if (value > limit) {
        err = -ERANGE;
    }

is subtly weird.  With str = "-1", you get value = UINT64_MAX.  With str
= "-18446744073709551615", you get value = 1.  Rejecting the former but
accepting the latter is weird.

> But not essential, so I can live with what you did here.
>
>
>> +++ b/qemu-img.c
>> @@ -370,10 +370,14 @@ static int add_old_style_options(const char *fmt, 
>> QemuOpts *opts,
>>  
>>  static int64_t cvtnum(const char *s)
>
>> +++ b/qemu-io-cmds.c
>> @@ -137,10 +137,14 @@ static char **breakline(char *input, int *count)
>>  
>>  static int64_t cvtnum(const char *s)
>
> May be some fallout based on rebase churn from earlier patch reviews,
> but nothing too serious to prevent you from adding:
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!