[Qemu-devel] [PATCH v2 3/3] qapi: support nested structs in OptsVisitor

[Kővágó, Zoltán]

The current OptsVisitor flattens the whole structure, if there are same
named fields under different paths (like `in' and `out' in `Audiodev'),
the current visitor can't cope with them (for example setting
`frequency=44100' will set the in's frequency to 44100 and leave out's
frequency unspecified).

This patch fixes it, by always requiring a complete path in case of
nested structs.  Fields in the path are separated by dots, similar to C
structs (without pointers), like `in.frequency' or `out.frequency'.

You must provide a full path even in non-ambiguous cases.  The qapi
flattening commits for Numa and Netdev and NetLegacy ensures that this
change doesn't create backward compatibility problems.  (Previously
OptsVisitor didn't bother with structs and only consulted field names,
so something like `data.helper' was invalid previously, and stays so,
since unions are flattened now.)

Signed-off-by: Kővágó, Zoltán <address@hidden>
---
 qapi/opts-visitor.c                     | 116 ++++++++++++++++++++++++++------
 tests/qapi-schema/qapi-schema-test.json |   9 ++-
 tests/qapi-schema/qapi-schema-test.out  |   4 ++
 tests/test-opts-visitor.c               |  34 ++++++++++
 4 files changed, 141 insertions(+), 22 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 900b2fa..70dbaf0 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -71,6 +71,7 @@ struct OptsVisitor
      * schema, with a single mandatory scalar member. */
     ListMode list_mode;
     GQueue *repeated_opts;
+    char *repeated_name;
 
     /* When parsing a list of repeating options as integers, values of the form
      * "a-b", representing a closed interval, are allowed. Elements in the
@@ -86,6 +87,9 @@ struct OptsVisitor
      * not survive or escape the OptsVisitor object.
      */
     QemuOpt *fake_id_opt;
+
+    /* List of field names leading to the current structure. */
+    GQueue *nested_names;
 };
 
 
@@ -100,6 +104,7 @@ static void
 opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
 {
     GQueue *list;
+    assert(opt);
 
     list = g_hash_table_lookup(unprocessed_opts, opt->name);
     if (list == NULL) {
@@ -127,6 +132,9 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
     if (obj) {
         *obj = g_malloc0(size > 0 ? size : 1);
     }
+
+    g_queue_push_tail(ov->nested_names, (gpointer) name);
+
     if (ov->depth++ > 0) {
         return;
     }
@@ -169,6 +177,8 @@ opts_end_struct(Visitor *v, Error **errp)
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     GQueue *any;
 
+    g_queue_pop_tail(ov->nested_names);
+
     if (--ov->depth > 0) {
         return;
     }
@@ -198,15 +208,55 @@ opts_end_implicit_struct(Visitor *v, Error **errp)
 }
 
 
+static void
+sum_strlen(gpointer data, gpointer user_data)
+{
+    const char *str = data;
+    size_t *sum_len = user_data;
+
+    if (str) { /* skip NULLs */
+        *sum_len += strlen(str) + 1;
+    }
+}
+
+static void
+append_str(gpointer data, gpointer user_data)
+{
+    const char *str = data;
+    char *concat_str = user_data;
+
+    if (str) {
+        strcat(concat_str, str);
+        strcat(concat_str, ".");
+    }
+}
+
+/* lookup a name, using a fully qualified version */
 static GQueue *
-lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
+lookup_distinct(const OptsVisitor *ov, const char *name, char **out_key,
+                Error **errp)
 {
-    GQueue *list;
+    GQueue *list = NULL;
+    char *key;
+    size_t sum_len = strlen(name);
+
+    g_queue_foreach(ov->nested_names, sum_strlen, &sum_len);
+    key = g_malloc(sum_len+1);
+    key[0] = 0;
+    g_queue_foreach(ov->nested_names, append_str, key);
+    strcat(key, name);
+
+    list = g_hash_table_lookup(ov->unprocessed_opts, key);
+    if (list && out_key) {
+        *out_key = key;
+        key = NULL;
+    }
 
-    list = g_hash_table_lookup(ov->unprocessed_opts, name);
     if (!list) {
         error_setg(errp, QERR_MISSING_PARAMETER, name);
     }
+
+    g_free(key);
     return list;
 }
 
@@ -218,7 +268,8 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
 
     /* we can't traverse a list in a list */
     assert(ov->list_mode == LM_NONE);
-    ov->repeated_opts = lookup_distinct(ov, name, errp);
+    assert(ov->repeated_opts == NULL && ov->repeated_name == NULL);
+    ov->repeated_opts = lookup_distinct(ov, name, &ov->repeated_name, errp);
     if (ov->repeated_opts != NULL) {
         ov->list_mode = LM_STARTED;
     }
@@ -254,11 +305,9 @@ opts_next_list(Visitor *v, GenericList **list, Error 
**errp)
         /* range has been completed, fall through in order to pop option */
 
     case LM_IN_PROGRESS: {
-        const QemuOpt *opt;
-
-        opt = g_queue_pop_head(ov->repeated_opts);
+        g_queue_pop_head(ov->repeated_opts);
         if (g_queue_is_empty(ov->repeated_opts)) {
-            g_hash_table_remove(ov->unprocessed_opts, opt->name);
+            g_hash_table_remove(ov->unprocessed_opts, ov->repeated_name);
             return NULL;
         }
         link = &(*list)->next;
@@ -284,22 +333,28 @@ opts_end_list(Visitor *v, Error **errp)
            ov->list_mode == LM_SIGNED_INTERVAL ||
            ov->list_mode == LM_UNSIGNED_INTERVAL);
     ov->repeated_opts = NULL;
+
+    g_free(ov->repeated_name);
+    ov->repeated_name = NULL;
+
     ov->list_mode = LM_NONE;
 }
 
 
 static const QemuOpt *
-lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
+lookup_scalar(const OptsVisitor *ov, const char *name, char** out_key,
+              Error **errp)
 {
     if (ov->list_mode == LM_NONE) {
         GQueue *list;
 
         /* the last occurrence of any QemuOpt takes effect when queried by name
          */
-        list = lookup_distinct(ov, name, errp);
+        list = lookup_distinct(ov, name, out_key, errp);
         return list ? g_queue_peek_tail(list) : NULL;
     }
     assert(ov->list_mode == LM_IN_PROGRESS);
+    assert(out_key == NULL || *out_key == NULL);
     return g_queue_peek_head(ov->repeated_opts);
 }
 
@@ -321,13 +376,15 @@ opts_type_str(Visitor *v, char **obj, const char *name, 
Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
     *obj = g_strdup(opt->str ? opt->str : "");
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -337,8 +394,9 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, 
Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -355,13 +413,15 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, 
Error **errp)
         } else {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                        "on|yes|y|off|no|n");
+            g_free(key);
             return;
         }
     } else {
         *obj = true;
     }
 
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -373,13 +433,14 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, 
Error **errp)
     const char *str;
     long long val;
     char *endptr;
+    char *key = NULL;
 
     if (ov->list_mode == LM_SIGNED_INTERVAL) {
         *obj = ov->range_next.s;
         return;
     }
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -393,11 +454,13 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, 
Error **errp)
     if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
         if (*endptr == '\0') {
             *obj = val;
-            processed(ov, name);
+            processed(ov, key);
+            g_free(key);
             return;
         }
         if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
             long long val2;
+            assert(key == NULL);
 
             str = endptr + 1;
             val2 = strtoll(str, &endptr, 0);
@@ -418,6 +481,7 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, 
Error **errp)
     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                (ov->list_mode == LM_NONE) ? "an int64 value" :
                                             "an int64 value or range");
+    g_free(key);
 }
 
 
@@ -429,13 +493,14 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char 
*name, Error **errp)
     const char *str;
     unsigned long long val;
     char *endptr;
+    char *key = NULL;
 
     if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
         *obj = ov->range_next.u;
         return;
     }
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -447,11 +512,13 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char 
*name, Error **errp)
     if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) {
         if (*endptr == '\0') {
             *obj = val;
-            processed(ov, name);
+            processed(ov, key);
+            g_free(key);
             return;
         }
         if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
             unsigned long long val2;
+            assert(key == NULL);
 
             str = endptr + 1;
             if (parse_uint_full(str, &val2, 0) == 0 &&
@@ -470,6 +537,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char 
*name, Error **errp)
     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                (ov->list_mode == LM_NONE) ? "a uint64 value" :
                                             "a uint64 value or range");
+    g_free(key);
 }
 
 
@@ -480,8 +548,9 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, 
Error **errp)
     const QemuOpt *opt;
     int64_t val;
     char *endptr;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -491,11 +560,13 @@ opts_type_size(Visitor *v, uint64_t *obj, const char 
*name, Error **errp)
     if (val < 0 || *endptr) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                    "a size value representible as a non-negative int64");
+        g_free(key);
         return;
     }
 
     *obj = val;
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -506,7 +577,7 @@ opts_optional(Visitor *v, bool *present, const char *name)
 
     /* we only support a single mandatory scalar field in a list node */
     assert(ov->list_mode == LM_NONE);
-    *present = (lookup_distinct(ov, name, NULL) != NULL);
+    *present = (lookup_distinct(ov, name, NULL, NULL) != NULL);
 }
 
 
@@ -517,6 +588,8 @@ opts_visitor_new(const QemuOpts *opts)
 
     ov = g_malloc0(sizeof *ov);
 
+    ov->nested_names = g_queue_new();
+
     ov->visitor.start_struct = &opts_start_struct;
     ov->visitor.end_struct   = &opts_end_struct;
 
@@ -560,6 +633,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
     if (ov->unprocessed_opts != NULL) {
         g_hash_table_destroy(ov->unprocessed_opts);
     }
+    g_queue_free(ov->nested_names);
     g_free(ov->fake_id_opt);
     g_free(ov);
 }
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index ad37013..3a650c7 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -106,6 +106,11 @@
 { 'command': 'boxed', 'box': true, 'data': 'UserDefZero' }
 { 'command': 'boxed2', 'data': 'UserDefNativeListUnion', 'box': true }
 
+# For testing hierarchy support in opts-visitor
+{ 'struct': 'UserDefOptionsSub',
+  'data': {
+    '*nint': 'int' } }
+
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
 #
@@ -119,7 +124,9 @@
     '*u64' : [ 'uint64' ],
     '*u16' : [ 'uint16' ],
     '*i64x':   'int'     ,
-    '*u64x':   'uint64'  } }
+    '*u64x':   'uint64'  ,
+    'sub0':    'UserDefOptionsSub',
+    'sub1':    'UserDefOptionsSub' } }
 
 # testing event
 { 'struct': 'EventStructOne',
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index b3b4cb8..83f0d69 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -148,6 +148,10 @@ object UserDefOptions
     member u16: uint16List optional=True
     member i64x: int optional=True
     member u64x: uint64 optional=True
+    member sub0: UserDefOptionsSub optional=False
+    member sub1: UserDefOptionsSub optional=False
+object UserDefOptionsSub
+    member nint: int optional=True
 object UserDefThree
     base UserDefOne
     member base: str optional=False
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 1c753d9..4393266 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -178,6 +178,34 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer 
test_data)
     g_assert(f->userdef->u64->value == UINT64_MAX);
 }
 
+static void
+expect_both(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub0->nint == 13);
+    g_assert(f->userdef->sub1->has_nint);
+    g_assert(f->userdef->sub1->nint == 17);
+}
+
+static void
+expect_sub0(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub0->nint == 13);
+    g_assert(!f->userdef->sub1->has_nint);
+}
+
+static void
+expect_sub1(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(!f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub1->has_nint);
+    g_assert(f->userdef->sub1->nint == 13);
+}
+
 /* test cases */
 
 int
@@ -271,6 +299,12 @@ main(int argc, char **argv)
     add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
              "i64=-0x8000000000000000-0x7fffffffffffffff");
 
+    /* Test nested structs support */
+    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");
+    add_test("/visitor/opts/nested/both",        &expect_both,
+             "sub0.nint=13,sub1.nint=17");
+    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
+    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");
     g_test_run();
     return 0;
 }
-- 
2.5.2