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

[David Hildenbrand]

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)


-- 

Thanks,

David / dhildenb