[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/7] qapi: correctly parse uint64_t values fr
From: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/7] qapi: correctly parse uint64_t values from strings |
Date: |
Tue, 23 Oct 2018 17:07:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 23/10/2018 14:10, David Hildenbrand wrote:
> On 17/10/2018 14:42, Markus Armbruster wrote:
>> Quick peek only for now.
>>
>> David Hildenbrand <address@hidden> 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. Implementation inspired by original
>>> parse_str() implementation.
>>>
>>> E.g. we can now specify
>>> -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
>>> Instead of 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
>>>
>>> Signed-off-by: David Hildenbrand <address@hidden>
>>
>> Related work on the QObject input visitor:
>>
>> commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1
>> Author: Marc-André Lureau <address@hidden>
>> Date: Wed Jun 7 20:36:03 2017 +0400
>>
>> qapi: update the qobject visitor to use QNUM_U64
>>
>> Switch to use QNum/uint where appropriate to remove i64 limitation.
>>
>> The input visitor will cast i64 input to u64 for compatibility
>> reasons (existing json QMP client already use negative i64 for large
>> u64, and expect an implicit cast in qemu).
>>
>> Note: before the patch, uint64_t values above INT64_MAX are sent over
>> json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the
>> patch, they are sent unmodified. Clearly a bug fix, but we have to
>> consider compatibility issues anyway. libvirt should cope fine,
>> because its parsing of unsigned integers accepts negative values
>> modulo 2^64. There's hope that other clients will, too.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> Reviewed-by: Markus Armbruster <address@hidden>
>> Message-Id: <address@hidden>
>> [check_native_list() tweaked for consistency with signed case]
>> Signed-off-by: Markus Armbruster <address@hidden>
>>
>> Note who it considers backward compatibility. Have you done that for
>> the string input visitor? The commit message should tell.
>
> There should be no compat issues, negative values are still accepted. At
> least I can't think of any :) We simply allow accepting bigger values.
>
>>
>>> ---
>>> qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++----
>>> 1 file changed, 106 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>>> index b3fdd0827d..af0a841152 100644
>>> --- a/qapi/string-input-visitor.c
>>> +++ b/qapi/string-input-visitor.c
>>> @@ -19,6 +19,7 @@
>>> #include "qapi/qmp/qnull.h"
>>> #include "qemu/option.h"
>>> #include "qemu/queue.h"
>>> +#include "qemu/cutils.h"
>>> #include "qemu/range.h"
>>>
>>>
>>> @@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy)
>>> g_free(range);
>>> }
>>>
>>> -static int parse_str(StringInputVisitor *siv, const char *name, Error
>>> **errp)
>>> +static int parse_str_int64(StringInputVisitor *siv, const char *name,
>>> + Error **errp)
>>> {
>>> char *str = (char *) siv->string;
>>> long long start, end;
>>> @@ -118,6 +120,75 @@ error:
>>> return -1;
>>> }
>>>
>>> +static int parse_str_uint64(StringInputVisitor *siv, const char *name,
>>> + Error **errp)
>>> +{
>>> + const char *str = (char *) siv->string;
>>> + uint64_t start, end;
>>> + const char *endptr;
>>> + Range *cur;
>>> +
>>> + if (siv->ranges) {
>>> + return 0;
>>> + }
>>> +
>>> + if (!*str) {
>>> + return 0;
>>> + }
>>> +
>>> + do {
>>> + if (!qemu_strtou64(str, &endptr, 0, &start)) {
>>> + if (*endptr == '\0') {
>>> + cur = g_malloc0(sizeof(*cur));
>>> + range_set_bounds(cur, start, start);
>>> + siv->ranges = range_list_insert(siv->ranges, cur);
>>> + cur = NULL;
>>> + str = NULL;
>>> + } else if (*endptr == '-') {
>>> + str = endptr + 1;
>>> + if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end)
>>> {
>>> + if (*endptr == '\0') {
>>> + cur = g_malloc0(sizeof(*cur));
>>> + range_set_bounds(cur, start, end);
>>> + siv->ranges = range_list_insert(siv->ranges, cur);
>>> + cur = NULL;
>>> + str = NULL;
>>> + } else if (*endptr == ',') {
>>> + str = endptr + 1;
>>> + cur = g_malloc0(sizeof(*cur));
>>> + range_set_bounds(cur, start, end);
>>> + siv->ranges = range_list_insert(siv->ranges, cur);
>>> + cur = NULL;
>>> + } else {
>>> + goto error;
>>> + }
>>> + } else {
>>> + goto error;
>>> + }
>>> + } else if (*endptr == ',') {
>>> + str = endptr + 1;
>>> + cur = g_malloc0(sizeof(*cur));
>>> + range_set_bounds(cur, start, start);
>>> + siv->ranges = range_list_insert(siv->ranges, cur);
>>> + cur = NULL;
>>> + } else {
>>> + goto error;
>>> + }
>>> + } else {
>>> + goto error;
>>> + }
>>> + } while (str);
>>> +
>>> + return 0;
>>> +error:
>>> + g_list_foreach(siv->ranges, free_range, NULL);
>>> + g_list_free(siv->ranges);
>>> + siv->ranges = NULL;
>>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>>> + "an uint64 value or range");
>>> + return -1;
>>> +}
>>> +
>>
>> Do we actually need unsigned ranges? I'm asking because I hate this
>> code, and duplicating can only make it worse.
>>
>> [...]
>
> I don't think we need unsigned ranges BUT I am concerned about backwards
> compatibility. I'll have to check all users to make sure no property
> flagged as uint64_t will actually expect ranges. Then we can drop it.
> (and simplify this code)
>
>
... looking at the details, I think you are right, we should not need
that range code at all. I will drop it. Makes things a lot simpler :)
--
Thanks,
David / dhildenb
- [Qemu-devel] [PATCH v2 0/7] qapi/range/memory-device: fixes and cleanups, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 2/7] qapi: use qemu_strtoi64() in parse_str_int64, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 3/7] range: pass const pointer where possible, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 4/7] range: add some more functions, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 5/7] memory-device: use QEMU_IS_ALIGNED, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 6/7] memory-device: avoid overflows on very huge devices, David Hildenbrand, 2018/10/12
- [Qemu-devel] [PATCH v2 7/7] memory-device: rewrite address assignment using ranges, David Hildenbrand, 2018/10/12