[Qemu-devel] [PATCH v2 0/7] qapi/range/memory-device: fixes and cleanups

[Qemu-devel] [PATCH v2 0/7] qapi/range/memory-device: fixes and cleanups

[David Hildenbrand]

While working on memory device code, I noticed that specifiying an uint64_t
on command line does not work in all cases as we always parse an int64_t.
So I fix that and also cleanup the old int64_t parser.

To be able to fix some overflows in memory-device code in a clean way,
I am reusing the range implementation of qemu, for which I need some
more helpers.

This series is based on
    "[PATCH v5 00/16] memory-device: complete refactoring"
which should get merged soon.

v1 -> v2:
- "range: add some more functions"
-- Reduce number of functions
-- make range_init() return an error in case of overflow
-- provide range_init_nofail()
- "memory-device: rewrite address assignment using ranges"
-- Use new functions range_init/range_init_nofail
-- Use range_contains_range instead of starts_before/ends_after

David Hildenbrand (7):
  qapi: correctly parse uint64_t values from strings
  qapi: use qemu_strtoi64() in parse_str_int64
  range: pass const pointer where possible
  range: add some more functions
  memory-device: use QEMU_IS_ALIGNED
  memory-device: avoid overflows on very huge devices
  memory-device: rewrite address assignment using ranges

 hw/mem/memory-device.c      |  60 +++++++++-------
 include/qemu/range.h        |  68 +++++++++++++++++-
 qapi/string-input-visitor.c | 133 ++++++++++++++++++++++++++++++------
 3 files changed, 210 insertions(+), 51 deletions(-)

-- 
2.17.1

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

[David Hildenbrand]

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>
---
 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;
+}
+
 static void
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -128,7 +199,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size,
     assert(list);
     siv->list = list;
 
-    if (parse_str(siv, name, errp) < 0) {
+    if (parse_str_int64(siv, name, errp) < 0) {
         *list = NULL;
         return;
     }
@@ -216,7 +287,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
-    if (parse_str(siv, name, errp) < 0) {
+    if (parse_str_int64(siv, name, errp) < 0) {
         return;
     }
 
@@ -252,15 +323,39 @@ 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);
+
+    if (parse_str_uint64(siv, name, errp) < 0) {
+        return;
+    }
+
+    if (!siv->ranges) {
+        goto error;
+    }
+
+    if (!siv->cur_range) {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
+            goto error;
+        }
+
+        siv->cur = range_lob(r);
     }
+
+    *obj = siv->cur;
+    siv->cur++;
+    return;
+
+error:
+    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+               "an uint64 value or range");
 }
 
 static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/7] qapi: use qemu_strtoi64() in parse_str_int64

[David Hildenbrand]

The qemu api claims to be easier to use, and the resulting code
shows that.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/string-input-visitor.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index af0a841152..ee38ab1d10 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -48,10 +48,10 @@ static void free_range(void *range, void *dummy)
 static int parse_str_int64(StringInputVisitor *siv, const char *name,
                            Error **errp)
 {
-    char *str = (char *) siv->string;
-    long long start, end;
+    const char *str = (char *) siv->string;
+    const char *endptr;
+    int64_t start, end;
     Range *cur;
-    char *endptr;
 
     if (siv->ranges) {
         return 0;
@@ -62,9 +62,7 @@ static int parse_str_int64(StringInputVisitor *siv, const char *name,
     }
 
     do {
-        errno = 0;
-        start = strtoll(str, &endptr, 0);
-        if (errno == 0 && endptr > str) {
+        if (!qemu_strtoi64(str, &endptr, 0, &start)) {
             if (*endptr == '\0') {
                 cur = g_malloc0(sizeof(*cur));
                 range_set_bounds(cur, start, start);
@@ -73,11 +71,7 @@ static int parse_str_int64(StringInputVisitor *siv, const char *name,
                 str = NULL;
             } else if (*endptr == '-') {
                 str = endptr + 1;
-                errno = 0;
-                end = strtoll(str, &endptr, 0);
-                if (errno == 0 && endptr > str && start <= end &&
-                    (start > INT64_MAX - 65536 ||
-                     end < start + 65536)) {
+                if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
                     if (*endptr == '\0') {
                         cur = g_malloc0(sizeof(*cur));
                         range_set_bounds(cur, start, end);
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/7] range: pass const pointer where possible

[David Hildenbrand]

If there are no changes, let's use a const pointer.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/range.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index f28f0c1825..7e75f4e655 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -39,7 +39,7 @@ struct Range {
     uint64_t upb;        /* inclusive upper bound */
 };
 
-static inline void range_invariant(Range *range)
+static inline void range_invariant(const Range *range)
 {
     assert(range->lob <= range->upb || range->lob == range->upb + 1);
 }
@@ -48,14 +48,14 @@ static inline void range_invariant(Range *range)
 #define range_empty ((Range){ .lob = 1, .upb = 0 })
 
 /* Is @range empty? */
-static inline bool range_is_empty(Range *range)
+static inline bool range_is_empty(const Range *range)
 {
     range_invariant(range);
     return range->lob > range->upb;
 }
 
 /* Does @range contain @val? */
-static inline bool range_contains(Range *range, uint64_t val)
+static inline bool range_contains(const Range *range, uint64_t val)
 {
     return val >= range->lob && val <= range->upb;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/7] range: add some more functions

[David Hildenbrand]

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity
range_init_nofail(): Init using lower bound and size, validity asserted
range_size(): Extract the size of a range
range_overlaps_range(): Check for overlaps of two ranges
range_contains_range(): Check if one range is contained in the other

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 7e75f4e655..ba606c6bc0 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;
+}
+
+/*
+ * Check if @range1 overlaps with @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_overlaps_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return !(range2->upb < range1->lob || range1->upb < range2->lob);
+}
+
+/*
+ * Check if @range1 contains @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_contains_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return range1->lob <= range2->lob && range1->upb >= range2->upb;
+}
+
 /*
  * Extend @range to the smallest interval that includes @extend_by, too.
  */
-- 
2.17.1

[Qemu-devel] [PATCH v2 5/7] memory-device: use QEMU_IS_ALIGNED

[David Hildenbrand]

Shorter and easier to read.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 7de1ccd497..996ad1490f 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -120,7 +120,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     g_assert(address_space_end >= address_space_start);
 
     /* address_space_start indicates the maximum alignment we expect */
-    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
+    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;
@@ -131,13 +131,13 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
         return 0;
     }
 
-    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
+    if (hint && !QEMU_IS_ALIGNED(*hint, align)) {
         error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
                    align);
         return 0;
     }
 
-    if (QEMU_ALIGN_UP(size, align) != size) {
+    if (!QEMU_IS_ALIGNED(size, align)) {
         error_setg(errp, "backend memory size must be multiple of 0x%"
                    PRIx64, align);
         return 0;
-- 
2.17.1

[Qemu-devel] [PATCH v2 6/7] memory-device: avoid overflows on very huge devices

[David Hildenbrand]

Should not be a problem right now, but it could theoretically happen
in the future.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 996ad1490f..8be63c8032 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
 
     /* will we exceed the total amount of memory specified */
     memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+    if (used_region_size + size < used_region_size ||
+        used_region_size + size > ms->maxram_size - ms->ram_size) {
         error_setg(errp, "not enough space, currently 0x%" PRIx64
                    " in use of total space for memory devices 0x" RAM_ADDR_FMT,
                    used_region_size, ms->maxram_size - ms->ram_size);
-- 
2.17.1

[Qemu-devel] [PATCH v2 7/7] memory-device: rewrite address assignment using ranges

[David Hildenbrand]

Let's rewrite it properly using ranges. This fixes certain overflows that
are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly reports an error instead of succeeding.

"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 8be63c8032..2fb6fc2145 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;
@@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], range overflow", *hint, size);
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
+                       "], range overflow", *hint, size);
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */
@@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));
 
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
         goto out;
     }
 out:
     g_slist_free(list);
-    return new_addr;
+    return range_lob(&new);
 }
 
 MemoryDeviceInfoList *qmp_memory_device_list(void)
-- 
2.17.1

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

[Markus Armbruster]

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.

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

[...]

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


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

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