Re: [Qemu-devel] [PATCH v3 2/7] qapi: correctly parse uint64_t values from strings

[David Hildenbrand <david@redhat.com>]

On 31.10.18 15:32, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> Right now, we parse uint64_t values just like int64_t values, resulting
>> in negative values getting accepted and certain valid large numbers only
>> being representable as negative numbers. Also, reported errors indicate
>> that an int64_t is expected.
>>
>> Parse uin64_t separately. We don't have to worry about ranges.
> 
> The commit message should mention *why* we don't we have to worry about
> ranges.

"Parse uin64_t separately. We don't have to worry about ranges as far as
I can see. Ranges are parsed and processed via start_list()/next_list()
and friends. parse_type_int64() only has to deal with ranges as it
reuses the function parse_str(). E.g. parse_type_size() also does not
have to handle ranges. (I assume that we could easily reimplement
parse_type_int64() in a similar fashion, too).

The only thing that will change is that uint64_t properties that didn't
expect a range will now actually bail out if a range is supplied."

I'll do some more testing.

> 
>>
>> E.g. we can now also specify
>>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
>> Instead of only going via negative values
>>     -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000
>>
>> Resulting in the same values
>>
>> (qemu) info memory-devices
>> Memory device [nvdimm]: "nv1"
>>   addr: 0xffffffffc0000000
>>   slot: 0
>>   node: 0
>>
> 
> Suggest to mention this makes the string-input-visitor catch up with the
> qobject-input-visitor, which got changed similarly in commit
> 5923f85fb82.

Yes, I will add that!

> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  qapi/string-input-visitor.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index c1454f999f..f2df027325 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -247,15 +247,16 @@ error:
>>  static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>>                                Error **errp)
>>  {
>> -    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
>> -    int64_t i;
>> -    Error *err = NULL;
>> -    parse_type_int64(v, name, &i, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -    } else {
>> -        *obj = i;
>> +    StringInputVisitor *siv = to_siv(v);
>> +    uint64_t val;
>> +
>> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> 
> Works because qemu_strtou64() accepts negative numbers and interprets
> them modulo 2^64.

I will also add a comment to the description that negative numbers will
continue to work.

> 
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> +                   "an uint64 value");
> 
> I think this should be "a uint64 value".

As I am not a native speaker, I will stick to your suggestion unless
somebody else speaks up.

> 
>> +        return;
>>      }
>> +
>> +    *obj = val;
>>  }
>>  
>>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> 
> Patch looks good to me otherwise.
> 

Thanks!

-- 

Thanks,

David / dhildenb