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

[David Hildenbrand <david@redhat.com>]

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 <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. 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 <david@redhat.com>
>>
>> Related work on the QObject input visitor:
>>
>> commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1
>> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 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 <marcandre.lureau@redhat.com>
>>     Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>     Message-Id: <20170607163635.17635-12-marcandre.lureau@redhat.com>
>>     [check_native_list() tweaked for consistency with signed case]
>>     Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> 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