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

[Markus Armbruster]

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.

> ---
>  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.

[...]