[Qemu-devel] [PATCH v14 10/21] qapi: permit auto-creating nested structs

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

Some of the historical command line opts that had their
keys in in a completely flat namespace are now represented
by QAPI schemas that use a nested structs. When converting
the QemuOpts to QObject, there is no information about
compound types available, so the QObject will be completely
flat, even after the qdict_crumple() call. So when starting
a struct, we may not have a QDict available in the input
data, so we auto-create a QDict containing all the currently
unvisited input data keys. Not all historical command line
opts require this, so the behaviour is opt-in, by specifying
how many levels of structs are permitted to be auto-created.

Note that this only works if the child struct is the last
field to the visited in the parent struct. This is always
the case for currently existing legacy command line options.

The example is the NetLegacy type which has 3 levels of
structs. The modern way to represent this in QemuOpts would
be the dot-separated component approach

  -net vlan=1,id=foo,name=bar,opts.type=tap,\
       opts.data.fd=3,opts.data.script=ifup

The legacy syntax will just be presenting

  -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup

So we need to auto-create 3 levels of struct when visiting.

The implementation here will enable visiting in both the
modern and legacy syntax, compared to OptsVisitor which
only allows the legacy syntax.

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

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 1809f48..94051f3 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -45,7 +45,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
  * If @autocreate_list is true, then as an alternative to a normal QList,
  * list values can be stored as a QString or QDict instead, which will
  * be interpreted as representing single element lists. This should only
- * by used if compatibility is required with the OptsVisitor which allowed
+ * be used if compatibility is required with the OptsVisitor which allowed
  * repeated keys, without list indexes, to represent lists. e.g. set this
  * to true if you have compatibility requirements for
  *
@@ -55,6 +55,22 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
  *
  *   -arg foo.0=hello,foo.1=world
  *
+ * If @autocreate_struct_levels is non-zero, then when visiting structs,
+ * if the corresponding QDict is not found, it will automatically create
+ * a QDict containing all remaining unvisited options. This should only
+ * be used if compatibility is required with the OptsVisitor which flatten
+ * structs so that all keys were at the same level. e.g. set this to a
+ * non-zero number if you compatibility requirements for
+ *
+ *   -arg type=person,surname=blogs,forename=fred
+ *
+ * to be treated as equivalent to the perferred syntax
+ *
+ *   -arg type=person,data.surname=blogs,data.forename=fred
+ *
+ * The value given determines how many levels of structs are allowed to
+ * be flattened in this way.
+ *
  * 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.
@@ -63,7 +79,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
  * visit_free() when no longer required.
  */
 Visitor *qobject_input_visitor_new_autocast(QObject *obj,
-                                            bool autocreate_list);
+                                            bool autocreate_list,
+                                            size_t autocreate_struct_levels);
 
 
 /**
@@ -80,6 +97,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,
                                         Error **errp);
 
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index d88e9f9..1be4865 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -52,6 +52,14 @@ struct QObjectInputVisitor
     /* Whether we can auto-create single element lists when
      * encountering a non-QList type */
     bool autocreate_list;
+
+    /* Current depth of recursion into structs */
+    size_t struct_level;
+
+    /* Numbers of levels at which we will
+     * consider auto-creating a struct containing
+     * remaining unvisited items */
+    size_t autocreate_struct_levels;
 };
 
 static QObjectInputVisitor *to_qiv(Visitor *v)
@@ -79,7 +87,12 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
 
     if (qobject_type(qobj) == QTYPE_QDICT) {
         assert(name);
-        ret = qdict_get(qobject_to_qdict(qobj), name);
+        if (qiv->autocreate_struct_levels &&
+            !g_hash_table_contains(tos->h, name)) {
+            ret = NULL;
+        } else {
+            ret = qdict_get(qobject_to_qdict(qobj), name);
+        }
         if (tos->h && consume && ret) {
             bool removed = g_hash_table_remove(tos->h, name);
             assert(removed);
@@ -114,7 +127,8 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
     tos->qapi = qapi;
     qobject_incref(obj);
 
-    if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
+    if ((qiv->autocreate_struct_levels || qiv->strict) &&
+        qobject_type(obj) == QTYPE_QDICT) {
         h = g_hash_table_new(g_str_hash, g_str_equal);
         qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
         tos->h = h;
@@ -163,6 +177,9 @@ static void qobject_input_pop(Visitor *v, void **obj)
     StackObject *tos = QSLIST_FIRST(&qiv->stack);
 
     assert(tos && tos->qapi == obj);
+    if (tos->h != NULL) {
+        qiv->struct_level--;
+    }
     QSLIST_REMOVE_HEAD(&qiv->stack, node);
     qobject_input_stack_object_free(tos);
 }
@@ -171,12 +188,38 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
                                        size_t size, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true);
+    QObject *qobj;
+    QDict *subopts = NULL;
     Error *err = NULL;
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
 
+    qobj = qobject_input_get_object(qiv, name, true);
     if (obj) {
         *obj = NULL;
     }
+
+    if (!qobj && (qiv->struct_level < qiv->autocreate_struct_levels)) {
+        /* Create a new dict that contains all the currently
+         * unvisited items */
+        if (tos) {
+            GHashTableIter iter;
+            const char *key;
+
+            subopts = qdict_new();
+
+            g_hash_table_iter_init(&iter, tos->h);
+            while (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
+                QObject *val = qdict_get(qobject_to_qdict(tos->obj), key);
+                qobject_incref(val);
+                qdict_put_obj(subopts, key, val);
+            }
+            g_hash_table_remove_all(tos->h);
+            qobj = QOBJECT(subopts);
+        } else {
+            qobj = qiv->root;
+        }
+    }
+
     if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "QDict");
@@ -184,6 +227,7 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
     }
 
     qobject_input_push(qiv, qobj, obj, &err);
+    qiv->struct_level++;
     if (err) {
         error_propagate(errp, err);
         return;
@@ -192,6 +236,7 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
     if (obj) {
         *obj = g_malloc0(size);
     }
+    QDECREF(subopts);
 }
 
 
@@ -530,7 +575,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict)
 }
 
 Visitor *qobject_input_visitor_new_autocast(QObject *obj,
-                                            bool autocreate_list)
+                                            bool autocreate_list,
+                                            size_t autocreate_struct_levels)
 {
     QObjectInputVisitor *v;
 
@@ -556,6 +602,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
     v->visitor.free = qobject_input_free;
     v->strict = true;
     v->autocreate_list = autocreate_list;
+    v->autocreate_struct_levels = autocreate_struct_levels;
 
     v->root = obj;
     qobject_incref(obj);
@@ -566,6 +613,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,
                                         Error **errp)
 {
     QDict *pdict;
@@ -583,7 +631,8 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
     }
 
     v = qobject_input_visitor_new_autocast(pobj,
-                                           autocreate_list);
+                                           autocreate_list,
+                                           autocreate_struct_levels);
  cleanup:
     qobject_decref(pobj);
     QDECREF(pdict);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index c227565..ab55f99 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -40,11 +40,13 @@ static void visitor_input_teardown(TestInputVisitorData *data,
 /* The various test_init functions are provided instead of a test setup
    function so that the JSON string used by the tests are kept in the test
    functions (and not in main()). */
-static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
-                                                 bool strict, bool autocast,
-                                                 bool autocreate_list,
-                                                 const char *json_string,
-                                                 va_list *ap)
+static Visitor *
+visitor_input_test_init_internal(TestInputVisitorData *data,
+                                 bool strict, bool autocast,
+                                 bool autocreate_list,
+                                 size_t autocreate_struct_levels,
+                                 const char *json_string,
+                                 va_list *ap)
 {
     visitor_input_teardown(data, NULL);
 
@@ -53,10 +55,11 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
 
     if (autocast) {
         assert(strict);
-        data->qiv = qobject_input_visitor_new_autocast(data->obj,
-                                                       autocreate_list);
+        data->qiv = qobject_input_visitor_new_autocast(
+            data->obj, autocreate_list, autocreate_struct_levels);
     } else {
         assert(!autocreate_list);
+        assert(!autocreate_struct_levels);
         data->qiv = qobject_input_visitor_new(data->obj, strict);
     }
     g_assert(data->qiv);
@@ -74,7 +77,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,
+                                         autocreate_list, 0,
                                          json_string, &ap);
     va_end(ap);
     return v;
@@ -88,7 +91,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,
+    v = visitor_input_test_init_internal(data, true, false, false, 0,
                                          json_string, &ap);
     va_end(ap);
     return v;
@@ -104,7 +107,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,
+    return visitor_input_test_init_internal(data, true, false, false, 0,
                                             json_string, NULL);
 }
 
@@ -372,6 +375,109 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
     qapi_free_UserDefTwo(udp);
 }
 
+static void test_visitor_in_struct_autocreate(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    Visitor *v;
+    int64_t vlan;
+    char *id = NULL;
+    char *type;
+    int64_t fd;
+    char *script = NULL;
+
+    v = visitor_input_test_init_internal(
+        data, true, true, false, 3,
+        "{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
+        "'script': 'ifup' }", NULL);
+
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+    visit_type_int64(v, "vlan", &vlan, &error_abort);
+    visit_type_str(v, "id", &id, &error_abort);
+
+    visit_start_struct(v, "opts", NULL, 0, &error_abort);
+    visit_type_str(v, "type", &type, &error_abort);
+
+    visit_start_struct(v, "data", NULL, 0, &error_abort);
+
+    visit_type_int64(v, "fd", &fd, &error_abort);
+    visit_type_str(v, "script", &script, &error_abort);
+
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+
+    g_assert_cmpstr(id, ==, "foo");
+    g_assert_cmpint(vlan, ==, 1);
+    g_assert_cmpstr(type, ==, "tap");
+    g_assert_cmpstr(script, ==, "ifup");
+    g_assert_cmpint(fd, ==, 3);
+
+    g_free(id);
+    g_free(type);
+    g_free(script);
+}
+
+static void test_visitor_in_struct_autocreate_extra(TestInputVisitorData *data,
+                                                    const void *unused)
+{
+    Visitor *v;
+    int64_t vlan;
+    char *id = NULL;
+    char *type;
+    int64_t fd;
+    char *script = NULL;
+    Error *err = NULL;
+
+    v = visitor_input_test_init_internal(
+        data, true, true, false, 3,
+        "{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
+        "'script': 'ifup' }", NULL);
+
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+    visit_type_int64(v, "vlan", &vlan, &error_abort);
+
+    visit_start_struct(v, "opts", NULL, 0, &error_abort);
+    visit_type_str(v, "type", &type, &error_abort);
+
+    visit_start_struct(v, "data", NULL, 0, &error_abort);
+
+    visit_type_int64(v, "fd", &fd, &error_abort);
+    visit_type_str(v, "script", &script, &error_abort);
+
+    /* We've not visited 'id' so should see a complaint */
+    visit_check_struct(v, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+
+    /* We can't visit stuff in the base struct after
+     * we auto-created child structs */
+    visit_type_str(v, "id", &id, &err);
+    error_free_or_abort(&err);
+
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+
+    g_assert_cmpstr(id, ==, NULL);
+    g_assert_cmpint(vlan, ==, 1);
+    g_assert_cmpstr(type, ==, "tap");
+    g_assert_cmpstr(script, ==, "ifup");
+    g_assert_cmpint(fd, ==, 3);
+
+    g_free(id);
+    g_free(type);
+    g_free(script);
+}
+
 static void test_visitor_in_list(TestInputVisitorData *data,
                                  const void *unused)
 {
@@ -1096,6 +1202,10 @@ int main(int argc, char **argv)
                            NULL, test_visitor_in_struct);
     input_visitor_test_add("/visitor/input/struct-nested",
                            NULL, test_visitor_in_struct_nested);
+    input_visitor_test_add("/visitor/input/struct-autocreate",
+                           NULL, test_visitor_in_struct_autocreate);
+    input_visitor_test_add("/visitor/input/struct-autocreate-extra",
+                           NULL, test_visitor_in_struct_autocreate_extra);
     input_visitor_test_add("/visitor/input/list",
                            NULL, test_visitor_in_list);
     input_visitor_test_add("/visitor/input/list-autocreate-noautocast",
-- 
2.7.4