qapi changes in preparation for blockdev-add

Signed-off-by Kevin Wolf <[email protected]>

---
 scripts/qapi-types.py | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

The new 'base' key in a union definition refers to a struct type, which is inlined into the union definition and can represent fields common to all kinds.

Signed-off-by Kevin Wolf <[email protected]>

---
 scripts/qapi-types.py | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Quoted 3 lines · Kevin Wolf

The new 'base' key in a union definition refers to a struct type, which is inlined into the union definition and can represent fields common to all kinds.

Is it worth listing an example of intended use in the commit message? If I understand correctly, then this qapi-schema.json file:

{ 'type': 'Base',
'data': { 'main': 'str' } }
{ 'type': 'Extra1',
'data': { 'foo': 'str' } }
{ 'type': 'Extra2',
'data': { 'bar': 'str' } }
{ 'union': 'Type',
'base': 'BaseType',
'data': { 'variant1': 'Extra1',

            'variant2': 'Extra2' } }

{ 'command': 'test',
'data': { 'arg': 'Type' } }

would then be used over the wire as:

{ "execute": "test",
"arguments": { "arg": { "type": "variant1",

       "data": { "main": "hello", "foo": "world" } } } }
{ "execute": "test",
  "arguments": { "arg": { "type": "variant2",
       "data": { "main": "hi", "bar": "there" } } } }

Signed-off-by Kevin Wolf <[email protected]>

---
 scripts/qapi-visit.py | 62 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

Signed-off-by Kevin Wolf <[email protected]>

---
 scripts/qapi-visit.py | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

These can be used when an embedded struct is parsed and members not belonging to the struct may be present in the input (parsing flat namespect QMP union with discriminator)

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qapi/visitor-impl.h |  4 ++++
 include/qapi/visitor.h      |  3 +++
 qapi/qapi-visit-core.c      | 16 ++++++++++++++++
 qapi/qmp-input-visitor.c    | 14 ++++++++++++++
 4 files changed, 37 insertions(+)

Quoted 3 lines · Kevin Wolf

These can be used when an embedded struct is parsed and members not belonging to the struct may be present in the input (parsing flat namespect QMP union with discriminator)

namespect? Not sure if you meant 'namespaced'?

Again, a comment demonstrating in sample QMP on what this is attempting would be helpful in the commit message. Without looking at later patches yet,

Instead of the rather verbose syntax that distinguishes base and subclass fields...

  { "type": "file",
    "read-only": true,
    "data": {
        "filename": "test"
    } }

...we can now have both in the same namespace, allowing a more direct mapping of the command line, and moving fields between the common base and subclasses without breaking the API:

{ "driver": "file",

    "read-only": true,
    "filename": "test" }

Signed-off-by Kevin Wolf <[email protected]>

---
 scripts/qapi-types.py | 10 +++---
 scripts/qapi-visit.py | 91 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 70 insertions(+), 31 deletions(-)

scripts/qapi-types.py +0 -0

index 960065b..db2f533 100644

This allows to just look at the next element without actually consuming it.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/qmp-input-visitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

The discriminator for anonymous unions is the data type. This allows to have a union type that allows both of these:

    { 'file': 'my_existing_block_device_id' }
    { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }

Unions like this are specified in the schema with an empty dict as discriminator. For this example you could take:

    { 'union': 'BlockRef',
      'discriminator': {},
      'data': { 'definition': 'BlockOptions'
                'reference': 'str' } }
    { 'type': 'ExampleObject',
      'data: { 'file': 'BlockRef' } }

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qapi/qmp/qobject.h  |  1 +
 include/qapi/visitor-impl.h |  2 ++
 include/qapi/visitor.h      |  3 +++
 qapi/qapi-visit-core.c      |  9 +++++++++
 qapi/qmp-input-visitor.c    | 14 ++++++++++++++
 qobject/qjson.c             |  2 ++
 scripts/qapi-types.py       | 42 ++++++++++++++++++++++++++++++++++++++++
 scripts/qapi-visit.py       | 47 +++++++++++++++++++++++++++++++++++++++++++++
 scripts/qapi.py             | 28 +++++++++++++++++++++++++++
 9 files changed, 148 insertions(+)

+++ b/scripts/qapi-visit.py

scripts/qapi-visit.py +18 -0

@@ -172,6 +172,49 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
 ''',
                  name=name)
 
+def generate_visit_anon_union(name, members):
+    ret = mcgen('''
+
+void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+{
+    Error *err = NULL;
+
+    if (!error_is_set(errp)) {
+        visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err);
+        visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err);
+        switch ((*obj)->kind) {
+''',
+    name=name)
+
+    for key in members:
+        assert (members[key] in builtin_types
+            or find_struct(members[key])
+            or find_union(members[key])), "Invalid anonymous union member"

scripts/qapi.py +4 -0

+++ b/scripts/qapi.py

@@ -17,6 +17,21 @@ builtin_types = [
     'uint8', 'uint16', 'uint32', 'uint64'
 ]
 
+builtin_type_qtypes = {
+    'str':      'QTYPE_QSTRING',
+    'int':      'QTYPE_QINT',

scripts/qapi.py +3 -0

+}
+
 def tokenize(data):
     while len(data):
         ch = data[0]

@@ -105,6 +120,7 @@ def parse_schema(fp):
         if expr_eval.has_key('enum'):
             add_enum(expr_eval['enum'])
         elif expr_eval.has_key('union'):
+            add_union(expr_eval)
             add_enum('%sKind' % expr_eval['union'])
         elif expr_eval.has_key('type'):
             add_struct(expr_eval)

Quoted 9 lines

The discriminator for anonymous unions is the data type. This allows to have a union type that allows both of these:

    { 'file': 'my_existing_block_device_id' }
    { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }

Unions like this are specified in the schema with an empty dict as discriminator. For this example you could take:

Quoted 6 lines

    { 'union': 'BlockRef',
      'discriminator': {},
      'data': { 'definition': 'BlockOptions'
                'reference': 'str' } }
    { 'type': 'ExampleObject',
      'data: { 'file': 'BlockRef' } }

Yay - a commit message that shows the new QMP wire format, and the qapi-schema.json that generated it.

[Without reading the patch yet]
I take it the 'data' of such a union must be completely distinguishable by type - the visitor for the 'file' key tries each subtype in turn ('BlockOptions' or 'str') until one does not have a parse error.

And
this changes the earlier patch that required 'discriminator' to be used only when 'base' was also present. Can it distinguish between two structs, or is the union only allowed to have one struct option and all other options are primitive types? Should we consider the use of arrays as a union member type?

Hmm, I wonder if this new anonymous union can be useful in helping us write the self-description of introspection in Amos' patches.

Quoted 41 lines

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qapi/qmp/qobject.h  |  1 +
 include/qapi/visitor-impl.h |  2 ++
 include/qapi/visitor.h      |  3 +++
 qapi/qapi-visit-core.c      |  9 +++++++++
 qapi/qmp-input-visitor.c    | 14 ++++++++++++++
 qobject/qjson.c             |  2 ++
 scripts/qapi-types.py       | 42 ++++++++++++++++++++++++++++++++++++++++
 scripts/qapi-visit.py       | 47 +++++++++++++++++++++++++++++++++++++++++++++
 scripts/qapi.py             | 28 +++++++++++++++++++++++++++
 9 files changed, 148 insertions(+)

+++ b/scripts/qapi-visit.py

scripts/qapi-visit.py +22 -0

@@ -172,6 +172,49 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
 ''',
                  name=name)
 
+def generate_visit_anon_union(name, members):
+    ret = mcgen('''
+
+void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+{
+    Error *err = NULL;
+
+    if (!error_is_set(errp)) {
+        visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err);
+        visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err);
+        switch ((*obj)->kind) {
+''',
+    name=name)
+
+    for key in members:
+        assert (members[key] in builtin_types
+            or find_struct(members[key])
+            or find_union(members[key])), "Invalid anonymous union member"
+
+        ret += mcgen('''
+        case %(abbrev)s_KIND_%(enum)s:
+            visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);

scripts/qapi-visit.py void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e · line 202

Quoted 6 lines

scripts/qapi-visit.py +6 -0

+            break;
+''',
+                abbrev = de_camel_case(name).upper(),
+                enum = c_fun(de_camel_case(key),False).upper(),
+                c_type=type_name(members[key]),
+                c_name=c_fun(key))

Inconsistent spacing around '='

scripts/qapi-visit.py void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e · line 206

Quoted 4 lines

scripts/qapi-visit.py +4 -0

+
+    ret += mcgen('''
+        default:
+            abort();

Does this mean I can cause qemu to abort if I pass bogus information on the wire? Using your commit message example,

    { 'file': false }

would hit the default case, right?

Quoted 15 lines

scripts/qapi.py +11 -0

+++ b/scripts/qapi.py

@@ -17,6 +17,21 @@ builtin_types = [
     'uint8', 'uint16', 'uint32', 'uint64'
 ]
 
+builtin_type_qtypes = {
+    'str':      'QTYPE_QSTRING',
+    'int':      'QTYPE_QINT',
+    'number':   'QTYPE_QFLOAT',
+    'bool':     'QTYPE_QBOOL',
+    'int8':     'QTYPE_QINT',
+    'int16':    'QTYPE_QINT',
+    'int32':    'QTYPE_QINT',
+    'int64':    'QTYPE_QINT',
+    'uint8':    'QTYPE_QINT',

scripts/qapi.py builtin_types = [ · line 32

Quoted 3 lines

scripts/qapi.py +3 -0

+    'uint16':   'QTYPE_QINT',
+    'uint32':   'QTYPE_QINT',
+    'uint64':   'QTYPE_QINT',

What happens if I try to write a union in qapi-schema.json that has both a 'int8' and 'uint32' branch? Since both of those resolve to the QTYPE_QINT visitor, does that end up causing the C code to generate a switch statement with duplicate labels?

Quoted 15 lines

scripts/qapi.py +3 -0

+}
+
 def tokenize(data):
     while len(data):
         ch = data[0]

@@ -105,6 +120,7 @@ def parse_schema(fp):
         if expr_eval.has_key('enum'):
             add_enum(expr_eval['enum'])
         elif expr_eval.has_key('union'):
+            add_union(expr_eval)
             add_enum('%sKind' % expr_eval['union'])
         elif expr_eval.has_key('type'):
             add_struct(expr_eval)

@@ -188,6 +204,7 @@ def type_name(name):
 

scripts/qapi.py def type_name(name): · line 207

Quoted 3 lines

scripts/qapi.py +1 -0

 enum_types = []
 struct_types = []
+union_types = []

Is it worth splitting the tracking of the union_types of qapi-schema.json into an independent patch?

Does it make sense to allow a union type (possibly only an anonymous union type) in place of a struct type as a top-level type reference? That is, just as we now allow { 'command':'foo', 'data': 'StructType' }, would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?

Am 11.07.2013 um 21:47 hat Eric Blake geschrieben:

Quoted 17 lines · Kevin Wolf

Quoted 15 lines

The discriminator for anonymous unions is the data type. This allows to have a union type that allows both of these:

    { 'file': 'my_existing_block_device_id' }
    { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }

Unions like this are specified in the schema with an empty dict as discriminator. For this example you could take:

    { 'union': 'BlockRef',
      'discriminator': {},
      'data': { 'definition': 'BlockOptions'
                'reference': 'str' } }
    { 'type': 'ExampleObject',
      'data: { 'file': 'BlockRef' } }

Quoted 2 lines

Yay - a commit message that shows the new QMP wire format, and the qapi-schema.json that generated it.

Yeah, sorry about these missing examples and detailed documentation. I wanted to get an RFC out quickly and probably underestimated how important it is even for reviewing an RFC.

However, I think keeping this information only in the commit messages isn't right either. Do we have any of the schema syntax documented anywhere? Should I create something in docs/(specs/)?

Quoted 5 lines

[Without reading the patch yet]
I take it the 'data' of such a union must be completely distinguishable by type - the visitor for the 'file' key tries each subtype in turn ('BlockOptions' or 'str') until one does not have a parse error.

Not exactly. It asks the visitor for the type of the next item and then uses a lookup table to get the right discriminator enum value for it.

Quoted 7 lines

And
this changes the earlier patch that required 'discriminator' to be used only when 'base' was also present. Can it distinguish between two structs, or is the union only allowed to have one struct option and all other options are primitive types? Should we consider the use of arrays as a union member type?

As the types it uses for lookup are QObject types and all structs map to QDict, only one struct is allowed.

This makes sense anyway, because allowing different structs would mean that you potentially get ambiguities when structs look alike (perhaps only differing in optional fields or something like that).

As soon as we find a good use case for anonymous unions of two structs, I might change my opinion, though. ;-)

Arrays are a logical extension of this, though I don't have any use for them now. I wouldn't expect them to be too hard to implement.

Quoted 3 lines

Hmm, I wonder if this new anonymous union can be useful in helping us write the self-description of introspection in Amos' patches.

Hm, what's the exact problem there? I know I read that someone (Paolo?) wrote that it was impossible, but I didn't immediately understand why and didn't feel like thinking a lot about either.

Quoted 10 lines

scripts/qapi-visit.py void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e · line 206

Quoted 4 lines

scripts/qapi-visit.py +4 -0

+
+    ret += mcgen('''
+        default:
+            abort();

Does this mean I can cause qemu to abort if I pass bogus information on the wire? Using your commit message example,

    { 'file': false }

Quoted 1 line

would hit the default case, right?

No, it wouldn't, but that case fails in an interesting way anyway. :-)

We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which isn't defined and therefore 0. This happens to be the enum value for BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message like "invalid argument type: got bool, expected dict".

Quoted 5 lines

Does it make sense to allow a union type (possibly only an anonymous union type) in place of a struct type as a top-level type reference? That is, just as we now allow { 'command':'foo', 'data': 'StructType' }, would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?

Sounds sensible to me. But do we have a use case for it?

+ * qdict_flatten(): For each nested QDict with key x, all fields with

+ * are moved to this QDict and their key is renamed to "x.y".

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qapi/qmp/qdict.h |  1 +
 qobject/qdict.c          | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

+++ b/qobject/qdict.c

qobject/qdict.c +11 -0

@@ -476,3 +476,50 @@ static void qdict_destroy_obj(QObject *obj)
 
     g_free(qdict);
 }
+
+static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
+{
+    QObject *value;
+    const QDictEntry *entry, *next;
+    const char *new_key;
+    bool delete;
+
+    entry = qdict_first(qdict);
+
+    while (entry != NULL) {

qobject/qdict.c +20 -0

+            delete = true;
+        }
+
+        if (delete) {
+            qdict_del(qdict, entry->key);
+        }
+
+        entry = next;
+    }
+}
+
+/**
+ * qdict_flatten(): For each nested QDict with key x, all fields with key y
+ * are moved to this QDict and their key is renamed to "x.y".
+ */
+void qdict_flatten(QObject *obj)
+{
+    QDict *qdict = qobject_to_qdict(obj);
+    qdict_do_flatten(qdict, qdict, NULL);
+}

Worth repeating this comment from the code into the commit message?

Quoted 1 line

+ * qdict_flatten(): For each nested QDict with key x, all fields with

key y

Quoted 1 line

+ * are moved to this QDict and their key is renamed to "x.y".

Otherwise, I had to read nearly the entire patch to learn what I was supposed to be reviewing.

Quoted 10 lines

qobject/qdict.c +10 -0

+static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
+{
+    QObject *value;
+    const QDictEntry *entry, *next;
+    const char *new_key;
+    bool delete;
+
+    entry = qdict_first(qdict);
+
+    while (entry != NULL) {

qobject/qdict.c static void qdict_destroy_obj(QObject *obj) · line 499

Quoted 10 lines

qobject/qdict.c +10 -0

+
+        next = qdict_next(qdict, entry);
+        value = qdict_entry_value(entry);
+        new_key = NULL;
+        delete = false;
+
+        if (prefix) {
+            qobject_incref(value);
+            new_key = g_strdup_printf("%s.%s", prefix, entry->key);
+            qdict_put_obj(target, new_key, value);

You are calling this function with the same parameter for both qdict and target. Doesn't that mean you are modifying qdict while iterating it? Is that safe? [/me re-reads] - oh, you recursively call this function, and this modification of target happens _only_ if prefix is non-null, which happens only:

qobject/qdict.c static void qdict_destroy_obj(QObject *obj) · line 505

Quoted 6 lines

qobject/qdict.c +6 -0

+            delete = true;
+        }
+
+        if (qobject_type(value) == QTYPE_QDICT) {
+            qdict_do_flatten(qobject_to_qdict(value), target,
+                             new_key ? new_key : entry->key);

when passing two different dicts into the function. Maybe add an assert(!prefix || qdict != target).

Quoted 21 lines

qobject/qdict.c +20 -0

+            delete = true;
+        }
+
+        if (delete) {
+            qdict_del(qdict, entry->key);
+        }
+
+        entry = next;
+    }
+}
+
+/**
+ * qdict_flatten(): For each nested QDict with key x, all fields with key y
+ * are moved to this QDict and their key is renamed to "x.y".
+ */
+void qdict_flatten(QObject *obj)
+{
+    QDict *qdict = qobject_to_qdict(obj);
+    qdict_do_flatten(qdict, qdict, NULL);
+}

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

This is traditionally -drive format=..., which is now translated into the new driver option. This gives us a more consistent way to select the driver of BlockDriverStates that can be used in QMP context, too.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c    |  7 +++++++
 blockdev.c | 20 ++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

This is just a quick hack to test things

Signed-off-by Kevin Wolf <[email protected]>

---
 blockdev.c       | 32 ++++++++++++++++++++++++++++++++
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qmp-commands.hx  |  6 ++++++
 3 files changed, 67 insertions(+)

blockdev.c +7 -0

index e71c4ee..e3a4fb8 100644
@@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+
+void qmp_blockdev_add(BlockOptions *options, Error **errp)
+{
+    QString *str;
+    QmpOutputVisitor *ov = qmp_output_visitor_new();

blockdev.c +4 -0

+
+    Error *local_err = NULL;
+    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qobject_to_qdict(obj), &local_err);
+    if (error_is_set(&local_err)) {

blockdev.c +1 -0

+
 static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
 {
     BlockJobInfoList **prev = opaque;

qapi-schema.json +0 -0

index a90aeb1..9f1cc8d 100644

qapi-schema.json +7 -0

+
+{ 'type': 'BlockOptionsFile',
+  'data': { 'filename': 'str' } }
+
+{ 'type': 'BlockOptionsRaw',
+  'data': { 'file': 'BlockRef' } }
+

qmp-commands.hx +6 -0

index 362f0e1..fe32ae7 100644
@@ -3043,3 +3043,9 @@ Example:
 <- { "return": {} }
 
 EQMP
+
+    {
+        .name       = "blockdev-add",
+        .args_type  = "options:q",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
+    },

Quoted 1 line

This is just a quick hack to test things

The rest of the series is mostly good to go, but not worth pushing until this is flushed out. But I love where it's headed!

Quoted 28 lines

Signed-off-by Kevin Wolf <[email protected]>

---
 blockdev.c       | 32 ++++++++++++++++++++++++++++++++
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qmp-commands.hx  |  6 ++++++
 3 files changed, 67 insertions(+)

blockdev.c +11 -0

index e71c4ee..e3a4fb8 100644
@@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+
+void qmp_blockdev_add(BlockOptions *options, Error **errp)
+{
+    QString *str;
+    QmpOutputVisitor *ov = qmp_output_visitor_new();
+    QObject *obj;
+    visit_type_BlockOptions(qmp_output_get_visitor(ov),
+                            &options, NULL, errp);
+    obj = qmp_output_get_qobject(ov);

blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1760

Quoted 6 lines

blockdev.c +6 -0

+    str = qobject_to_json_pretty(obj);
+    assert(str != NULL);
+    fprintf(stderr, "\n>>>>>>%s\n<<<<<<\n", qstring_get_str(str));
+    qdict_flatten(obj);
+    str = qobject_to_json_pretty(obj);
+    fprintf(stderr, "\n----->%s\n<-----\n", qstring_get_str(str));

Proof that it's a hack, with embedded debug messages :)

Quoted 10 lines

blockdev.c +10 -0

+
+    Error *local_err = NULL;
+    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qobject_to_qdict(obj), &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    } else {
+        drive_init(opts, IF_NONE);
+    }
+

blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1774

Quoted 4 lines

blockdev.c +4 -0

+    qobject_decref(obj);
+    qmp_output_visitor_cleanup(ov);
+    QDECREF(str);
+}

Rather elegant, now that the conversion between QMP and command line is all hidden behind common visitors, all described by a nice QAPI structure. Which is really what we've been wanting :)

Quoted 9 lines

blockdev.c +1 -0

+
 static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
 {
     BlockJobInfoList **prev = opaque;

qapi-schema.json +0 -0

index a90aeb1..9f1cc8d 100644

qapi-schema.json line 3708

Quoted 7 lines

qapi-schema.json +4 -0

@@ -3702,3 +3702,32 @@
             '*cpuid-input-ecx': 'int',
             'cpuid-register': 'X86CPURegister32',
             'features': 'int' } }
+
+
+{ 'type': 'BlockOptionsBase',
+  'data': { 'driver': 'str', '*read-only': 'bool' } }

Needs docs, of course; but as for the struct itself, it looks okay.

Quoted 14 lines

qapi-schema.json +14 -0

+
+{ 'type': 'BlockOptionsFile',
+  'data': { 'filename': 'str' } }
+
+{ 'type': 'BlockOptionsRaw',
+  'data': { 'file': 'BlockRef' } }
+
+{ 'type': 'BlockOptionsQcow2',
+  'data': { '*lazy-refcounts': 'bool', 'file': 'BlockRef' } }
+
+{ 'union': 'BlockOptions',
+  'base': 'BlockOptionsBase',
+  'discriminator': 'driver',
+  'data': {

qapi-schema.json line 3725

Quoted 3 lines

qapi-schema.json +3 -0

+      'file': 'BlockOptionsFile'
+      'raw': 'BlockOptionsRaw'
+      'qcow2': 'BlockOptionsQcow2'

Missing two ','; I'm surprised we haven't patched the qapi parser to be stricter on that front (especially once that Amos' patches for introspection demand valid JSON). [And I sooooo wish that JSON had followed C99's lead of allowing trailing comma]

qapi-schema.json line 3731

Quoted 6 lines

qapi-schema.json +6 -0

+  } }
+
+{ 'union': 'BlockRef',
+  'discriminator': {},
+  'data': { 'definition': 'BlockOptions'
+            'reference': 'str' } }

Demonstrating both named and anonymous discriminated unions, which serves as a good exercise of the earlier patches.

qapi-schema.json line 3733

Quoted 2 lines

qapi-schema.json +2 -0

+
+{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }

Sounds nice - and seems like it should be easy enough to extend BlockRef and/or BlockOptions to have a way to pass an fd even for backing files, getting to (my) end goal of using fd passing to get SELinux labeling for NFS files out of the box from libvirt.

Should this command return anything? For example, returning a BlockDeviceInfo (with its recursive listing of the entire backing chain) might be useful to check that qemu's view of the world matches what the caller passed in. Particularly important if we are able to let the user choose whether to pass the full chain or to just pass the top-most image and let qemu chase down the metadata in that image to open additional files for the rest of the chain.

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }

Sounds nice - and seems like it should be easy enough to extend BlockRef and/or BlockOptions to have a way to pass an fd even for backing files, getting to (my) end goal of using fd passing to get SELinux labeling for NFS files out of the box from libvirt.

In fact, I think it might be a schema-only patch at this point. :-)

Quoted 7 lines

Should this command return anything? For example, returning a BlockDeviceInfo (with its recursive listing of the entire backing chain) might be useful to check that qemu's view of the world matches what the caller passed in. Particularly important if we are able to let the user choose whether to pass the full chain or to just pass the top-most image and let qemu chase down the metadata in that image to open additional files for the rest of the chain.

Does it matter for you to have this immediately as a return value from blockdev-add, or wouldn't a following query-block on this device achieve the same?

I'm not against adding a return value when it's useful, but I don't think we should just return arbitrary query-* results if we can't think of anything else to return.

The goal of this series is to make QAPI ready to handle mostly unions in a less verbose way so that a future -blockdev command line option can be a direct mapping of the structure used by a blockdev-add QMP command. This series implements everything that I think is needed for this on the QAPI side. The block layer side is still lacking, but good enough that I could actually add a block device using the new command; the problem is here mostly that many -drive options aren't supported yet.

To give you an idea of what the interface looks like in the end, this is what you can pass now in theory (the block layer just won't like the read-only option and error out...):

{ "execute": "blockdev-add",

  "arguments": {
      "options": {
        "driver": "qcow2",
        "read-only": true,
        "lazy-refcounts": true,
        "file": {
            "driver": "file",
            "read-only": false,
            "filename": "/home/kwolf/images/hd.img"
        }
      }
    }

}

Kevin Wolf (11):
qapi-types.py: Split off generate_struct_fields() qapi-types.py: Implement 'base' for unions qapi-visit.py: Split off generate_visit_struct_fields() qapi-visit.py: Implement 'base' for unions
qapi: Add visitor for implicit structs
qapi: Flat unions with arbitrary discriminator
qapi: Add consume argument to qmp_input_get_object()
qapi: Anonymous unions
Implement qdict_flatten()
block: Allow "driver" option on the top level [WIP] block: Implement 'blockdev-add' QMP command

 block.c                     |   7 ++
 blockdev.c                  |  52 ++++++++++---
 include/qapi/qmp/qdict.h    |   1 +
 include/qapi/qmp/qobject.h  |   1 +
 include/qapi/visitor-impl.h |   6 ++
 include/qapi/visitor.h      |   6 ++
 qapi-schema.json            |  29 +++++++
 qapi/qapi-visit-core.c      |  25 +++++++
 qapi/qmp-input-visitor.c    |  47 +++++++++---
 qmp-commands.hx             |   6 ++
 qobject/qdict.c             |  47 ++++++++++++
 qobject/qjson.c             |   2 +
 scripts/qapi-types.py       |  83 ++++++++++++++++++--
 scripts/qapi-visit.py       | 179 ++++++++++++++++++++++++++++++++++++--------
 scripts/qapi.py             |  28 +++++++
 15 files changed, 459 insertions(+), 60 deletions(-)

Quoted 7 lines

The goal of this series is to make QAPI ready to handle mostly unions in a less verbose way so that a future -blockdev command line option can be a direct mapping of the structure used by a blockdev-add QMP command. This series implements everything that I think is needed for this on the QAPI side. The block layer side is still lacking, but good enough that I could actually add a block device using the new command; the problem is here mostly that many -drive options aren't supported yet.

Apologies for arriving late to the party...

Do you think this could be handled by OptsVisitor? See the message part of commit eb7ee2cb. (Directly following commits in the history demonstrate use of the visitor.)

Using OptsVisitor, you'd receive a flat structure, specific to the discriminator value selected. You'd have to transform that flat struct into the qapi object tree that blockdev-add already takes; the benefit is that the usual qemu-opt fishing and basic syntax validation would be covered.

Your requirements are probably more complex than what OptsVisitor can handle, but I'd like you to read the commit message and say, "yes, this is insufficient for me". :)