On 31.10.18 15:32, Markus Armbruster wrote:
> David Hildenbrand <da...@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 <da...@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
