[Qemu-devel] [PATCH v14 11/19] qapi: add integer range support for QObjectInputVisitor

["Daniel P. Berrange" <berrange@redhat.com>]

The traditional CLI arg syntax allows two ways to specify
integer lists, either one value per key, or a range of
values per key. eg the following are identical:

  -arg foo=5,foo=6,foo=7
  -arg foo=5-7

This extends the QObjectInputVisitor so that it is able
to parse ranges and turn them into distinct list entries.

This means that

  -arg foo=5-7

is treated as equivalent to

  -arg foo.0=5,foo.1=6,foo.2=7

Edge case tests are copied from test-opts-visitor to
ensure identical behaviour when parsing.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qapi/qobject-input-visitor.h |  22 +++-
 qapi/qobject-input-visitor.c         | 154 +++++++++++++++++++++++++--
 tests/test-qobject-input-visitor.c   | 195 +++++++++++++++++++++++++++++++++--
 3 files changed, 356 insertions(+), 15 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 90989f4..63f3782 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -19,6 +19,12 @@
 
 typedef struct QObjectInputVisitor QObjectInputVisitor;
 
+/* Inclusive upper bound on the size of any flattened range. This is a safety
+ * (= anti-annoyance) measure; wrong ranges should not cause long startup
+ * delays nor exhaust virtual memory.
+ */
+#define QIV_RANGE_MAX 65536
+
 /**
  * Create a new input visitor that converts @obj to a QAPI object.
  *
@@ -71,6 +77,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
  * The value given determines how many levels of structs are allowed to
  * be flattened in this way.
  *
+ * If @permit_int_ranges is true, then when visiting a list of integers,
+ * the integer value strings may encode ranges eg a single element
+ * containing "5-7" is treated as if there were three elements "5", "6",
+ * "7". This should only be used if compatibility is required with the
+ * OptsVisitor which would allow integer ranges. e.g. set this to true
+ * if you have compatibility requirements for
+ *
+ *   -arg val=5-8
+ *
+ * to be treated as equivalent to the preferred syntax:
+ *
+ *   -arg val.0=5,val.1=6,val.2=7,val.3=8
+ *
  * The visitor always operates in strict mode, requiring all dict keys
  * to be consumed during visitation. An error will be reported if this
  * does not happen.
@@ -80,6 +99,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
  */
 Visitor *qobject_input_visitor_new_autocast(QObject *obj,
                                             bool autocreate_list,
-                                            size_t autocreate_struct_levels);
+                                            size_t autocreate_struct_levels,
+                                            bool permit_int_ranges);
 
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 6cacd4b..a38e779 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -31,6 +31,8 @@ typedef struct StackObject
 
     GHashTable *h;           /* If obj is dict: unvisited keys */
     const QListEntry *entry; /* If obj is list: unvisited tail */
+    uint64_t range_val;
+    uint64_t range_limit;
 
     QSLIST_ENTRY(StackObject) node;
 } StackObject;
@@ -60,6 +62,10 @@ struct QObjectInputVisitor
      * consider auto-creating a struct containing
      * remaining unvisited items */
     size_t autocreate_struct_levels;
+
+    /* Whether int lists can have single values representing
+     * ranges of values */
+    bool permit_int_ranges;
 };
 
 static QObjectInputVisitor *to_qiv(Visitor *v)
@@ -282,7 +288,7 @@ static GenericList *qobject_input_next_list(Visitor *v, GenericList *tail,
     QObjectInputVisitor *qiv = to_qiv(v);
     StackObject *so = QSLIST_FIRST(&qiv->stack);
 
-    if (!so->entry) {
+    if ((so->range_val == so->range_limit) && !so->entry) {
         return NULL;
     }
     tail->next = g_malloc0(size);
@@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
                                               int64_t *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
-                                                                true));
+    QString *qstr;
     int64_t ret;
+    const char *end = NULL;
+    StackObject *tos;
+    bool inlist = false;
+
+    /* Preferentially generate values from a range, before
+     * trying to consume another QList element */
+    tos = QSLIST_FIRST(&qiv->stack);
+    if (tos) {
+        if ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
+            *obj = tos->range_val + 1;
+            tos->range_val++;
+            return;
+        } else {
+            inlist = tos->entry != NULL;
+        }
+    }
 
+    qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
+                                                       true));
     if (!qstr || !qstr->string) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "string");
         return;
     }
 
-    if (qemu_strtoll(qstr->string, NULL, 0, &ret) < 0) {
+    if (qemu_strtoll(qstr->string, &end, 0, &ret) < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
         return;
     }
     *obj = ret;
+
+    /*
+     * If we have string that represents an integer range (5-24),
+     * parse the end of the range and set things up so we'll process
+     * the rest of the range before consuming another element
+     * from the QList.
+     */
+    if (end && *end) {
+        if (!qiv->permit_int_ranges) {
+            error_setg(errp,
+                       "Integer ranges are not permitted here");
+            return;
+        }
+        if (!inlist) {
+            error_setg(errp,
+                       "Integer ranges are only permitted when "
+                       "visiting list parameters");
+            return;
+        }
+        if (*end != '-') {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+                       "a number range");
+            return;
+        }
+        end++;
+        if (qemu_strtoll(end, NULL, 0, &ret) < 0) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
+            return;
+        }
+        if (*obj > ret) {
+            error_setg(errp,
+                       "Parameter '%s' range start %" PRIu64
+                       " must be less than (or equal to) %" PRIu64,
+                       name, *obj, ret);
+            return;
+        }
+
+        if ((*obj <= (INT64_MAX - QIV_RANGE_MAX)) &&
+            (ret >= (*obj + QIV_RANGE_MAX))) {
+            error_setg(errp,
+                       "Parameter '%s' range must be less than %d",
+                       name, QIV_RANGE_MAX);
+            return;
+        }
+        if (*obj != ret) {
+            tos->range_val = *obj;
+            tos->range_limit = ret;
+        }
+    }
 }
 
 static void qobject_input_type_uint64(Visitor *v, const char *name,
@@ -366,21 +438,85 @@ static void qobject_input_type_uint64_autocast(Visitor *v, const char *name,
                                                uint64_t *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
-                                                                true));
+    QString *qstr;
     unsigned long long ret;
+    char *end = NULL;
+    StackObject *tos;
+    bool inlist = false;
+
+    /* Preferentially generate values from a range, before
+     * trying to consume another QList element */
+    tos = QSLIST_FIRST(&qiv->stack);
+    if (tos) {
+        if (tos->range_val < tos->range_limit) {
+            *obj = tos->range_val + 1;
+            tos->range_val++;
+            return;
+        } else {
+            inlist = tos->entry != NULL;
+        }
+    }
 
+    qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
+                                                       true));
     if (!qstr || !qstr->string) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "string");
         return;
     }
 
-    if (parse_uint_full(qstr->string, &ret, 0) < 0) {
+    if (parse_uint(qstr->string, &ret, &end, 0) < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
         return;
     }
     *obj = ret;
+
+    /*
+     * If we have string that represents an integer range (5-24),
+     * parse the end of the range and set things up so we'll process
+     * the rest of the range before consuming another element
+     * from the QList.
+     */
+    if (end && *end) {
+        if (!qiv->permit_int_ranges) {
+            error_setg(errp,
+                       "Integer ranges are not permitted here");
+            return;
+        }
+        if (!inlist) {
+            error_setg(errp,
+                       "Integer ranges are only permitted when "
+                       "visiting list parameters");
+            return;
+        }
+        if (*end != '-') {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+                       "a number range");
+            return;
+        }
+        end++;
+        if (parse_uint_full(end, &ret, 0) < 0) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
+            return;
+        }
+        if (*obj > ret) {
+            error_setg(errp,
+                       "Parameter '%s' range start %" PRIu64
+                       " must be less than (or equal to) %llu",
+                       name, *obj, ret);
+            return;
+        }
+        if ((ret - *obj) > (QIV_RANGE_MAX - 1)) {
+            error_setg(errp,
+                       "Parameter '%s' range must be less than %d",
+                       name, QIV_RANGE_MAX);
+            return;
+        }
+        if (*obj != ret) {
+            tos->range_val = *obj;
+            tos->range_limit = ret;
+        }
+    }
 }
 
 static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
@@ -576,7 +712,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict)
 
 Visitor *qobject_input_visitor_new_autocast(QObject *obj,
                                             bool autocreate_list,
-                                            size_t autocreate_struct_levels)
+                                            size_t autocreate_struct_levels,
+                                            bool permit_int_ranges)
 {
     QObjectInputVisitor *v;
 
@@ -603,6 +740,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
     v->strict = true;
     v->autocreate_list = autocreate_list;
     v->autocreate_struct_levels = autocreate_struct_levels;
+    v->permit_int_ranges = permit_int_ranges;
 
     v->root = obj;
     qobject_incref(obj);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index ab55f99..783ecd4 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -45,6 +45,7 @@ visitor_input_test_init_internal(TestInputVisitorData *data,
                                  bool strict, bool autocast,
                                  bool autocreate_list,
                                  size_t autocreate_struct_levels,
+                                 bool permit_int_ranges,
                                  const char *json_string,
                                  va_list *ap)
 {
@@ -56,10 +57,12 @@ visitor_input_test_init_internal(TestInputVisitorData *data,
     if (autocast) {
         assert(strict);
         data->qiv = qobject_input_visitor_new_autocast(
-            data->obj, autocreate_list, autocreate_struct_levels);
+            data->obj, autocreate_list, autocreate_struct_levels,
+            permit_int_ranges);
     } else {
         assert(!autocreate_list);
         assert(!autocreate_struct_levels);
+        assert(!permit_int_ranges);
         data->qiv = qobject_input_visitor_new(data->obj, strict);
     }
     g_assert(data->qiv);
@@ -77,7 +80,7 @@ Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
 
     va_start(ap, json_string);
     v = visitor_input_test_init_internal(data, strict, autocast,
-                                         autocreate_list, 0,
+                                         autocreate_list, 0, false,
                                          json_string, &ap);
     va_end(ap);
     return v;
@@ -91,7 +94,7 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
     va_list ap;
 
     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, true, false, false, 0,
+    v = visitor_input_test_init_internal(data, true, false, false, 0, false,
                                          json_string, &ap);
     va_end(ap);
     return v;
@@ -107,7 +110,7 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
 static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
                                             const char *json_string)
 {
-    return visitor_input_test_init_internal(data, true, false, false, 0,
+    return visitor_input_test_init_internal(data, true, false, false, 0, false,
                                             json_string, NULL);
 }
 
@@ -386,7 +389,7 @@ static void test_visitor_in_struct_autocreate(TestInputVisitorData *data,
     char *script = NULL;
 
     v = visitor_input_test_init_internal(
-        data, true, true, false, 3,
+        data, true, true, false, 3, false,
         "{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
         "'script': 'ifup' }", NULL);
 
@@ -435,7 +438,7 @@ static void test_visitor_in_struct_autocreate_extra(TestInputVisitorData *data,
     Error *err = NULL;
 
     v = visitor_input_test_init_internal(
-        data, true, true, false, 3,
+        data, true, true, false, 3, false,
         "{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
         "'script': 'ifup' }", NULL);
 
@@ -560,6 +563,88 @@ static void test_visitor_in_list_autocreate_int(TestInputVisitorData *data,
     head = NULL;
 }
 
+typedef struct {
+    const char *range_str;
+    bool expect_fail;
+    int64_t *values;
+    size_t nvalues;
+} TestIntRangeData;
+
+static void test_visitor_in_list_autocast_int_range(TestInputVisitorData *data,
+                                                    const void *opaque)
+{
+    const TestIntRangeData *idata = opaque;
+    int64List *item, *head = NULL;
+    Visitor *v;
+    size_t i;
+    Error *err = NULL;
+
+    /* Verify that we auto-expand signed int ranges */
+    v = visitor_input_test_init_internal(data, true, true, true, 0, true,
+                                         idata->range_str, NULL);
+
+    visit_type_int64List(v, NULL, &head, &err);
+    if (idata->expect_fail) {
+        error_free_or_abort(&err);
+        g_assert(head == NULL);
+    } else {
+        g_assert(err == NULL);
+        g_assert(head != NULL);
+        for (i = 0, item = head; item != NULL && i < idata->nvalues;
+             item = item->next, i++) {
+            if (idata->values != NULL) {
+                g_assert_cmpint(item->value, ==, idata->values[i]);
+            }
+        }
+        g_assert_cmpint(i, ==, idata->nvalues);
+
+        qapi_free_int64List(head);
+        head = NULL;
+    }
+}
+
+typedef struct {
+    const char *range_str;
+    bool expect_fail;
+    uint64_t *values;
+    size_t nvalues;
+} TestUIntRangeData;
+
+static void test_visitor_in_list_autocast_uint_range(TestInputVisitorData *data,
+                                                     const void *opaque)
+{
+    const TestUIntRangeData *idata = opaque;
+    uint64List *item, *head = NULL;
+    Visitor *v;
+    size_t i;
+    Error *err = NULL;
+
+    /* Verify that we auto-expand signed int ranges */
+    v = visitor_input_test_init_internal(data, true, true, true, 0, true,
+                                         idata->range_str, NULL);
+
+    visit_type_uint64List(v, NULL, &head, &err);
+    if (idata->expect_fail) {
+        g_assert(err != NULL);
+        g_assert(head == NULL);
+    } else {
+        g_assert(err == NULL);
+        g_assert(head != NULL);
+
+        for (i = 0, item = head; item != NULL && i < idata->nvalues;
+             item = item->next, i++) {
+            if (idata->values != NULL) {
+                g_assert_cmpint(item->value, ==, idata->values[i]);
+            }
+        }
+        g_assert_cmpint(i, ==, idata->nvalues);
+
+        qapi_free_uint64List(head);
+        head = NULL;
+    }
+}
+
+
 static void test_visitor_in_any(TestInputVisitorData *data,
                                 const void *unused)
 {
@@ -1253,6 +1338,104 @@ int main(int argc, char **argv)
     input_visitor_test_add("/visitor/input/native_list/number",
                            NULL, test_visitor_in_native_list_number);
 
+#define INT_RANGE_TEST(suffix, str, expect_fail, values, nvalues)       \
+    TestIntRangeData idata ## suffix =                                  \
+        { str, expect_fail, values, nvalues };                          \
+    input_visitor_test_add("/visitor/input/int-list-autocast-" #suffix, \
+                           &idata ## suffix,                            \
+                           test_visitor_in_list_autocast_int_range)
+
+#define UINT_RANGE_TEST(suffix, str, expect_fail, values, nvalues)      \
+    TestUIntRangeData udata ## suffix =                                 \
+        { str, expect_fail, values, nvalues };                          \
+    input_visitor_test_add("/visitor/input/uint-list-autocast-" #suffix,\
+                           &udata ## suffix,                            \
+                           test_visitor_in_list_autocast_uint_range)
+
+
+    int64_t multvals[] = { -1, 0, -11, -10, -9, 5, -16, -15, -14, -13, 14,
+                           15, 7, 2, 3, 9, 10, 11, 12, -7, -6, -5, -4, -3 };
+    INT_RANGE_TEST(multiple,
+                   "['-1-0','-11--9','5-5',"         \
+                   "'-16--13','14-15',"              \
+                   "'7','2-3','9-12','-7--3']",
+                   false, multvals, G_N_ELEMENTS(multvals));
+
+    INT_RANGE_TEST(errno,
+                   "'0x7fffffffffffffff-0x8000000000000000'",
+                   true, NULL, 0);
+    INT_RANGE_TEST(incomplete, "'5-'",
+                   true, NULL, 0);
+    INT_RANGE_TEST(trailing, "'5-6z'",
+                   true, NULL, 0);
+    INT_RANGE_TEST(empty, "\"6-5\"",
+                   true, NULL, 0);
+    int64_t negvals[] = { -10, -9, -8, -7 };
+    INT_RANGE_TEST(neg, "\"-10--7\"",
+                   false, negvals, G_N_ELEMENTS(negvals));
+    INT_RANGE_TEST(minval,
+                   "'-0x8000000000000000--0x8000000000000000'",
+                   false, (int64_t[]){ -0x8000000000000000 }, 1);
+    INT_RANGE_TEST(maxval,
+                   "'0x7fffffffffffffff-0x7fffffffffffffff'",
+                   false, (int64_t[]){ 0x7fffffffffffffff }, 1);
+
+    UINT_RANGE_TEST(errno,
+                    "'0xffffffffffffffff-0x10000000000000000'",
+                    true, NULL, 0);
+    UINT_RANGE_TEST(incomplete, "'5-'",
+                    true, NULL, 0);
+    UINT_RANGE_TEST(trailing, "'5-6z'",
+                    true, NULL, 0);
+    UINT_RANGE_TEST(empty, "'6-5'",
+                    true, NULL, 0);
+    UINT_RANGE_TEST(neg, "\"-10--7\"",
+                    true, NULL, 0);
+    UINT_RANGE_TEST(minval, "'0-0'",
+                    false, (uint64_t[]){ 0 }, 1);
+    UINT_RANGE_TEST(maxval,
+                    "'0xffffffffffffffff-0xffffffffffffffff'",
+                    false, (uint64_t[]){ 0xffffffffffffffff }, 1);
+
+    /* Test maximum range sizes. The macro value is open-coded here
+     * *intentionally*; the test case must use concrete values by design. If
+     * QIV_RANGE_MAX is changed, the following values need to be
+     * recalculated as well. The assert and this comment should help with it.
+     */
+    g_assert(QIV_RANGE_MAX == 65536);
+
+    /* The unsigned case is simple, a u64-u64 difference can always be
+     * represented as a u64.
+     */
+    UINT_RANGE_TEST(max,
+                    "'0-65535'",
+                    false, NULL, QIV_RANGE_MAX);
+    UINT_RANGE_TEST(2big,
+                    "'0-65536'",
+                    true, NULL, 0);
+
+    INT_RANGE_TEST(max_pos_a,
+                   "'0x7fffffffffff0000-0x7fffffffffffffff'",
+                   false, NULL, 100);
+    INT_RANGE_TEST(max_pos_b,
+                   "'0x7ffffffffffeffff-0x7ffffffffffffffe'",
+                   false, NULL, 100);
+    INT_RANGE_TEST(max_neg_a,
+                   "'-0x8000000000000000--0x7fffffffffff0001'",
+                   false, NULL, 100);
+    INT_RANGE_TEST(max_neg_b,
+                   "'-0x7fffffffffffffff--0x7fffffffffff0000'",
+                   false, NULL, 100);
+    INT_RANGE_TEST(2big_pos,
+                   "'0x7ffffffffffeffff-0x7fffffffffffffff'",
+                   true, NULL, 0);
+    INT_RANGE_TEST(2big_neg,
+                   "'-0x8000000000000000--0x7fffffffffff0000'",
+                   true, NULL, 0);
+    INT_RANGE_TEST(2big_neg_pos,
+                   "'-0x8000000000000000-0x7fffffffffffffff'",
+                   true, NULL, 0);
+
     g_test_run();
 
     return 0;
-- 
2.7.4