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 <berra...@redhat.com> --- include/qapi/qobject-input-visitor.h | 23 ++++- qapi/qobject-input-visitor.c | 158 ++++++++++++++++++++++++++-- tests/test-qobject-input-visitor.c | 195 +++++++++++++++++++++++++++++++++-- 3 files changed, 360 insertions(+), 16 deletions(-) diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h index 94051f3..242b767 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,7 +99,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); /** @@ -98,6 +118,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj, Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, bool autocreate_list, size_t autocreate_struct_levels, + bool permit_int_ranges, Error **errp); #endif diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 1be4865..6b3d0f2 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); @@ -614,6 +752,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj, Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, bool autocreate_list, size_t autocreate_struct_levels, + bool permit_int_ranges, Error **errp) { QDict *pdict; @@ -632,7 +771,8 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, v = qobject_input_visitor_new_autocast(pobj, autocreate_list, - autocreate_struct_levels); + autocreate_struct_levels, + permit_int_ranges); cleanup: qobject_decref(pobj); QDECREF(pdict); 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