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

[Qemu-devel] [PATCH v3 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 pulled soon.

v2 -> v3:
- "qapi: correctly parse uint64_t values from strings"
-- don't parse range
-- don't rename "parse_str"

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: use qemu_strtoi64() in parse_str
  qapi: correctly parse uint64_t values from strings
  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 | 34 ++++++++-----------
 3 files changed, 114 insertions(+), 48 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

The qemu api claims to be easier to use, and the resulting code seems to
agree.

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

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b3fdd0827d..c1454f999f 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -20,6 +20,7 @@
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
+#include "qemu/cutils.h"
 
 
 struct StringInputVisitor
@@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
 
 static int parse_str(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;
@@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
     }
 
     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);
@@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
                 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 v3 2/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. We don't have to worry about ranges.

E.g. we can now also specify
    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only 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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index c1454f999f..f2df027325 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -247,15 +247,16 @@ 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);
+    uint64_t val;
+
+    if (qemu_strtou64(siv->string, NULL, 0, &val)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                   "an uint64 value");
+        return;
     }
+
+    *obj = val;
 }
 
 static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
-- 
2.17.1

[Qemu-devel] [PATCH v3 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 v3 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 v3 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 v3 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 v3 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 v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]


On Tue, Oct 23, 2018 at 05:23:00PM +0200, David Hildenbrand wrote:
> The qemu api claims to be easier to use, and the resulting code seems to
> agree.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qapi/string-input-visitor.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..c1454f999f 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
>  #include "qemu/option.h"
>  #include "qemu/queue.h"
>  #include "qemu/range.h"
> +#include "qemu/cutils.h"
>  
>  
>  struct StringInputVisitor
> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>  
>  static int parse_str(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;
> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>      }
>  
>      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);
> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>                  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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Tue, Oct 23, 2018 at 05:23:01PM +0200, David Hildenbrand wrote:
> 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. We don't have to worry about ranges.
> 
> E.g. we can now also specify
>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
> Instead of only 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 | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index c1454f999f..f2df027325 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -247,15 +247,16 @@ 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);
> +    uint64_t val;
> +
> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                   "an uint64 value");
> +        return;
>      }
> +
> +    *obj = val;
>  }

It's not obvious to me why this looks so different from the code in
parse_type_int64().  Should we be using qemu_strtoi64() in the
pre-existing function, instead of what's there now?

>  
>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On Tue, Oct 23, 2018 at 05:23:02PM +0200, David Hildenbrand wrote:
> 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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

On Tue, Oct 23, 2018 at 05:23:04PM +0200, David Hildenbrand wrote:
> Shorter and easier to read.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

On Tue, Oct 23, 2018 at 05:23:05PM +0200, David Hildenbrand wrote:
> Should not be a problem right now, but it could theoretically happen
> in the future.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Igor Mammedov]

On Tue, 23 Oct 2018 17:23:05 +0200
David Hildenbrand <david@redhat.com> wrote:

> Should not be a problem right now, but it could theoretically happen
> in the future.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@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);

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

[David Hildenbrand]

> 
> It's not obvious to me why this looks so different from the code in
> parse_type_int64().  Should we be using qemu_strtoi64() in the
> pre-existing function, instead of what's there now?

The existing function has to be that complicated because it calls into
the same function used to parse ranges. We don't need ranges (or
create/modify) any, so this is not necessary.

This function is similar to the other parse functions (not parsing
ranges), e.g. parse_type_bool(). Thanks!

> 
>>  
>>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 23.10.18 17:22, David Hildenbrand wrote:
> 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 pulled soon.
> 
> v2 -> v3:
> - "qapi: correctly parse uint64_t values from strings"
> -- don't parse range
> -- don't rename "parse_str"
> 
> 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: use qemu_strtoi64() in parse_str
>   qapi: correctly parse uint64_t values from strings
>   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 | 34 ++++++++-----------
>  3 files changed, 114 insertions(+), 48 deletions(-)
> 

Any more comments? If not, I think this is good to go.

-- 

Thanks,

David / dhildenb

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

[Markus Armbruster]

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. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about
ranges.

>
> E.g. we can now also specify
>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
> Instead of only 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
>

Suggest to mention this makes the string-input-visitor catch up with the
qobject-input-visitor, which got changed similarly in commit
5923f85fb82.

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  qapi/string-input-visitor.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index c1454f999f..f2df027325 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -247,15 +247,16 @@ 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);
> +    uint64_t val;
> +
> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {

Works because qemu_strtou64() accepts negative numbers and interprets
them modulo 2^64.

> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                   "an uint64 value");

I think this should be "a uint64 value".

> +        return;
>      }
> +
> +    *obj = val;
>  }
>  
>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

Patch looks good to me otherwise.

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> The qemu api claims to be easier to use, and the resulting code seems to
> agree.

Ah, an opportunity to nitpick spelling!  "The QEMU API", and "qapi: Use
qemu_strtoi64() ..."

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  qapi/string-input-visitor.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..c1454f999f 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
>  #include "qemu/option.h"
>  #include "qemu/queue.h"
>  #include "qemu/range.h"
> +#include "qemu/cutils.h"
>  
>  
>  struct StringInputVisitor
> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>  
>  static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>  {
> -    char *str = (char *) siv->string;
> -    long long start, end;
> +    const char *str = (char *) siv->string;

And an opportunity to nitpick whitespace!  Drop the space between (char
*) and siv->string while there.

> +    const char *endptr;
> +    int64_t start, end;
>      Range *cur;
> -    char *endptr;
>  
>      if (siv->ranges) {
>          return 0;
> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>      }
>  
>      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);
> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>                  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) {

You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
explain that to me?  I'm feeling particularly dense today...

>                      if (*endptr == '\0') {
>                          cur = g_malloc0(sizeof(*cur));
>                          range_set_bounds(cur, start, end);

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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. We don't have to worry about ranges.
>
> The commit message should mention *why* we don't we have to worry about
> ranges.
>
>>
>> E.g. we can now also specify
>>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
>> Instead of only 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
>>
>
> Suggest to mention this makes the string-input-visitor catch up with the
> qobject-input-visitor, which got changed similarly in commit
> 5923f85fb82.

One more thing: the qobject-input-visitor change also updated the
corresponding output visitor.  Shouldn't we do the same here?

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 31.10.18 15:40, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> The qemu api claims to be easier to use, and the resulting code seems to
>> agree.
> 
> Ah, an opportunity to nitpick spelling!  "The QEMU API", and "qapi: Use
> qemu_strtoi64() ..."

Whatever floats your boat ;) Will change.

> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  qapi/string-input-visitor.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b3fdd0827d..c1454f999f 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -20,6 +20,7 @@
>>  #include "qemu/option.h"
>>  #include "qemu/queue.h"
>>  #include "qemu/range.h"
>> +#include "qemu/cutils.h"
>>  
>>  
>>  struct StringInputVisitor
>> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>>  
>>  static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>  {
>> -    char *str = (char *) siv->string;
>> -    long long start, end;
>> +    const char *str = (char *) siv->string;
> 
> And an opportunity to nitpick whitespace!  Drop the space between (char
> *) and siv->string while there.

Shouldn't checkpatch complain, too? Will fix.

> 
>> +    const char *endptr;
>> +    int64_t start, end;
>>      Range *cur;
>> -    char *endptr;
>>  
>>      if (siv->ranges) {
>>          return 0;
>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>      }
>>  
>>      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);
>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>                  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) {
> 
> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
> explain that to me?  I'm feeling particularly dense today...

qemu_strtoi64 performs all different kinds of error handling completely
internally. This old code here was an attempt to filter out -EWHATEVER
from the response. No longer needed as errors and the actual value are
reported via different ways.

Thanks!

> 
>>                      if (*endptr == '\0') {
>>                          cur = g_malloc0(sizeof(*cur));
>>                          range_set_bounds(cur, start, end);


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 31.10.18 15:32, Markus Armbruster wrote:
> 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. We don't have to worry about ranges.
> 
> The commit message should mention *why* we don't we have to worry about
> ranges.

"Parse uin64_t separately. We don't have to worry about ranges as far as
I can see. Ranges are parsed and processed via start_list()/next_list()
and friends. parse_type_int64() only has to deal with ranges as it
reuses the function parse_str(). E.g. parse_type_size() also does not
have to handle ranges. (I assume that we could easily reimplement
parse_type_int64() in a similar fashion, too).

The only thing that will change is that uint64_t properties that didn't
expect a range will now actually bail out if a range is supplied."

I'll do some more testing.

> 
>>
>> E.g. we can now also specify
>>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
>> Instead of only 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
>>
> 
> Suggest to mention this makes the string-input-visitor catch up with the
> qobject-input-visitor, which got changed similarly in commit
> 5923f85fb82.

Yes, I will add that!

> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  qapi/string-input-visitor.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index c1454f999f..f2df027325 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -247,15 +247,16 @@ 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);
>> +    uint64_t val;
>> +
>> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> 
> Works because qemu_strtou64() accepts negative numbers and interprets
> them modulo 2^64.

I will also add a comment to the description that negative numbers will
continue to work.

> 
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> +                   "an uint64 value");
> 
> I think this should be "a uint64 value".

As I am not a native speaker, I will stick to your suggestion unless
somebody else speaks up.

> 
>> +        return;
>>      }
>> +
>> +    *obj = val;
>>  }
>>  
>>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> 
> Patch looks good to me otherwise.
> 

Thanks!

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 31.10.18 15:44, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> 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. We don't have to worry about ranges.
>>
>> The commit message should mention *why* we don't we have to worry about
>> ranges.
>>
>>>
>>> E.g. we can now also specify
>>>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
>>> Instead of only 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
>>>
>>
>> Suggest to mention this makes the string-input-visitor catch up with the
>> qobject-input-visitor, which got changed similarly in commit
>> 5923f85fb82.
> 
> One more thing: the qobject-input-visitor change also updated the
> corresponding output visitor.  Shouldn't we do the same here?
> 

I'll have a look if something has to be done on that side.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 31.10.18 15:40, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> The qemu api claims to be easier to use, and the resulting code seems to
>>> agree.
[...]
>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>      }
>>>  
>>>      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);
>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>                  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) {
>> 
>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>> explain that to me?  I'm feeling particularly dense today...
>
> qemu_strtoi64 performs all different kinds of error handling completely
> internally. This old code here was an attempt to filter out -EWHATEVER
> from the response. No longer needed as errors and the actual value are
> reported via different ways.

I understand why errno == 0 && endptr > str go away.  They also do in
the previous hunk.

The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
unobvious.  What does it do before the patch?

The condition goes back to commit 659268ffbff, which predates my watch
as maintainer.  Its commit message is of no particular help.  Its code
is... allright, the less I say about that, the better.

We're parsing a range here.  We already parsed its lower bound into
@start (and guarded against errors), and its upper bound into @end (and
guarded against errors).

If the condition you delete is false, we goto error.  So the condition
is about range validity.  I figure it's an attempt to require valid
ranges to be no "wider" than 65535.  The second part end < start + 65536
checks exactly that, except shit happens when start + 65536 overflows.
The first part attempts to guard against that, but

(1) INT64_MAX is *wrong*, because we compute in long long, and

(2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.

WTF?!?

Unless I'm mistaken, the condition is not about handling any of the
errors that qemu_strtoi64() handles for us.

The easiest way for you out of this morass is probably to keep the
condition exactly as it was, then use the "my patch doesn't make things
any worse" get-out-of-jail-free card.

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 31.10.18 18:55, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 31.10.18 15:40, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>> agree.
> [...]
>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>      }
>>>>  
>>>>      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);
>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>                  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) {
>>>
>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>> explain that to me?  I'm feeling particularly dense today...
>>
>> qemu_strtoi64 performs all different kinds of error handling completely
>> internally. This old code here was an attempt to filter out -EWHATEVER
>> from the response. No longer needed as errors and the actual value are
>> reported via different ways.
> 
> I understand why errno == 0 && endptr > str go away.  They also do in
> the previous hunk.
> 
> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
> unobvious.  What does it do before the patch?
> 
> The condition goes back to commit 659268ffbff, which predates my watch
> as maintainer.  Its commit message is of no particular help.  Its code
> is... allright, the less I say about that, the better.
> 
> We're parsing a range here.  We already parsed its lower bound into
> @start (and guarded against errors), and its upper bound into @end (and
> guarded against errors).
> 
> If the condition you delete is false, we goto error.  So the condition
> is about range validity.  I figure it's an attempt to require valid
> ranges to be no "wider" than 65535.  The second part end < start + 65536
> checks exactly that, except shit happens when start + 65536 overflows.
> The first part attempts to guard against that, but
> 
> (1) INT64_MAX is *wrong*, because we compute in long long, and
> 
> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
> 
> WTF?!?
> 
> Unless I'm mistaken, the condition is not about handling any of the
> errors that qemu_strtoi64() handles for us.
> 
> The easiest way for you out of this morass is probably to keep the
> condition exactly as it was, then use the "my patch doesn't make things
> any worse" get-out-of-jail-free card.
> 

Looking at the code in qapi/string-output-visitor.c related to range and
list handling I feel like using the get-out-of-jail-free card to get out
of qapi code now :) Too much magic in that code and too little time for
me to understand it all.

Thanks for your time and review anyway. My time is better invested in
other parts of QEMU. I will drop both patches from this series.

-- 

Thanks,

David / dhildenb

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

[Eduardo Habkost]

On Wed, Oct 31, 2018 at 11:14:48AM +0100, David Hildenbrand wrote:
> On 23.10.18 17:22, David Hildenbrand wrote:
> > 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 pulled soon.
> > 
> > v2 -> v3:
> > - "qapi: correctly parse uint64_t values from strings"
> > -- don't parse range
> > -- don't rename "parse_str"
> > 
> > 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: use qemu_strtoi64() in parse_str
> >   qapi: correctly parse uint64_t values from strings
> >   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 | 34 ++++++++-----------
> >  3 files changed, 114 insertions(+), 48 deletions(-)
> > 
> 
> Any more comments? If not, I think this is good to go.

I'm assuming you want to drop patches 1 and 2, that's correct?

I'm queueing patches 3/7, 5/7, and 6/7 on machine-next.

Patch 4/7 still needs a reviewer.

Patch 7/7 seems to depend on patch 4, so I'm not queueing it yet.

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 23 Oct 2018 17:23:03 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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)) {
compilation fails with:
 "error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"

the same for range_invariant,
following should fix issues:

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 3586eb1..4b05bd1 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,7 +48,7 @@ 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;



> +        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.
>   */

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

[David Hildenbrand]

On 01.11.18 11:00, Igor Mammedov wrote:
> On Tue, 23 Oct 2018 17:23:03 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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)) {
> compilation fails with:
>  "error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"
> 
> the same for range_invariant,
> following should fix issues:

I guess you missed patch #3.


-- 

Thanks,

David / dhildenb

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

[Igor Mammedov]

On Thu, 1 Nov 2018 11:29:51 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 01.11.18 11:00, Igor Mammedov wrote:
> > On Tue, 23 Oct 2018 17:23:03 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> 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)) {  
> > compilation fails with:
> >  "error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"
> > 
> > the same for range_invariant,
> > following should fix issues:  
> 
> I guess you missed patch #3.
Yep, I was applying it randomly. I'd squash 3 and 4 but considering 3
it already queued  there is no point in doing so.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3585 bytes --]

On Wed, Oct 31, 2018 at 06:18:33PM +0100, David Hildenbrand wrote:
> On 31.10.18 15:32, Markus Armbruster wrote:
> > 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. We don't have to worry about ranges.
> > 
> > The commit message should mention *why* we don't we have to worry about
> > ranges.
> 
> "Parse uin64_t separately. We don't have to worry about ranges as far as
> I can see. Ranges are parsed and processed via start_list()/next_list()
> and friends. parse_type_int64() only has to deal with ranges as it
> reuses the function parse_str(). E.g. parse_type_size() also does not
> have to handle ranges. (I assume that we could easily reimplement
> parse_type_int64() in a similar fashion, too).
> 
> The only thing that will change is that uint64_t properties that didn't
> expect a range will now actually bail out if a range is supplied."
> 
> I'll do some more testing.
> 
> > 
> >>
> >> E.g. we can now also specify
> >>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
> >> Instead of only 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
> >>
> > 
> > Suggest to mention this makes the string-input-visitor catch up with the
> > qobject-input-visitor, which got changed similarly in commit
> > 5923f85fb82.
> 
> Yes, I will add that!
> 
> > 
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  qapi/string-input-visitor.c | 17 +++++++++--------
> >>  1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> >> index c1454f999f..f2df027325 100644
> >> --- a/qapi/string-input-visitor.c
> >> +++ b/qapi/string-input-visitor.c
> >> @@ -247,15 +247,16 @@ 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);
> >> +    uint64_t val;
> >> +
> >> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> > 
> > Works because qemu_strtou64() accepts negative numbers and interprets
> > them modulo 2^64.
> 
> I will also add a comment to the description that negative numbers will
> continue to work.
> 
> > 
> >> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> >> +                   "an uint64 value");
> > 
> > I think this should be "a uint64 value".
> 
> As I am not a native speaker, I will stick to your suggestion unless
> somebody else speaks up.

I am a native speaker and "a uint64 value" sounds better to me.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Hildenbrand]

On 01.11.18 12:05, Igor Mammedov wrote:
> On Thu, 1 Nov 2018 11:29:51 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 01.11.18 11:00, Igor Mammedov wrote:
>>> On Tue, 23 Oct 2018 17:23:03 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> 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)) {  
>>> compilation fails with:
>>>  "error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"
>>>
>>> the same for range_invariant,
>>> following should fix issues:  
>>
>> I guess you missed patch #3.
> Yep, I was applying it randomly. I'd squash 3 and 4 but considering 3
> it already queued  there is no point in doing so.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 

Thanks Igor, would be great if you (or one of the other CC people) could
have a look at the remaining patch #7. Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 31.10.18 18:55, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>> agree.
>> [...]
>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>      }
>>>>>  
>>>>>      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);
>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>                  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) {
>>>>
>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>> explain that to me?  I'm feeling particularly dense today...
>>>
>>> qemu_strtoi64 performs all different kinds of error handling completely
>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>> from the response. No longer needed as errors and the actual value are
>>> reported via different ways.
>> 
>> I understand why errno == 0 && endptr > str go away.  They also do in
>> the previous hunk.
>> 
>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>> unobvious.  What does it do before the patch?
>> 
>> The condition goes back to commit 659268ffbff, which predates my watch
>> as maintainer.  Its commit message is of no particular help.  Its code
>> is... allright, the less I say about that, the better.
>> 
>> We're parsing a range here.  We already parsed its lower bound into
>> @start (and guarded against errors), and its upper bound into @end (and
>> guarded against errors).
>> 
>> If the condition you delete is false, we goto error.  So the condition
>> is about range validity.  I figure it's an attempt to require valid
>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>> checks exactly that, except shit happens when start + 65536 overflows.
>> The first part attempts to guard against that, but
>> 
>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>> 
>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>> 
>> WTF?!?
>> 
>> Unless I'm mistaken, the condition is not about handling any of the
>> errors that qemu_strtoi64() handles for us.
>> 
>> The easiest way for you out of this morass is probably to keep the
>> condition exactly as it was, then use the "my patch doesn't make things
>> any worse" get-out-of-jail-free card.
>> 
>
> Looking at the code in qapi/string-output-visitor.c related to range and
> list handling I feel like using the get-out-of-jail-free card to get out
> of qapi code now :) Too much magic in that code and too little time for
> me to understand it all.
>
> Thanks for your time and review anyway. My time is better invested in
> other parts of QEMU. I will drop both patches from this series.

Understand.

When I first looked at the ranges stuff in the string input visitor, I
felt the urge to clean it up, then sat on my hands until it passed.

The rest is reasonable once you understand how it works.  The learning
curve is less than pleasant, though.

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 05.11.18 16:37, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 31.10.18 18:55, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>> agree.
>>> [...]
>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>      }
>>>>>>  
>>>>>>      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);
>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>                  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) {
>>>>>
>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>
>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>> from the response. No longer needed as errors and the actual value are
>>>> reported via different ways.
>>>
>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>> the previous hunk.
>>>
>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>> unobvious.  What does it do before the patch?
>>>
>>> The condition goes back to commit 659268ffbff, which predates my watch
>>> as maintainer.  Its commit message is of no particular help.  Its code
>>> is... allright, the less I say about that, the better.
>>>
>>> We're parsing a range here.  We already parsed its lower bound into
>>> @start (and guarded against errors), and its upper bound into @end (and
>>> guarded against errors).
>>>
>>> If the condition you delete is false, we goto error.  So the condition
>>> is about range validity.  I figure it's an attempt to require valid
>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>> checks exactly that, except shit happens when start + 65536 overflows.
>>> The first part attempts to guard against that, but
>>>
>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>
>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>
>>> WTF?!?
>>>
>>> Unless I'm mistaken, the condition is not about handling any of the
>>> errors that qemu_strtoi64() handles for us.
>>>
>>> The easiest way for you out of this morass is probably to keep the
>>> condition exactly as it was, then use the "my patch doesn't make things
>>> any worse" get-out-of-jail-free card.
>>>
>>
>> Looking at the code in qapi/string-output-visitor.c related to range and
>> list handling I feel like using the get-out-of-jail-free card to get out
>> of qapi code now :) Too much magic in that code and too little time for
>> me to understand it all.
>>
>> Thanks for your time and review anyway. My time is better invested in
>> other parts of QEMU. I will drop both patches from this series.
> 
> Understand.
> 
> When I first looked at the ranges stuff in the string input visitor, I
> felt the urge to clean it up, then sat on my hands until it passed.
> 
> The rest is reasonable once you understand how it works.  The learning
> curve is less than pleasant, though.
> 

Maybe I'll pick this up again when I have more time to invest.

The general concept

1. of having an input visitor that is able to parse different types
(expected by e.g. a property) sounds sane to me.

2. of having a list of *something*, assuming it is int64_t, and assuming
it is to be parsed into a list of ranges sounds completely broken to me.

I was not even able to find an example QEMU comand line for 2. Is this
maybe some very old code that nobody actually uses anymore? (who uses
list of ranges?)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Eric Blake]

On 11/5/18 9:53 AM, David Hildenbrand wrote:

>> When I first looked at the ranges stuff in the string input visitor, I
>> felt the urge to clean it up, then sat on my hands until it passed.
>>
>> The rest is reasonable once you understand how it works.  The learning
>> curve is less than pleasant, though.
>>
> 
> Maybe I'll pick this up again when I have more time to invest.
> 
> The general concept
> 
> 1. of having an input visitor that is able to parse different types
> (expected by e.g. a property) sounds sane to me.
> 
> 2. of having a list of *something*, assuming it is int64_t, and assuming
> it is to be parsed into a list of ranges sounds completely broken to me.
> 
> I was not even able to find an example QEMU comand line for 2. Is this
> maybe some very old code that nobody actually uses anymore? (who uses
> list of ranges?)

I believe that the -numa node,cpus=first-last code is an example use of 
a list of ranges.  At any rate, tests/test-string-input-visitor.c has 
test_visitor_in_intList() to ensure we don't break back-compat with the 
existing clients that use ranges, as awkward as they may be.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 05.11.18 16:37, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>> agree.
>>>> [...]
>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>      }
>>>>>>>  
>>>>>>>      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);
>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>                  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) {
>>>>>>
>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>>
>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>> from the response. No longer needed as errors and the actual value are
>>>>> reported via different ways.
>>>>
>>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>>> the previous hunk.
>>>>
>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>> unobvious.  What does it do before the patch?
>>>>
>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>> as maintainer.  Its commit message is of no particular help.  Its code
>>>> is... allright, the less I say about that, the better.
>>>>
>>>> We're parsing a range here.  We already parsed its lower bound into
>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>> guarded against errors).
>>>>
>>>> If the condition you delete is false, we goto error.  So the condition
>>>> is about range validity.  I figure it's an attempt to require valid
>>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>> The first part attempts to guard against that, but
>>>>
>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>
>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>
>>>> WTF?!?
>>>>
>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>> errors that qemu_strtoi64() handles for us.
>>>>
>>>> The easiest way for you out of this morass is probably to keep the
>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>> any worse" get-out-of-jail-free card.
>>>>
>>>
>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>> list handling I feel like using the get-out-of-jail-free card to get out
>>> of qapi code now :) Too much magic in that code and too little time for
>>> me to understand it all.
>>>
>>> Thanks for your time and review anyway. My time is better invested in
>>> other parts of QEMU. I will drop both patches from this series.
>> 
>> Understand.
>> 
>> When I first looked at the ranges stuff in the string input visitor, I
>> felt the urge to clean it up, then sat on my hands until it passed.
>> 
>> The rest is reasonable once you understand how it works.  The learning
>> curve is less than pleasant, though.
>> 
>
> Maybe I'll pick this up again when I have more time to invest.
>
> The general concept
>
> 1. of having an input visitor that is able to parse different types
> (expected by e.g. a property) sounds sane to me.
>
> 2. of having a list of *something*, assuming it is int64_t, and assuming
> it is to be parsed into a list of ranges sounds completely broken to me.

Starting point: the string visitors can only do scalars.  We have a need
for lists of integers (see below).  The general solution would be
generalizing these visitors to lists (and maybe objects while we're at
it).  YAGNI.  So we put in a quick hack that can do just lists of
integers.

Except applying YAGNI to stable interfaces is *bonkers*.

> I was not even able to find an example QEMU comand line for 2. Is this
> maybe some very old code that nobody actually uses anymore? (who uses
> list of ranges?)

The one I remember offhand is -numa node,cpus=..., but that one's
actually parsed with the options visitor.  Which is even hairier, but at
least competently coded.

To find uses, we need to follow the uses of the string visitors.

Of the callers of string_input_visitor_new(),
object_property_get_uint16List() is the only one that deals with lists.
It's used by query_memdev() for property host-nodes.

The callers of string_output_visitor_new() lead to MigrationInfo member
postcopy-vcpu-blocktime, and Memdev member host-nodes again.

Searching the QAPI schema for lists of integers coughs up a few more
candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
(likewise), block latency histogram stuff (likewise).

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 05.11.18 21:43, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 05.11.18 16:37, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>
>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>> agree.
>>>>> [...]
>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>      }
>>>>>>>>  
>>>>>>>>      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);
>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>                  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) {
>>>>>>>
>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>>>
>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>> reported via different ways.
>>>>>
>>>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>>>> the previous hunk.
>>>>>
>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>> unobvious.  What does it do before the patch?
>>>>>
>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>> as maintainer.  Its commit message is of no particular help.  Its code
>>>>> is... allright, the less I say about that, the better.
>>>>>
>>>>> We're parsing a range here.  We already parsed its lower bound into
>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>> guarded against errors).
>>>>>
>>>>> If the condition you delete is false, we goto error.  So the condition
>>>>> is about range validity.  I figure it's an attempt to require valid
>>>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>> The first part attempts to guard against that, but
>>>>>
>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>
>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>
>>>>> WTF?!?
>>>>>
>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>> errors that qemu_strtoi64() handles for us.
>>>>>
>>>>> The easiest way for you out of this morass is probably to keep the
>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>> any worse" get-out-of-jail-free card.
>>>>>
>>>>
>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>> of qapi code now :) Too much magic in that code and too little time for
>>>> me to understand it all.
>>>>
>>>> Thanks for your time and review anyway. My time is better invested in
>>>> other parts of QEMU. I will drop both patches from this series.
>>>
>>> Understand.
>>>
>>> When I first looked at the ranges stuff in the string input visitor, I
>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>
>>> The rest is reasonable once you understand how it works.  The learning
>>> curve is less than pleasant, though.
>>>
>>
>> Maybe I'll pick this up again when I have more time to invest.
>>
>> The general concept
>>
>> 1. of having an input visitor that is able to parse different types
>> (expected by e.g. a property) sounds sane to me.
>>
>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>> it is to be parsed into a list of ranges sounds completely broken to me.
> 
> Starting point: the string visitors can only do scalars.  We have a need
> for lists of integers (see below).  The general solution would be
> generalizing these visitors to lists (and maybe objects while we're at
> it).  YAGNI.  So we put in a quick hack that can do just lists of
> integers.
> 
> Except applying YAGNI to stable interfaces is *bonkers*.
> 
>> I was not even able to find an example QEMU comand line for 2. Is this
>> maybe some very old code that nobody actually uses anymore? (who uses
>> list of ranges?)
> 
> The one I remember offhand is -numa node,cpus=..., but that one's
> actually parsed with the options visitor.  Which is even hairier, but at
> least competently coded.
> 
> To find uses, we need to follow the uses of the string visitors.
> 
> Of the callers of string_input_visitor_new(),
> object_property_get_uint16List() is the only one that deals with lists.
> It's used by query_memdev() for property host-nodes.
> 
> The callers of string_output_visitor_new() lead to MigrationInfo member
> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
> 
> Searching the QAPI schema for lists of integers coughs up a few more
> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
> (likewise), block latency histogram stuff (likewise).
> 

As Eric pointed out, tests/test-string-input-visitor.c actually tests
for range support in test_visitor_in_intList.

I might be completely wrong, but actually the string input visitor
should not pre-parse stuff into a list of ranges, but instead parse on
request (parse_type_...) and advance in the logical list on "next_list".
And we should parse ranges *only* if we are expecting a list. Because a
range is simply a short variant of a list. A straight parse_type_uint64
should bail out if we haven't started a list.

I guess I am starting to understand how this magic is supposed to work.
Always parsing and processing one list token at a time
("size"/"uint64_t" or "range of such") should be the way to go. And if
nobody requested to parse a list (start_list()), also ranges should not
be allowed. This pre-parsing of the whole list and unconditional use of
ranges should go.

Ranges are still ugly but needed as far as I can understand (as a
shortcut for lengthy lists).

Am I on the right track?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 05.11.18 17:48, Eric Blake wrote:
> On 11/5/18 9:53 AM, David Hildenbrand wrote:
> 
>>> When I first looked at the ranges stuff in the string input visitor, I
>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>
>>> The rest is reasonable once you understand how it works.  The learning
>>> curve is less than pleasant, though.
>>>
>>
>> Maybe I'll pick this up again when I have more time to invest.
>>
>> The general concept
>>
>> 1. of having an input visitor that is able to parse different types
>> (expected by e.g. a property) sounds sane to me.
>>
>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>> it is to be parsed into a list of ranges sounds completely broken to me.
>>
>> I was not even able to find an example QEMU comand line for 2. Is this
>> maybe some very old code that nobody actually uses anymore? (who uses
>> list of ranges?)
> 
> I believe that the -numa node,cpus=first-last code is an example use of 
> a list of ranges.  At any rate, tests/test-string-input-visitor.c has 
> test_visitor_in_intList() to ensure we don't break back-compat with the 
> existing clients that use ranges, as awkward as they may be.
> 

Indeed, thanks for the pointer. Ranges should indeed be supported. But
maybe we can refactor this ugly "pre parse all uint64_t ranges" thingy.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 05.11.18 21:43, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>
>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>> agree.
>>>>>> [...]
>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>      }
>>>>>>>>>  
>>>>>>>>>      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);
>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>                  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) {
>>>>>>>>
>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>>>>
>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>> reported via different ways.
>>>>>>
>>>>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>>>>> the previous hunk.
>>>>>>
>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>> unobvious.  What does it do before the patch?
>>>>>>
>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>> as maintainer.  Its commit message is of no particular help.  Its code
>>>>>> is... allright, the less I say about that, the better.
>>>>>>
>>>>>> We're parsing a range here.  We already parsed its lower bound into
>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>> guarded against errors).
>>>>>>
>>>>>> If the condition you delete is false, we goto error.  So the condition
>>>>>> is about range validity.  I figure it's an attempt to require valid
>>>>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>> The first part attempts to guard against that, but
>>>>>>
>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>
>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>
>>>>>> WTF?!?
>>>>>>
>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>
>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>> any worse" get-out-of-jail-free card.
>>>>>>
>>>>>
>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>> me to understand it all.
>>>>>
>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>
>>>> Understand.
>>>>
>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>
>>>> The rest is reasonable once you understand how it works.  The learning
>>>> curve is less than pleasant, though.
>>>>
>>>
>>> Maybe I'll pick this up again when I have more time to invest.
>>>
>>> The general concept
>>>
>>> 1. of having an input visitor that is able to parse different types
>>> (expected by e.g. a property) sounds sane to me.
>>>
>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>> it is to be parsed into a list of ranges sounds completely broken to me.
>> 
>> Starting point: the string visitors can only do scalars.  We have a need
>> for lists of integers (see below).  The general solution would be
>> generalizing these visitors to lists (and maybe objects while we're at
>> it).  YAGNI.  So we put in a quick hack that can do just lists of
>> integers.
>> 
>> Except applying YAGNI to stable interfaces is *bonkers*.
>> 
>>> I was not even able to find an example QEMU comand line for 2. Is this
>>> maybe some very old code that nobody actually uses anymore? (who uses
>>> list of ranges?)
>> 
>> The one I remember offhand is -numa node,cpus=..., but that one's
>> actually parsed with the options visitor.  Which is even hairier, but at
>> least competently coded.
>> 
>> To find uses, we need to follow the uses of the string visitors.
>> 
>> Of the callers of string_input_visitor_new(),
>> object_property_get_uint16List() is the only one that deals with lists.
>> It's used by query_memdev() for property host-nodes.
>> 
>> The callers of string_output_visitor_new() lead to MigrationInfo member
>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>> 
>> Searching the QAPI schema for lists of integers coughs up a few more
>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>> (likewise), block latency histogram stuff (likewise).
>> 
>
> As Eric pointed out, tests/test-string-input-visitor.c actually tests
> for range support in test_visitor_in_intList.
>
> I might be completely wrong, but actually the string input visitor
> should not pre-parse stuff into a list of ranges, but instead parse on
> request (parse_type_...) and advance in the logical list on "next_list".
> And we should parse ranges *only* if we are expecting a list. Because a
> range is simply a short variant of a list. A straight parse_type_uint64
> should bail out if we haven't started a list.

Yes, parse_type_int64() & friends should simply parse the appropriate
integer, *except* when we're working on a list.  Then they should return
the next integer, which may or may not require parsing.

Say, input is "1-3,5", and the visitor is called like

    visit_start_list()
    visit_next_list()   more input, returns "there's more"
    visit_type_int()    parses "1-3,", buffers 2-3, returns 1
    visit_next_list()   buffer not empty, returns "there's more"
    visit_type_int()    unbuffers and returns 2
    visit_next_list()   buffer not empty, returns "there's more"
    visit_type_int()    unbuffers and returns 3 
    visit_next_list()   more input, returns "there's more"
    visit_type_int()    parses "5", returns 5
    visit_next_list()   buffer empty and no more input, returns "done"
    visit_end_list()   

Alternatively, parse and buffer the whole list at once.

> I guess I am starting to understand how this magic is supposed to work.
> Always parsing and processing one list token at a time
> ("size"/"uint64_t" or "range of such") should be the way to go. And if
> nobody requested to parse a list (start_list()), also ranges should not
> be allowed. This pre-parsing of the whole list and unconditional use of
> ranges should go.
>
> Ranges are still ugly but needed as far as I can understand (as a
> shortcut for lengthy lists).
>
> Am I on the right track?

I believe you are.

The overall problem is to convert between text and (QAPI-generated) C
data structures.

We have a "low magic" solution for JSON text: we split the problem like

    JSON text --> QObject --> C data --> QObject --> JSON text
               |           |          |           |
           JSON parser     |          |     JSON formatter
                QObject input visitor |
                            QObject output visitor

The JSON parser is slightly magical around numbers.  Everything else is
straightforward.

We have a "moderate magic" solution for key-value text (used for option
arguments):

    key-value text --> QObject --> C data
                    |           |
              keyval_parse()    |
                      QObject input visitor
                         keyval variant

Key-value text is less expressive than JSON: it can't distinguish the
scalar types (everthing's a string), and it can't do empty arrays or
empty non-root objects.  The visitor magically converts strings to
whatever type is expected.

We have a "bad magic" solution for "strings":

    string --> C data --> string
            |          |
  string input visitor |
              string output visitor

Initially, this visitor was simple: only scalars.  Adding ranges was a
misguided idea.  The way they were coded should never have passed
review.

We have a "more bad magic" solution for certain option arguments:

    key-value text --> QemuOpts --> C data
                    |            |
             qemu_opts_parse()   |
                            opts visitor

Predates the other key-value solution.  Less general (by design), and
nevertheless more complex.  I hope the other one can replace it one day.

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 07.11.18 16:29, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 05.11.18 21:43, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>
>>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>>
>>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>>> agree.
>>>>>>> [...]
>>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>      }
>>>>>>>>>>  
>>>>>>>>>>      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);
>>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>                  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) {
>>>>>>>>>
>>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>>>>>
>>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>>> reported via different ways.
>>>>>>>
>>>>>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>>>>>> the previous hunk.
>>>>>>>
>>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>>> unobvious.  What does it do before the patch?
>>>>>>>
>>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>>> as maintainer.  Its commit message is of no particular help.  Its code
>>>>>>> is... allright, the less I say about that, the better.
>>>>>>>
>>>>>>> We're parsing a range here.  We already parsed its lower bound into
>>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>>> guarded against errors).
>>>>>>>
>>>>>>> If the condition you delete is false, we goto error.  So the condition
>>>>>>> is about range validity.  I figure it's an attempt to require valid
>>>>>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>>> The first part attempts to guard against that, but
>>>>>>>
>>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>>
>>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>>
>>>>>>> WTF?!?
>>>>>>>
>>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>>
>>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>>> any worse" get-out-of-jail-free card.
>>>>>>>
>>>>>>
>>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>>> me to understand it all.
>>>>>>
>>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>>
>>>>> Understand.
>>>>>
>>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>>
>>>>> The rest is reasonable once you understand how it works.  The learning
>>>>> curve is less than pleasant, though.
>>>>>
>>>>
>>>> Maybe I'll pick this up again when I have more time to invest.
>>>>
>>>> The general concept
>>>>
>>>> 1. of having an input visitor that is able to parse different types
>>>> (expected by e.g. a property) sounds sane to me.
>>>>
>>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>>
>>> Starting point: the string visitors can only do scalars.  We have a need
>>> for lists of integers (see below).  The general solution would be
>>> generalizing these visitors to lists (and maybe objects while we're at
>>> it).  YAGNI.  So we put in a quick hack that can do just lists of
>>> integers.
>>>
>>> Except applying YAGNI to stable interfaces is *bonkers*.
>>>
>>>> I was not even able to find an example QEMU comand line for 2. Is this
>>>> maybe some very old code that nobody actually uses anymore? (who uses
>>>> list of ranges?)
>>>
>>> The one I remember offhand is -numa node,cpus=..., but that one's
>>> actually parsed with the options visitor.  Which is even hairier, but at
>>> least competently coded.
>>>
>>> To find uses, we need to follow the uses of the string visitors.
>>>
>>> Of the callers of string_input_visitor_new(),
>>> object_property_get_uint16List() is the only one that deals with lists.
>>> It's used by query_memdev() for property host-nodes.
>>>
>>> The callers of string_output_visitor_new() lead to MigrationInfo member
>>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>>
>>> Searching the QAPI schema for lists of integers coughs up a few more
>>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>>> (likewise), block latency histogram stuff (likewise).
>>>
>>
>> As Eric pointed out, tests/test-string-input-visitor.c actually tests
>> for range support in test_visitor_in_intList.
>>
>> I might be completely wrong, but actually the string input visitor
>> should not pre-parse stuff into a list of ranges, but instead parse on
>> request (parse_type_...) and advance in the logical list on "next_list".
>> And we should parse ranges *only* if we are expecting a list. Because a
>> range is simply a short variant of a list. A straight parse_type_uint64
>> should bail out if we haven't started a list.
> 
> Yes, parse_type_int64() & friends should simply parse the appropriate
> integer, *except* when we're working on a list.  Then they should return
> the next integer, which may or may not require parsing.
> 
> Say, input is "1-3,5", and the visitor is called like
> 
>     visit_start_list()
>     visit_next_list()   more input, returns "there's more"
>     visit_type_int()    parses "1-3,", buffers 2-3, returns 1
>     visit_next_list()   buffer not empty, returns "there's more"
>     visit_type_int()    unbuffers and returns 2
>     visit_next_list()   buffer not empty, returns "there's more"
>     visit_type_int()    unbuffers and returns 3 
>     visit_next_list()   more input, returns "there's more"
>     visit_type_int()    parses "5", returns 5
>     visit_next_list()   buffer empty and no more input, returns "done"
>     visit_end_list()   
> 

Would it be valid to do something like this (skipping elements without a
proper visit_type_int)

visit_start_list();
visit_next_list();  more input, returns "there's more"
visit_next_list();  parses "1-3,", buffers 2-3, skips over 1
visit_type_int();   returns 2
...

Or mixing types

visit_start_list();
visit_next_list();
visit_type_int64();
visit_next_list();
visit_type_uint64();

IOW, can I assume that after every visit_next_list(), visit_type_X is
called, and that X remains the same for one pass over the list?

Also, I assume it is supposed to work like this

visit_start_list();
visit_next_list();  more input, returns "there's more"
visit_type_int();   returns 1 (parses 1-3, buffers 1-3)
visit_type_int();   returns 1
visit_type_int();   returns 1
visit_next_list();  more input, unbuffers 1
visit_type_int();   returns 2

So unbuffering actually happens on visit_next_list()?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 07.11.18 16:29, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 05.11.18 21:43, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>
>>>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>>>
>>>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>>>> agree.
>>>>>>>> [...]
>>>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>>      }
>>>>>>>>>>>  
>>>>>>>>>>>      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);
>>>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>>                  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) {
>>>>>>>>>>
>>>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
>>>>>>>>>> explain that to me?  I'm feeling particularly dense today...
>>>>>>>>>
>>>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>>>> reported via different ways.
>>>>>>>>
>>>>>>>> I understand why errno == 0 && endptr > str go away.  They also do in
>>>>>>>> the previous hunk.
>>>>>>>>
>>>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>>>> unobvious.  What does it do before the patch?
>>>>>>>>
>>>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>>>> as maintainer.  Its commit message is of no particular help.  Its code
>>>>>>>> is... allright, the less I say about that, the better.
>>>>>>>>
>>>>>>>> We're parsing a range here.  We already parsed its lower bound into
>>>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>>>> guarded against errors).
>>>>>>>>
>>>>>>>> If the condition you delete is false, we goto error.  So the condition
>>>>>>>> is about range validity.  I figure it's an attempt to require valid
>>>>>>>> ranges to be no "wider" than 65535.  The second part end < start + 65536
>>>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>>>> The first part attempts to guard against that, but
>>>>>>>>
>>>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>>>
>>>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>>>
>>>>>>>> WTF?!?
>>>>>>>>
>>>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>>>
>>>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>>>> any worse" get-out-of-jail-free card.
>>>>>>>>
>>>>>>>
>>>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>>>> me to understand it all.
>>>>>>>
>>>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>>>
>>>>>> Understand.
>>>>>>
>>>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>>>
>>>>>> The rest is reasonable once you understand how it works.  The learning
>>>>>> curve is less than pleasant, though.
>>>>>>
>>>>>
>>>>> Maybe I'll pick this up again when I have more time to invest.
>>>>>
>>>>> The general concept
>>>>>
>>>>> 1. of having an input visitor that is able to parse different types
>>>>> (expected by e.g. a property) sounds sane to me.
>>>>>
>>>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>>>
>>>> Starting point: the string visitors can only do scalars.  We have a need
>>>> for lists of integers (see below).  The general solution would be
>>>> generalizing these visitors to lists (and maybe objects while we're at
>>>> it).  YAGNI.  So we put in a quick hack that can do just lists of
>>>> integers.
>>>>
>>>> Except applying YAGNI to stable interfaces is *bonkers*.
>>>>
>>>>> I was not even able to find an example QEMU comand line for 2. Is this
>>>>> maybe some very old code that nobody actually uses anymore? (who uses
>>>>> list of ranges?)
>>>>
>>>> The one I remember offhand is -numa node,cpus=..., but that one's
>>>> actually parsed with the options visitor.  Which is even hairier, but at
>>>> least competently coded.
>>>>
>>>> To find uses, we need to follow the uses of the string visitors.
>>>>
>>>> Of the callers of string_input_visitor_new(),
>>>> object_property_get_uint16List() is the only one that deals with lists.
>>>> It's used by query_memdev() for property host-nodes.
>>>>
>>>> The callers of string_output_visitor_new() lead to MigrationInfo member
>>>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>>>
>>>> Searching the QAPI schema for lists of integers coughs up a few more
>>>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>>>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>>>> (likewise), block latency histogram stuff (likewise).
>>>>
>>>
>>> As Eric pointed out, tests/test-string-input-visitor.c actually tests
>>> for range support in test_visitor_in_intList.
>>>
>>> I might be completely wrong, but actually the string input visitor
>>> should not pre-parse stuff into a list of ranges, but instead parse on
>>> request (parse_type_...) and advance in the logical list on "next_list".
>>> And we should parse ranges *only* if we are expecting a list. Because a
>>> range is simply a short variant of a list. A straight parse_type_uint64
>>> should bail out if we haven't started a list.
>> 
>> Yes, parse_type_int64() & friends should simply parse the appropriate
>> integer, *except* when we're working on a list.  Then they should return
>> the next integer, which may or may not require parsing.
>> 
>> Say, input is "1-3,5", and the visitor is called like
>> 
>>     visit_start_list()
>>     visit_next_list()   more input, returns "there's more"
>>     visit_type_int()    parses "1-3,", buffers 2-3, returns 1
>>     visit_next_list()   buffer not empty, returns "there's more"
>>     visit_type_int()    unbuffers and returns 2
>>     visit_next_list()   buffer not empty, returns "there's more"
>>     visit_type_int()    unbuffers and returns 3 
>>     visit_next_list()   more input, returns "there's more"
>>     visit_type_int()    parses "5", returns 5
>>     visit_next_list()   buffer empty and no more input, returns "done"
>>     visit_end_list()   
>> 
>
> Would it be valid to do something like this (skipping elements without a
> proper visit_type_int)
>
> visit_start_list();
> visit_next_list();  more input, returns "there's more"
> visit_next_list();  parses "1-3,", buffers 2-3, skips over 1
> visit_type_int();   returns 2
> ...

Excellent question!

Here's the relevant part of visit_start_list()'s contract in visitor.h:

 * After visit_start_list() succeeds, the caller may visit its members
 * one after the other.  A real visit (where @obj is non-NULL) uses
 * visit_next_list() for traversing the linked list, while a virtual
 * visit (where @obj is NULL) uses other means.  For each list
 * element, call the appropriate visit_type_FOO() with name set to
 * NULL and obj set to the address of the value member of the list
 * element.  Finally, visit_end_list() needs to be called with the
 * same @list to clean up, even if intermediate visits fail.  See the
 * examples above.

So, you *may* visit members, and you *must* call visit_end_list().

But what are "real" and "virtual" visits?  Again, the contract:

 * @list must be non-NULL for a real walk, in which case @size
 * determines how much memory an input or clone visitor will allocate
 * into *@list (at least sizeof(GenericList)).  Some visitors also
 * allow @list to be NULL for a virtual walk, in which case @size is
 * ignored.

I'm not sure whether I just decreased or increased global confusion ;)

The file comment explains:

 * The QAPI schema defines both a set of C data types, and a QMP wire
 * format.  QAPI objects can contain references to other QAPI objects,
 * resulting in a directed acyclic graph.  QAPI also generates visitor
 * functions to walk these graphs.  This file represents the interface
 * for doing work at each node of a QAPI graph; it can also be used
 * for a virtual walk, where there is no actual QAPI C struct.

A real walk with an output visitor works like this (error handling
omitted for now):

    Error *err = NULL;
    intList *tail;
    size_t size = sizeof(**obj);

    // fetch list's head into *obj, start the list in output
    visit_start_list(v, name, (GenericList **)obj, size, &err);

    // iterate over list's tails
    // below the hood, visit_next_list() iterates over the GenericList
    for (tail = *obj; tail;
         tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
        // visit current tail's first member, add it to output
        visit_type_int(v, NULL, &tail->value, &err);
    }

    // end the list in output
    visit_end_list(v, (void **)obj);

The exact same code works for a real walk with an input visitor, where
visit_next_list() iterates over the *input* and builds up the
GenericList.  Compare qobject_input_next_list() and
qobject_output_next_list().

The code above is a straight copy of generated visit_type_intList() with
error handling cut and comments added.

A real walk works on a QAPI-generated C type.  QAPI generates a real
walk for each such type.  Open-coding a real walk would senselessly
duplicate the generated one, so we don't.  Thus, a real walk always
visits each member.

Okay, I lied: it visits each member until it runs into an error and
bails out.  See generated visit_type_intList() in
qapi/qapi-builtin-visit.c.

A virtual walk doesn't work with a GenericList *.  Calling
visit_next_list() makes no sense then.  visitor.h gives this example of
a virtual walk:

 * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
 * like:
 *
 * <example>
 *  Visitor *v;
 *  Error *err = NULL;
 *  int value;
 *
 *  v = FOO_visitor_new(...);
 *  visit_start_struct(v, NULL, NULL, 0, &err);
 *  if (err) {
 *      goto out;
 *  }
 *  visit_start_list(v, "list", NULL, 0, &err);
 *  if (err) {
 *      goto outobj;
 *  }
 *  value = 1;
 *  visit_type_int(v, NULL, &value, &err);
 *  if (err) {
 *      goto outlist;
 *  }
 *  value = 2;
 *  visit_type_int(v, NULL, &value, &err);
 *  if (err) {
 *      goto outlist;
 *  }
 * outlist:
 *  visit_end_list(v, NULL);
 *  if (!err) {
 *      visit_check_struct(v, &err);
 *  }
 * outobj:
 *  visit_end_struct(v, NULL);
 * out:
 *  error_propagate(errp, err);
 *  visit_free(v);

Why could this be useful?

1. With the QObject input visitor, it's an alternative way to
   destructure a QObject into a bunch of C variables.  The "obvious" way
   would be calling the QObject accessors.  By using the visitors you
   get much the error checking code for free.  YMMV.

2. With the QObject output visitor, it's an alternative way to build up
   a QObject.  The "obvious" way would be calling the constructors
   directly.

3. With the string input / output visitors, it's a way to parse / format
   string visitor syntax without having to construct the C type first.

Right now, we have no virtual list walks outside tests.  We do have
virtual struct walks outside tests.

> Or mixing types
>
> visit_start_list();
> visit_next_list();
> visit_type_int64();
> visit_next_list();
> visit_type_uint64();

Another excellent question.

QAPI can only express homogeneous lists, i.e. lists of some type T.

The generated visit_type_TList call the same visit_type_T() for all list
members.

QAPI type 'any' is the top type, but visit_type_anyList() still calls
visit_type_any() for all list members.

Virtual walks could of course do anything.  Since they don't exist
outside tests, we can outlaw doing things that cause us trouble.

The virtual walks we currently have in tests/ seem to walk only
homogeneous lists, i.e. always call the same visit_type_T().

> IOW, can I assume that after every visit_next_list(), visit_type_X is
> called, and that X remains the same for one pass over the list?

As far as I can tell, existing code is just fine with that assumption.
We'd have to write it into the contract, though.

> Also, I assume it is supposed to work like this
>
> visit_start_list();
> visit_next_list();  more input, returns "there's more"
> visit_type_int();   returns 1 (parses 1-3, buffers 1-3)
> visit_type_int();   returns 1
> visit_type_int();   returns 1
> visit_next_list();  more input, unbuffers 1
> visit_type_int();   returns 2
>
> So unbuffering actually happens on visit_next_list()?

I believe this violates the contract.

If it's a real walk, the contract wants you to call visit_next_list()
between subsequent visit_type_int().

If it's a virtual walk, calling visit_next_list() makes no sense.

More questions?

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

>> Would it be valid to do something like this (skipping elements without a
>> proper visit_type_int)
>>
>> visit_start_list();
>> visit_next_list();  more input, returns "there's more"
>> visit_next_list();  parses "1-3,", buffers 2-3, skips over 1
>> visit_type_int();   returns 2
>> ...
> 
> Excellent question!
> 
> Here's the relevant part of visit_start_list()'s contract in visitor.h:
> 
>  * After visit_start_list() succeeds, the caller may visit its members
>  * one after the other.  A real visit (where @obj is non-NULL) uses
>  * visit_next_list() for traversing the linked list, while a virtual
>  * visit (where @obj is NULL) uses other means.  For each list
>  * element, call the appropriate visit_type_FOO() with name set to
>  * NULL and obj set to the address of the value member of the list
>  * element.  Finally, visit_end_list() needs to be called with the
>  * same @list to clean up, even if intermediate visits fail.  See the
>  * examples above.
> 
> So, you *may* visit members, and you *must* call visit_end_list().
> 
> But what are "real" and "virtual" visits?  Again, the contract:
> 
>  * @list must be non-NULL for a real walk, in which case @size
>  * determines how much memory an input or clone visitor will allocate
>  * into *@list (at least sizeof(GenericList)).  Some visitors also
>  * allow @list to be NULL for a virtual walk, in which case @size is
>  * ignored.
> 
> I'm not sure whether I just decreased or increased global confusion ;)

I was reading over these comments and got slightly confused :)

> 
> The file comment explains:
> 
>  * The QAPI schema defines both a set of C data types, and a QMP wire
>  * format.  QAPI objects can contain references to other QAPI objects,
>  * resulting in a directed acyclic graph.  QAPI also generates visitor
>  * functions to walk these graphs.  This file represents the interface
>  * for doing work at each node of a QAPI graph; it can also be used
>  * for a virtual walk, where there is no actual QAPI C struct.
> 
> A real walk with an output visitor works like this (error handling
> omitted for now):
> 
>     Error *err = NULL;
>     intList *tail;
>     size_t size = sizeof(**obj);
> 
>     // fetch list's head into *obj, start the list in output
>     visit_start_list(v, name, (GenericList **)obj, size, &err);
> 
>     // iterate over list's tails
>     // below the hood, visit_next_list() iterates over the GenericList
>     for (tail = *obj; tail;
>          tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
>         // visit current tail's first member, add it to output
>         visit_type_int(v, NULL, &tail->value, &err);
>     }
> 
>     // end the list in output
>     visit_end_list(v, (void **)obj);
> 
> The exact same code works for a real walk with an input visitor, where
> visit_next_list() iterates over the *input* and builds up the
> GenericList.  Compare qobject_input_next_list() and
> qobject_output_next_list().
> 
> The code above is a straight copy of generated visit_type_intList() with
> error handling cut and comments added.
> 
> A real walk works on a QAPI-generated C type.  QAPI generates a real
> walk for each such type.  Open-coding a real walk would senselessly
> duplicate the generated one, so we don't.  Thus, a real walk always
> visits each member.
> 
> Okay, I lied: it visits each member until it runs into an error and
> bails out.  See generated visit_type_intList() in
> qapi/qapi-builtin-visit.c.
> 
> A virtual walk doesn't work with a GenericList *.  Calling
> visit_next_list() makes no sense then.  visitor.h gives this example of
> a virtual walk:

Alright, so we must not support virtual walks. But supporting it would
not harm :)

> 
>  * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
>  * like:
>  *
>  * <example>
>  *  Visitor *v;
>  *  Error *err = NULL;
>  *  int value;
>  *
>  *  v = FOO_visitor_new(...);
>  *  visit_start_struct(v, NULL, NULL, 0, &err);
>  *  if (err) {
>  *      goto out;
>  *  }
>  *  visit_start_list(v, "list", NULL, 0, &err);
>  *  if (err) {
>  *      goto outobj;
>  *  }
>  *  value = 1;
>  *  visit_type_int(v, NULL, &value, &err);
>  *  if (err) {
>  *      goto outlist;
>  *  }
>  *  value = 2;
>  *  visit_type_int(v, NULL, &value, &err);
>  *  if (err) {
>  *      goto outlist;
>  *  }
>  * outlist:
>  *  visit_end_list(v, NULL);
>  *  if (!err) {
>  *      visit_check_struct(v, &err);
>  *  }
>  * outobj:
>  *  visit_end_struct(v, NULL);
>  * out:
>  *  error_propagate(errp, err);
>  *  visit_free(v);
> 
> Why could this be useful?
> 
> 1. With the QObject input visitor, it's an alternative way to
>    destructure a QObject into a bunch of C variables.  The "obvious" way
>    would be calling the QObject accessors.  By using the visitors you
>    get much the error checking code for free.  YMMV.
> 
> 2. With the QObject output visitor, it's an alternative way to build up
>    a QObject.  The "obvious" way would be calling the constructors
>    directly.
> 
> 3. With the string input / output visitors, it's a way to parse / format
>    string visitor syntax without having to construct the C type first.
> 
> Right now, we have no virtual list walks outside tests.  We do have
> virtual struct walks outside tests.
> 
>> Or mixing types
>>
>> visit_start_list();
>> visit_next_list();
>> visit_type_int64();
>> visit_next_list();
>> visit_type_uint64();
> 
> Another excellent question.
> 
> QAPI can only express homogeneous lists, i.e. lists of some type T.
> 
> The generated visit_type_TList call the same visit_type_T() for all list
> members.

Okay, my point would be: Somebody coding its own visit code should not
assume this to work.

> 
> QAPI type 'any' is the top type, but visit_type_anyList() still calls
> visit_type_any() for all list members.
> 
> Virtual walks could of course do anything.  Since they don't exist
> outside tests, we can outlaw doing things that cause us trouble.
> 
> The virtual walks we currently have in tests/ seem to walk only
> homogeneous lists, i.e. always call the same visit_type_T().

Okay, so bailing out if types are switched (e.g. just about to report a
range of uin64_t and somebody asks for a int64_t) is valid.

> 
>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>> called, and that X remains the same for one pass over the list?
> 
> As far as I can tell, existing code is just fine with that assumption.
> We'd have to write it into the contract, though.
> 
>> Also, I assume it is supposed to work like this
>>
>> visit_start_list();
>> visit_next_list();  more input, returns "there's more"
>> visit_type_int();   returns 1 (parses 1-3, buffers 1-3)
>> visit_type_int();   returns 1
>> visit_type_int();   returns 1
>> visit_next_list();  more input, unbuffers 1
>> visit_type_int();   returns 2
>>
>> So unbuffering actually happens on visit_next_list()?
> 
> I believe this violates the contract.
> 
> If it's a real walk, the contract wants you to call visit_next_list()
> between subsequent visit_type_int().
> 
> If it's a virtual walk, calling visit_next_list() makes no sense.
> 
> More questions?
> 

Thanks for the excessive answer! I think that should be enough to come
up with a RFC of a *rewrite* of the string input visitor :)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

>>> Would it be valid to do something like this (skipping elements without a
>>> proper visit_type_int)
>>>
>>> visit_start_list();
>>> visit_next_list();  more input, returns "there's more"
>>> visit_next_list();  parses "1-3,", buffers 2-3, skips over 1
>>> visit_type_int();   returns 2
>>> ...
>> 
>> Excellent question!
>> 
>> Here's the relevant part of visit_start_list()'s contract in visitor.h:
>> 
>>  * After visit_start_list() succeeds, the caller may visit its members
>>  * one after the other.  A real visit (where @obj is non-NULL) uses
>>  * visit_next_list() for traversing the linked list, while a virtual
>>  * visit (where @obj is NULL) uses other means.  For each list
>>  * element, call the appropriate visit_type_FOO() with name set to
>>  * NULL and obj set to the address of the value member of the list
>>  * element.  Finally, visit_end_list() needs to be called with the
>>  * same @list to clean up, even if intermediate visits fail.  See the
>>  * examples above.
>> 
>> So, you *may* visit members, and you *must* call visit_end_list().
>> 
>> But what are "real" and "virtual" visits?  Again, the contract:
>> 
>>  * @list must be non-NULL for a real walk, in which case @size
>>  * determines how much memory an input or clone visitor will allocate
>>  * into *@list (at least sizeof(GenericList)).  Some visitors also
>>  * allow @list to be NULL for a virtual walk, in which case @size is
>>  * ignored.
>> 
>> I'm not sure whether I just decreased or increased global confusion ;)
>
> I was reading over these comments and got slightly confused :)
>
>> 
>> The file comment explains:
>> 
>>  * The QAPI schema defines both a set of C data types, and a QMP wire
>>  * format.  QAPI objects can contain references to other QAPI objects,
>>  * resulting in a directed acyclic graph.  QAPI also generates visitor
>>  * functions to walk these graphs.  This file represents the interface
>>  * for doing work at each node of a QAPI graph; it can also be used
>>  * for a virtual walk, where there is no actual QAPI C struct.
>> 
>> A real walk with an output visitor works like this (error handling
>> omitted for now):
>> 
>>     Error *err = NULL;
>>     intList *tail;
>>     size_t size = sizeof(**obj);
>> 
>>     // fetch list's head into *obj, start the list in output
>>     visit_start_list(v, name, (GenericList **)obj, size, &err);
>> 
>>     // iterate over list's tails
>>     // below the hood, visit_next_list() iterates over the GenericList
>>     for (tail = *obj; tail;
>>          tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
>>         // visit current tail's first member, add it to output
>>         visit_type_int(v, NULL, &tail->value, &err);
>>     }
>> 
>>     // end the list in output
>>     visit_end_list(v, (void **)obj);
>> 
>> The exact same code works for a real walk with an input visitor, where
>> visit_next_list() iterates over the *input* and builds up the
>> GenericList.  Compare qobject_input_next_list() and
>> qobject_output_next_list().
>> 
>> The code above is a straight copy of generated visit_type_intList() with
>> error handling cut and comments added.
>> 
>> A real walk works on a QAPI-generated C type.  QAPI generates a real
>> walk for each such type.  Open-coding a real walk would senselessly
>> duplicate the generated one, so we don't.  Thus, a real walk always
>> visits each member.
>> 
>> Okay, I lied: it visits each member until it runs into an error and
>> bails out.  See generated visit_type_intList() in
>> qapi/qapi-builtin-visit.c.
>> 
>> A virtual walk doesn't work with a GenericList *.  Calling
>> visit_next_list() makes no sense then.  visitor.h gives this example of
>> a virtual walk:
>
> Alright, so we must not support virtual walks. But supporting it would
> not harm :)

Hmm, let me check something... aha!  Both string-input-visitor.h and
string-output-visitor.h have this comment:

 * The string input visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes.  It also
 * requires a non-null list argument to visit_start_list().

The last sentence means virtual walks are not supported.  We owe thanks
to Eric Blake for his commit d9f62dde130 :)

>> 
>>  * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
>>  * like:
>>  *
>>  * <example>
>>  *  Visitor *v;
>>  *  Error *err = NULL;
>>  *  int value;
>>  *
>>  *  v = FOO_visitor_new(...);
>>  *  visit_start_struct(v, NULL, NULL, 0, &err);
>>  *  if (err) {
>>  *      goto out;
>>  *  }
>>  *  visit_start_list(v, "list", NULL, 0, &err);
>>  *  if (err) {
>>  *      goto outobj;
>>  *  }
>>  *  value = 1;
>>  *  visit_type_int(v, NULL, &value, &err);
>>  *  if (err) {
>>  *      goto outlist;
>>  *  }
>>  *  value = 2;
>>  *  visit_type_int(v, NULL, &value, &err);
>>  *  if (err) {
>>  *      goto outlist;
>>  *  }
>>  * outlist:
>>  *  visit_end_list(v, NULL);
>>  *  if (!err) {
>>  *      visit_check_struct(v, &err);
>>  *  }
>>  * outobj:
>>  *  visit_end_struct(v, NULL);
>>  * out:
>>  *  error_propagate(errp, err);
>>  *  visit_free(v);
>> 
>> Why could this be useful?
>> 
>> 1. With the QObject input visitor, it's an alternative way to
>>    destructure a QObject into a bunch of C variables.  The "obvious" way
>>    would be calling the QObject accessors.  By using the visitors you
>>    get much the error checking code for free.  YMMV.
>> 
>> 2. With the QObject output visitor, it's an alternative way to build up
>>    a QObject.  The "obvious" way would be calling the constructors
>>    directly.
>> 
>> 3. With the string input / output visitors, it's a way to parse / format
>>    string visitor syntax without having to construct the C type first.
>> 
>> Right now, we have no virtual list walks outside tests.  We do have
>> virtual struct walks outside tests.
>> 
>>> Or mixing types
>>>
>>> visit_start_list();
>>> visit_next_list();
>>> visit_type_int64();
>>> visit_next_list();
>>> visit_type_uint64();
>> 
>> Another excellent question.
>> 
>> QAPI can only express homogeneous lists, i.e. lists of some type T.
>> 
>> The generated visit_type_TList call the same visit_type_T() for all list
>> members.
>
> Okay, my point would be: Somebody coding its own visit code should not
> assume this to work.
>
>> 
>> QAPI type 'any' is the top type, but visit_type_anyList() still calls
>> visit_type_any() for all list members.
>> 
>> Virtual walks could of course do anything.  Since they don't exist
>> outside tests, we can outlaw doing things that cause us trouble.
>> 
>> The virtual walks we currently have in tests/ seem to walk only
>> homogeneous lists, i.e. always call the same visit_type_T().
>
> Okay, so bailing out if types are switched (e.g. just about to report a
> range of uin64_t and somebody asks for a int64_t) is valid.

I think that would be okay.

>> 
>>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>>> called, and that X remains the same for one pass over the list?
>> 
>> As far as I can tell, existing code is just fine with that assumption.
>> We'd have to write it into the contract, though.
>> 
>>> Also, I assume it is supposed to work like this
>>>
>>> visit_start_list();
>>> visit_next_list();  more input, returns "there's more"
>>> visit_type_int();   returns 1 (parses 1-3, buffers 1-3)
>>> visit_type_int();   returns 1
>>> visit_type_int();   returns 1
>>> visit_next_list();  more input, unbuffers 1
>>> visit_type_int();   returns 2
>>>
>>> So unbuffering actually happens on visit_next_list()?
>> 
>> I believe this violates the contract.
>> 
>> If it's a real walk, the contract wants you to call visit_next_list()
>> between subsequent visit_type_int().
>> 
>> If it's a virtual walk, calling visit_next_list() makes no sense.
>> 
>> More questions?
>> 
>
> Thanks for the excessive answer! I think that should be enough to come
> up with a RFC of a *rewrite* of the string input visitor :)

You're welcome!  I love great questions, they make me *think*.

Besides, if something's worth doing, it's probably worth overdoing ;)

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 08.11.18 10:13, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>>>> Would it be valid to do something like this (skipping elements without a
>>>> proper visit_type_int)
>>>>
>>>> visit_start_list();
>>>> visit_next_list();  more input, returns "there's more"
>>>> visit_next_list();  parses "1-3,", buffers 2-3, skips over 1
>>>> visit_type_int();   returns 2
>>>> ...
>>>
>>> Excellent question!
>>>
>>> Here's the relevant part of visit_start_list()'s contract in visitor.h:
>>>
>>>  * After visit_start_list() succeeds, the caller may visit its members
>>>  * one after the other.  A real visit (where @obj is non-NULL) uses
>>>  * visit_next_list() for traversing the linked list, while a virtual
>>>  * visit (where @obj is NULL) uses other means.  For each list
>>>  * element, call the appropriate visit_type_FOO() with name set to
>>>  * NULL and obj set to the address of the value member of the list
>>>  * element.  Finally, visit_end_list() needs to be called with the
>>>  * same @list to clean up, even if intermediate visits fail.  See the
>>>  * examples above.
>>>
>>> So, you *may* visit members, and you *must* call visit_end_list().
>>>
>>> But what are "real" and "virtual" visits?  Again, the contract:
>>>
>>>  * @list must be non-NULL for a real walk, in which case @size
>>>  * determines how much memory an input or clone visitor will allocate
>>>  * into *@list (at least sizeof(GenericList)).  Some visitors also
>>>  * allow @list to be NULL for a virtual walk, in which case @size is
>>>  * ignored.
>>>
>>> I'm not sure whether I just decreased or increased global confusion ;)
>>
>> I was reading over these comments and got slightly confused :)
>>
>>>
>>> The file comment explains:
>>>
>>>  * The QAPI schema defines both a set of C data types, and a QMP wire
>>>  * format.  QAPI objects can contain references to other QAPI objects,
>>>  * resulting in a directed acyclic graph.  QAPI also generates visitor
>>>  * functions to walk these graphs.  This file represents the interface
>>>  * for doing work at each node of a QAPI graph; it can also be used
>>>  * for a virtual walk, where there is no actual QAPI C struct.
>>>
>>> A real walk with an output visitor works like this (error handling
>>> omitted for now):
>>>
>>>     Error *err = NULL;
>>>     intList *tail;
>>>     size_t size = sizeof(**obj);
>>>
>>>     // fetch list's head into *obj, start the list in output
>>>     visit_start_list(v, name, (GenericList **)obj, size, &err);
>>>
>>>     // iterate over list's tails
>>>     // below the hood, visit_next_list() iterates over the GenericList
>>>     for (tail = *obj; tail;
>>>          tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
>>>         // visit current tail's first member, add it to output
>>>         visit_type_int(v, NULL, &tail->value, &err);
>>>     }
>>>
>>>     // end the list in output
>>>     visit_end_list(v, (void **)obj);
>>>
>>> The exact same code works for a real walk with an input visitor, where
>>> visit_next_list() iterates over the *input* and builds up the
>>> GenericList.  Compare qobject_input_next_list() and
>>> qobject_output_next_list().
>>>
>>> The code above is a straight copy of generated visit_type_intList() with
>>> error handling cut and comments added.
>>>
>>> A real walk works on a QAPI-generated C type.  QAPI generates a real
>>> walk for each such type.  Open-coding a real walk would senselessly
>>> duplicate the generated one, so we don't.  Thus, a real walk always
>>> visits each member.
>>>
>>> Okay, I lied: it visits each member until it runs into an error and
>>> bails out.  See generated visit_type_intList() in
>>> qapi/qapi-builtin-visit.c.
>>>
>>> A virtual walk doesn't work with a GenericList *.  Calling
>>> visit_next_list() makes no sense then.  visitor.h gives this example of
>>> a virtual walk:
>>
>> Alright, so we must not support virtual walks. But supporting it would
>> not harm :)
> 
> Hmm, let me check something... aha!  Both string-input-visitor.h and
> string-output-visitor.h have this comment:
> 
>  * The string input visitor does not implement support for visiting
>  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>  * requires a non-null list argument to visit_start_list().
> 
> The last sentence means virtual walks are not supported.  We owe thanks
> to Eric Blake for his commit d9f62dde130 :)
> 
>>>
>>>  * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
>>>  * like:
>>>  *
>>>  * <example>
>>>  *  Visitor *v;
>>>  *  Error *err = NULL;
>>>  *  int value;
>>>  *
>>>  *  v = FOO_visitor_new(...);
>>>  *  visit_start_struct(v, NULL, NULL, 0, &err);
>>>  *  if (err) {
>>>  *      goto out;
>>>  *  }
>>>  *  visit_start_list(v, "list", NULL, 0, &err);
>>>  *  if (err) {
>>>  *      goto outobj;
>>>  *  }
>>>  *  value = 1;
>>>  *  visit_type_int(v, NULL, &value, &err);
>>>  *  if (err) {
>>>  *      goto outlist;
>>>  *  }
>>>  *  value = 2;
>>>  *  visit_type_int(v, NULL, &value, &err);
>>>  *  if (err) {
>>>  *      goto outlist;
>>>  *  }
>>>  * outlist:
>>>  *  visit_end_list(v, NULL);
>>>  *  if (!err) {
>>>  *      visit_check_struct(v, &err);
>>>  *  }
>>>  * outobj:
>>>  *  visit_end_struct(v, NULL);
>>>  * out:
>>>  *  error_propagate(errp, err);
>>>  *  visit_free(v);
>>>
>>> Why could this be useful?
>>>
>>> 1. With the QObject input visitor, it's an alternative way to
>>>    destructure a QObject into a bunch of C variables.  The "obvious" way
>>>    would be calling the QObject accessors.  By using the visitors you
>>>    get much the error checking code for free.  YMMV.
>>>
>>> 2. With the QObject output visitor, it's an alternative way to build up
>>>    a QObject.  The "obvious" way would be calling the constructors
>>>    directly.
>>>
>>> 3. With the string input / output visitors, it's a way to parse / format
>>>    string visitor syntax without having to construct the C type first.
>>>
>>> Right now, we have no virtual list walks outside tests.  We do have
>>> virtual struct walks outside tests.
>>>
>>>> Or mixing types
>>>>
>>>> visit_start_list();
>>>> visit_next_list();
>>>> visit_type_int64();
>>>> visit_next_list();
>>>> visit_type_uint64();
>>>
>>> Another excellent question.
>>>
>>> QAPI can only express homogeneous lists, i.e. lists of some type T.
>>>
>>> The generated visit_type_TList call the same visit_type_T() for all list
>>> members.
>>
>> Okay, my point would be: Somebody coding its own visit code should not
>> assume this to work.
>>
>>>
>>> QAPI type 'any' is the top type, but visit_type_anyList() still calls
>>> visit_type_any() for all list members.
>>>
>>> Virtual walks could of course do anything.  Since they don't exist
>>> outside tests, we can outlaw doing things that cause us trouble.
>>>
>>> The virtual walks we currently have in tests/ seem to walk only
>>> homogeneous lists, i.e. always call the same visit_type_T().
>>
>> Okay, so bailing out if types are switched (e.g. just about to report a
>> range of uin64_t and somebody asks for a int64_t) is valid.
> 
> I think that would be okay.
> 
>>>
>>>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>>>> called, and that X remains the same for one pass over the list?
>>>
>>> As far as I can tell, existing code is just fine with that assumption.
>>> We'd have to write it into the contract, though.
>>>
>>>> Also, I assume it is supposed to work like this
>>>>
>>>> visit_start_list();
>>>> visit_next_list();  more input, returns "there's more"
>>>> visit_type_int();   returns 1 (parses 1-3, buffers 1-3)
>>>> visit_type_int();   returns 1
>>>> visit_type_int();   returns 1
>>>> visit_next_list();  more input, unbuffers 1
>>>> visit_type_int();   returns 2
>>>>
>>>> So unbuffering actually happens on visit_next_list()?
>>>
>>> I believe this violates the contract.
>>>
>>> If it's a real walk, the contract wants you to call visit_next_list()
>>> between subsequent visit_type_int().
>>>
>>> If it's a virtual walk, calling visit_next_list() makes no sense.
>>>
>>> More questions?
>>>
>>
>> Thanks for the excessive answer! I think that should be enough to come
>> up with a RFC of a *rewrite* of the string input visitor :)
> 
> You're welcome!  I love great questions, they make me *think*.
> 
> Besides, if something's worth doing, it's probably worth overdoing ;)
> 

I found some more ugliness, looking at the tests. I am not sure the test
is correct here.

test_visitor_in_intList():

v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");

-> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
visitor actually does sorting + duplicate elimination. Now I consider
this is wrong. We are parsing a list of integers, not an ordered set.

What's your take on this?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> I found some more ugliness, looking at the tests. I am not sure the test
> is correct here.
>
> test_visitor_in_intList():
>
> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>
> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
> visitor actually does sorting + duplicate elimination. Now I consider
> this is wrong. We are parsing a list of integers, not an ordered set.
>
> What's your take on this?

I think you're right.  Visitors are tied to QAPI, and QAPI does *lists*,
not sets.  Lists are more general than sets.

I figure what drove development of this code was a need for sets, so
sets got implemented.  Review fail.

If the visitor does lists, whatever needs sets can convert the lists to
sets.

I'd advise against evolving the current code towards sanity.  Instead,
rewrite, and rely on tests to avoid unwanted changes in behavior.

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[Eric Blake]

On 11/8/18 7:05 AM, David Hildenbrand wrote:

>>> Thanks for the excessive answer! I think that should be enough to come
>>> up with a RFC of a *rewrite* of the string input visitor :)
>>
>> You're welcome!  I love great questions, they make me *think*.
>>
>> Besides, if something's worth doing, it's probably worth overdoing ;)
>>
> 
> I found some more ugliness, looking at the tests. I am not sure the test
> is correct here.
> 
> test_visitor_in_intList():
> 
> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
> 
> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
> visitor actually does sorting + duplicate elimination. Now I consider
> this is wrong. We are parsing a list of integers, not an ordered set.
> 
> What's your take on this?

The test matches the current reality (yes, the string input visitor 
currently sorts and deduplicates) - but you are also right that in 
general JSON [] lists are supposed to preserve order. I suspect our use 
of -numa parsing relies on the result being sorted - but I would not be 
opposed to a rewrite that dumbs down the string visitor to provide an 
unsorted list, and move the sorting into a post-pass belonging to the 
-numa code.  Updating the tests to match as part of a rewrite is thus 
okay, if we have the justification that we audited all other users to 
see that they either don't care or that we can move the sorting out of 
the list walker and into the callers that do care.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 08.11.18 15:36, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
> 
> I think you're right.  Visitors are tied to QAPI, and QAPI does *lists*,
> not sets.  Lists are more general than sets.
> 
> I figure what drove development of this code was a need for sets, so
> sets got implemented.  Review fail.
> 
> If the visitor does lists, whatever needs sets can convert the lists to
> sets.
> 
> I'd advise against evolving the current code towards sanity.  Instead,
> rewrite, and rely on tests to avoid unwanted changes in behavior.
> 

With the current rewrite I have, I can parse uint64 and int64 lists. The
other types will bail out if lists are used right now.

The changes that have to be done to the tests are:

diff --git a/tests/test-string-input-visitor.c
b/tests/test-string-input-visitor.c
index 88e0e1aa9a..5d2d707e80 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t
*expected, size_t n)
     uint64List *tail;
     int i;

-    /* BUG: unsigned numbers above INT64_MAX don't work */
-    for (i = 0; i < n; i++) {
-        if (expected[i] > INT64_MAX) {
-            Error *err = NULL;
-            visit_type_uint64List(v, NULL, &res, &err);
-            error_free_or_abort(&err);
-            return;
-        }
-    }
-
     visit_type_uint64List(v, NULL, &res, &error_abort);
     tail = res;
     for (i = 0; i < n; i++) {
@@ -118,9 +108,9 @@ static void
test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    /* Note: the visitor *sorts* ranges *unsigned* */
-    int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
+    int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3,
4, 5, 6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
-    int64_t expect3[] = { INT64_MAX, INT64_MIN };
+    int64_t expect3[] = { INT64_MIN, INT64_MAX };
     uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
@@ -189,7 +179,7 @@ static void
test_visitor_in_intList(TestInputVisitorData *data,
     visit_type_int64(v, NULL, &tail->value, &err);
     g_assert_cmpint(tail->value, ==, 0);
     visit_type_int64(v, NULL, &val, &err);
-    g_assert_cmpint(val, ==, 1); /* BUG */
+    error_free_or_abort(&err);
     visit_check_list(v, &error_abort);
     visit_end_list(v, (void **)&res);


Basically fixing two bugs (yey, let's make our tests pass by hardcoding
BUG behavior, so the actually fixed code will break them)

And converting two assumptions about ordered sets into unordered lists.

(using unsigned ranges for handling signed ranges is completely broken,
as can also be seen via the removed "Note:")

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 08.11.18 15:42, Eric Blake wrote:
> On 11/8/18 7:05 AM, David Hildenbrand wrote:
> 
>>>> Thanks for the excessive answer! I think that should be enough to come
>>>> up with a RFC of a *rewrite* of the string input visitor :)
>>>
>>> You're welcome!  I love great questions, they make me *think*.
>>>
>>> Besides, if something's worth doing, it's probably worth overdoing ;)
>>>
>>
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
> 
> The test matches the current reality (yes, the string input visitor 
> currently sorts and deduplicates) - but you are also right that in 
> general JSON [] lists are supposed to preserve order. I suspect our use 
> of -numa parsing relies on the result being sorted - but I would not be 
> opposed to a rewrite that dumbs down the string visitor to provide an 
> unsorted list, and move the sorting into a post-pass belonging to the 
> -numa code.  Updating the tests to match as part of a rewrite is thus 
> okay, if we have the justification that we audited all other users to 
> see that they either don't care or that we can move the sorting out of 
> the list walker and into the callers that do care.
> 

-numa is parsed via the object parser and not the string input parser as
far as I understood Markus. I think only "host-nodes" for memdev would
be relevant right now, and as far as I can see, that shouldn't break
(have to take a closer look).

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 23.10.18 17:23, David Hildenbrand wrote:
> 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)
> 

Ping, anybody has some time to review this?

-- 

Thanks,

David / dhildenb

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

[Igor Mammedov]

On Tue, 23 Oct 2018 17:23:06 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.
s/error/error out/

> 
> "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);
maybe replace "range overflow" with "too big" or something else more user friendly 

> +            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);
                                         ^^^^^^^^^^^^^^^^^^^
this theoretically could overflow and already past 'as' check so it would
return an invalid address without erroring out.

But in practice we don't have memory device container ending right on 64bit
limit, so it's not really an issue.


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

beside minor notes patch looks good

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

[David Hildenbrand]

On 13.11.18 13:26, Igor Mammedov wrote:
> On Tue, 23 Oct 2018 17:23:06 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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.
> s/error/error out/

Thanks, fixed.

> 
>>
>> "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);
> maybe replace "range overflow" with "too big" or something else more user friendly 

I guess I'll use the same error message for these two cases

"can't add memory device [...], usable range for memory devices [...]"

That will include the "range overflow" scenario when a hint was given.

> 
>> +            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);
>                                          ^^^^^^^^^^^^^^^^^^^
> this theoretically could overflow and already past 'as' check so it would
> return an invalid address without erroring out.
> 
> But in practice we don't have memory device container ending right on 64bit
> limit, so it's not really an issue.

I'll add a simple check for "!next_addr".

> 
> 
>> +            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)
> 
> beside minor notes patch looks good
> 

Thanks!

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v4] 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 errors out instead of succeeding. (Note that qapi
parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for
memory devices [0x140000000:0xe00000000]"

Signed-off-by: David Hildenbrand <david@redhat.com>
---

v3 -> v4:
- Use better error messages
- Fix one theretical overflow

 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 8be63c8032..28e871f562 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,25 @@ 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);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));
             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, device too big");
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */
@@ -166,30 +168,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 (!next_addr || 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.2