'blockdev-add' QMP command

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

---
 scripts/qapi-types.py | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

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

scripts/qapi-types.py +6 -1

index e1239e1..9882b95 100644
@@ -157,7 +157,12 @@ typedef enum %(name)s
 
     return lookup_decl + enum_decl
 
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+    name = expr['union']
+    typeinfo = expr['data']
+    base = expr.get('base')
+
     ret = mcgen('''
 struct %(name)s
 {

scripts/qapi-types.py +3 -1

+
+    ret += mcgen('''
 };
 ''')
 

@@ -359,7 +371,7 @@ for expr in exprs:
         ret += generate_type_cleanup_decl(expr['type'])
         fdef.write(generate_type_cleanup(expr['type']) + "\n")
     elif expr.has_key('union'):
-        ret += generate_union(expr['union'], expr['data'])
+        ret += generate_union(expr)
         ret += generate_type_cleanup_decl(expr['union'] + "List")
         fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
         ret += generate_type_cleanup_decl(expr['union'])

Signature · 1 line

1.8.1.4

Quoted 52 lines

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

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

scripts/qapi-types.py +6 -1

index e1239e1..9882b95 100644
@@ -157,7 +157,12 @@ typedef enum %(name)s
 
     return lookup_decl + enum_decl
 
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+    name = expr['union']
+    typeinfo = expr['data']
+    base = expr.get('base')
+
     ret = mcgen('''
 struct %(name)s
 {

scripts/qapi-types.py struct %(name)s · line 188

Quoted 8 lines

scripts/qapi-types.py +5 -0

@@ -176,6 +181,13 @@ struct %(name)s
 
     ret += mcgen('''
     };
+''')
+
+    if base:
+        struct = find_struct(base)
+        ret += generate_struct_fields(struct['data'])

generate_struct_fields() doesn't exist in upstream.

[qemu-upstream]$ grep generate_struct_fields -r *
scripts/qapi-types.py: ret += generate_struct_fields(struct['data'])
[qemu-upstream]$

Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:

Quoted 65 lines · Kevin Wolf

Quoted 52 lines

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

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

scripts/qapi-types.py +6 -1

index e1239e1..9882b95 100644
@@ -157,7 +157,12 @@ typedef enum %(name)s
 
     return lookup_decl + enum_decl
 
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+    name = expr['union']
+    typeinfo = expr['data']
+    base = expr.get('base')
+
     ret = mcgen('''
 struct %(name)s
 {

scripts/qapi-types.py struct %(name)s · line 188

Quoted 8 lines

scripts/qapi-types.py +5 -0

@@ -176,6 +181,13 @@ struct %(name)s
 
     ret += mcgen('''
     };
+''')
+
+    if base:
+        struct = find_struct(base)
+        ret += generate_struct_fields(struct['data'])

generate_struct_fields() doesn't exist in upstream.

Quoted 3 lines

[qemu-upstream]$ grep generate_struct_fields -r *
scripts/qapi-types.py: ret += generate_struct_fields(struct['data'])
[qemu-upstream]$

Yup, something went wrong while applying the series, that patch was simply dropped (and interestingly it didn't result in any conflicts or compile errors). I'll include it in my next pull request.

Quoted 70 lines · On Tue, 27 Aug 2013 17:58:59 +0200 Kevin Wolf <[email protected]> wrote:

Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:

Quoted 68 lines · Kevin Wolf

Quoted 52 lines

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

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

scripts/qapi-types.py +6 -1

index e1239e1..9882b95 100644
@@ -157,7 +157,12 @@ typedef enum %(name)s
 
     return lookup_decl + enum_decl
 
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+    name = expr['union']
+    typeinfo = expr['data']
+    base = expr.get('base')
+
     ret = mcgen('''
 struct %(name)s
 {

scripts/qapi-types.py struct %(name)s · line 188

Quoted 8 lines

scripts/qapi-types.py +5 -0

@@ -176,6 +181,13 @@ struct %(name)s
 
     ret += mcgen('''
     };
+''')
+
+    if base:
+        struct = find_struct(base)
+        ret += generate_struct_fields(struct['data'])

generate_struct_fields() doesn't exist in upstream.

[qemu-upstream]$ grep generate_struct_fields -r *
scripts/qapi-types.py: ret += generate_struct_fields(struct['data'])
[qemu-upstream]$

Quoted 3 lines

Yup, something went wrong while applying the series, that patch was simply dropped (and interestingly it didn't result in any conflicts or compile errors). I'll include it in my next pull request.

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:

Quoted 77 lines

Quoted 73 lines · On Tue, 27 Aug 2013 17:58:59 +0200 Kevin Wolf <[email protected]> wrote:

Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:

Quoted 65 lines · Kevin Wolf

Quoted 52 lines

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

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

scripts/qapi-types.py +6 -1

index e1239e1..9882b95 100644
@@ -157,7 +157,12 @@ typedef enum %(name)s
 
     return lookup_decl + enum_decl
 
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+    name = expr['union']
+    typeinfo = expr['data']
+    base = expr.get('base')
+
     ret = mcgen('''
 struct %(name)s
 {

scripts/qapi-types.py struct %(name)s · line 188

Quoted 8 lines

scripts/qapi-types.py +5 -0

@@ -176,6 +181,13 @@ struct %(name)s
 
     ret += mcgen('''
     };
+''')
+
+    if base:
+        struct = find_struct(base)
+        ret += generate_struct_fields(struct['data'])

generate_struct_fields() doesn't exist in upstream.

Quoted 3 lines

[qemu-upstream]$ grep generate_struct_fields -r *
scripts/qapi-types.py: ret += generate_struct_fields(struct['data'])
[qemu-upstream]$

Yup, something went wrong while applying the series, that patch was simply dropped (and interestingly it didn't result in any conflicts or compile errors). I'll include it in my next pull request.

Quoted 2 lines

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter.

Quoted 6 lines

Quoted 3 lines

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter.

I won't say it's a huge deal, but any downstreamers basing on 1.6 will have a hard time if they backport blockdev-add or any future command that my depend on this.

Quoted 7 lines · Luiz Capitulino

Quoted 6 lines

Quoted 3 lines

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter.

Quoted 3 lines

I won't say it's a huge deal, but any downstreamers basing on 1.6 will have a hard time if they backport blockdev-add or any future command that my depend on this.

Any downstreamers that plans to backport blockdev-add would also backport this as part of their efforts. I don't see that as any different from any other backport effort that includes requiring multiple non-contiguous pre-req patches.

We don't need it on the 1.6
stable tree, and downstream is no worse for the wear.

Quoted 9 lines · On Thu, 29 Aug 2013 11:02:26 -0600 Eric Blake <[email protected]> wrote:

Quoted 7 lines · Luiz Capitulino

Quoted 6 lines

Quoted 3 lines

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter.

Quoted 8 lines

Quoted 3 lines

I won't say it's a huge deal, but any downstreamers basing on 1.6 will have a hard time if they backport blockdev-add or any future command that my depend on this.

Any downstreamers that plans to backport blockdev-add would also backport this as part of their efforts. I don't see that as any different from any other backport effort that includes requiring multiple non-contiguous pre-req patches.

Backport work can be hard. I'd praise people for making it easier for me and would curse people for making it harder for no reason.

Quoted 2 lines

We don't need it on the 1.6
stable tree, and downstream is no worse for the wear.

Why is the feature there in the first place then?

Quoted 60 lines · On Thu, 29 Aug 2013 18:06:50 +0200 Kevin Wolf <[email protected]> wrote:

Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:

Quoted 58 lines

Quoted 45 lines · On Tue, 27 Aug 2013 17:58:59 +0200 Kevin Wolf <[email protected]> wrote:

Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:

Quoted 44 lines · Kevin Wolf

Quoted 31 lines

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.

For example the following schema definition...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would result in this generated C struct:

    struct BlockOptions
    {
        BlockOptionsKind kind;
        union {
            void *data;
            BlockOptionsRaw * raw;
            BlockOptionsQcow2 * qcow2;
        };
        bool read_only;
    };

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

scripts/qapi-types.py struct %(name)s · line 188

Quoted 8 lines

scripts/qapi-types.py +5 -0

@@ -176,6 +181,13 @@ struct %(name)s
 
     ret += mcgen('''
     };
+''')
+
+    if base:
+        struct = find_struct(base)
+        ret += generate_struct_fields(struct['data'])

generate_struct_fields() doesn't exist in upstream.

Quoted 7 lines

Quoted 3 lines

[qemu-upstream]$ grep generate_struct_fields -r *
scripts/qapi-types.py: ret += generate_struct_fields(struct['data'])
[qemu-upstream]$

Yup, something went wrong while applying the series, that patch was simply dropped (and interestingly it didn't result in any conflicts or compile errors). I'll include it in my next pull request.

Strange, it appears on your pull request... But anyway, your series made it into 1.6.0, so I think we'll need the missing patch in 1.6.1 too?

Quoted 2 lines

There's no user in 1.6 (or would we have a build failure) because I didn't merge blockdev-add, so I guess it doesn't matter.

I won't say it's a huge deal, but any downstreamers basing on 1.6 will have a hard time if they backport blockdev-add or any future command that my depend on this.

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

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

This implements the visitor part of base types for unions. Parsed into QMP, this example schema definiton...

    { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }
    { 'type': 'BlockOptionsQcow2, 'data': { 'lazy-refcounts': 'bool' } }

    { 'union': 'BlockOptions',
      'base': 'BlockOptionsBase',
      'data': {
          'raw': 'BlockOptionsRaw'
          'qcow2': 'BlockOptionsQcow2'
      } }

...would describe the following JSON object:

    { "type": "qcow2",
      "read-only": true,
      "data": { "lazy-refcounts": false } }

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

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

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

---
 docs/qapi-code-gen.txt | 58 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 5 deletions(-)

+=== Complex types ===

A complex type is a dictionary containing a single key who's value is a

These can be used when an embedded struct is parsed and members not belonging to the struct may be present in the input (e.g. parsing a flat namespace QMP union, where fields from both the base and one of the alternative types are mixed in the JSON object)

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(+)

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]>

---
 docs/qapi-code-gen.txt | 22 +++++++++++++
 scripts/qapi-types.py  |  6 ++++
 scripts/qapi-visit.py  | 86 ++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 91 insertions(+), 23 deletions(-)

docs/qapi-code-gen.txt +0 -0

index 555ca66..c187fda 100644

docs/qapi-code-gen.txt +3 -0

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

@@ -161,7 +161,9 @@ def generate_union(expr):
 
     name = expr['union']
     typeinfo = expr['data']
+
     base = expr.get('base')
+    discriminator = expr.get('discriminator')
 
     ret = mcgen('''
 struct %(name)s

docs/qapi-code-gen.txt +2 -0

         ret += generate_struct_fields(struct['data'])
+    else:
+        assert not discriminator
 
     ret += mcgen('''
 };

scripts/qapi-visit.py +0 -1

index db6fa44..cd33e44 100644
--- a/scripts/qapi-visit.py

@@ -157,9 +178,17 @@ def generate_visit_union(expr):
     members = expr['data']
 

Quoted 9 lines

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

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

Quoted 7 lines

...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" }

MUCH nicer!

Quoted 17 lines

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

---
 docs/qapi-code-gen.txt | 22 +++++++++++++
 scripts/qapi-types.py  |  6 ++++
 scripts/qapi-visit.py  | 86 ++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 91 insertions(+), 23 deletions(-)

docs/qapi-code-gen.txt +0 -0

index 555ca66..c187fda 100644
@@ -103,6 +103,28 @@ And it looks like this on the wire:
    "data" : { "backing-file": "/some/place/my-image",
               "lazy-refcounts": true } }
 

docs/qapi-code-gen.txt And it looks like this on the wire: · line 109

Quoted 4 lines

docs/qapi-code-gen.txt +4 -0

+
+Flat union types avoid the nesting on the wire. They are used whenever a
+specific field of the base type is declared as the discriminator ('type' is
+then no longer generated). The discriminator must always be a string field.

Since an 'enum' is always sent over the wire as a string, is it appropriate to allow the discriminator to be an enum field instead of a 'str'? But that could be done in a followup patch; your initial usage is just fine with 'str'. Besides, if we allow the discriminator to have 'enum' type, that would imply that we want to guarantee coverage that all of the 'data' members of the union type correspond to the members of the union. On the other hand, that would be extra type safety - if we extend the enum that backs the discriminator, we'd immediately be reminded if we forgot to also extend the union based on that enum. Again, food for thought for a future patch, and not something needed for this one.

docs/qapi-code-gen.txt And it looks like this on the wire: · line 113

Quoted 4 lines

docs/qapi-code-gen.txt +4 -0

+The above example can then be modified as follows:
+
+ { 'type': 'BlockdevCommonOptions',
+   'data': { 'driver': 'str' 'readonly': 'bool' } }

Missing a comma.

Quoted 13 lines

docs/qapi-code-gen.txt +3 -0

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

@@ -161,7 +161,9 @@ def generate_union(expr):
 
     name = expr['union']
     typeinfo = expr['data']
+
     base = expr.get('base')
+    discriminator = expr.get('discriminator')
 
     ret = mcgen('''
 struct %(name)s

@@ -185,7 +187,11 @@ struct %(name)s
 

docs/qapi-code-gen.txt struct %(name)s · line 191

Quoted 4 lines

docs/qapi-code-gen.txt +2 -0

     if base:
         struct = find_struct(base)
+        if discriminator:
+            del struct['data'][discriminator]

I asked before, but didn't get an answer; my question may just show my unfamiliarity with python. Is this modifying the original 'struct', such that other uses of the struct after this point will no longer contain the discriminator key? Or is it only modifying a copy of 'struct', with the original left intact? But based on the rest of your patch...

I think my findings are easy fixes; so I'm okay if you fix them and then add:

Reviewed-by Eric Blake <[email protected]>

Quoted 11 lines

scripts/qapi-types.py struct %(name)s · line 191

Quoted 4 lines

if base:
struct = find_struct(base)

scripts/qapi-types.py +2 -0

+        if discriminator:
+            del struct['data'][discriminator]

I asked before, but didn't get an answer; my question may just show my unfamiliarity with python. Is this modifying the original 'struct', such that other uses of the struct after this point will no longer contain the discriminator key? Or is it only modifying a copy of 'struct', with the original left intact? But based on the rest of your patch...

Sorry, this is in fact my own unfamiliarity with Python, combined with failure to fix all cases when you pointed it out. The only reason I didn't reply to that part of your review was that I thought it would be obvious when I send a fixed version. Well, except if I don't.

I've changed this hunk now to match the other one:

(quoted diff) +0 -0

@@ -184,8 +186,13 @@ struct %(name)s
 ''')
 

Quoted 4 lines

I think my findings are easy fixes; so I'm okay if you fix them and then add:

Reviewed-by Eric Blake <[email protected]>

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]>

Reviewed-by Eric Blake <[email protected]>

---
 docs/qapi-code-gen.txt | 22 ++++++++++++
 scripts/qapi-types.py  | 11 ++++--
 scripts/qapi-visit.py  | 90 +++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 98 insertions(+), 25 deletions(-)

v2:
- make check failed because some of the nested struct code didn't correctly put separators between the field names, ending up with foo.barbaz instead of foo.bar.baz for fields, and foo_barbar instead of foo_bar_baz for functions. Fixed that.

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(-)

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

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

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]>

---

+++ b/docs/qapi-code-gen.txt

docs/qapi-code-gen.txt +0 -0

@@ -125,6 +125,31 @@ Resulting in this JSON object:
    "lazy-refcounts": true }
 
 

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(-)

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

---
 include/qemu/option.h |  1 +
 util/qemu-option.c    | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

In QMP, we want to use dashes instead of underscores in QMP argument names, and use nested options for throttling. Convert them for -drive before calling into the code that will be shared between -drive and blockdev-add.

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

---
 blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

Quoted 10 lines

In QMP, we want to use dashes instead of underscores in QMP argument names, and use nested options for throttling. Convert them for -drive before calling into the code that will be shared between -drive and blockdev-add.

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

---
 blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

This patch will probably conflict with Benoît's work on leaky bucket throttling; can the two of you decide which one should go in first? Are we trying to target both this series and leaky bucket throttling for 1.6?

blockdev.c DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) · line 489

Quoted 6 lines

blockdev.c +1 -1

@@ -485,17 +486,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     /* disk I/O throttling */
     io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
-                           qemu_opt_get_number(opts, "bps", 0);
+        qemu_opt_get_number(opts, "throttling.bps-total", 0);

I like the rename to bps-total, to make it more obvious why it is provided in addition to bps-{read,write}.

blockdev.c DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) · line 493

Quoted 6 lines

blockdev.c +2 -2

     io_limits.bps[BLOCK_IO_LIMIT_READ]   =
-                           qemu_opt_get_number(opts, "bps_rd", 0);
+        qemu_opt_get_number(opts, "throttling.bps-read", 0);
     io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
-                           qemu_opt_get_number(opts, "bps_wr", 0);
+        qemu_opt_get_number(opts, "throttling.bps-write", 0);

Thank you for spelling out the names; QMP doesn't need the abbreviations that the command line has.

blockdev.c err: · line 746

Quoted 6 lines

blockdev.c +6 -0

+DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+{
+    /* Change legacy command line options into QMP ones */
+    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
+    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
+    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");

Of course, the intent here is to preserve back-compat (the old spelling continues to work, even if it differs from the QMP spelling). But will this patch also allow the command line to learn support for the new spellings? If so, is that worth mentioning as an intentional side effect in the commit message?

Quoted 3 lines

This patch will probably conflict with Benoît's work on leaky bucket throttling; can the two of you decide which one should go in first? Are we trying to target both this series and leaky bucket throttling for 1.6?

If you complete the review before I leave today, I might still send a pull request, but as I'm going to disable blockdev-add and the new options once again for 1.6, it doesn't really matter that much.

Benoît's series is for 1.7 as well, if I understood Stefan correctly. He

said he was going to merge a bug fix part of it for 1.6 and leave the rest for 1.7. (I haven't been following the throttling series myself, that's why I can't comment in much more detail.)

Quoted 5 lines

Quoted 4 lines

This patch will probably conflict with Benoît's work on leaky bucket throttling; can the two of you decide which one should go in first? Are we trying to target both this series and leaky bucket throttling for 1.6?

Quoted 3 lines

If you complete the review before I leave today, I might still send a pull request, but as I'm going to disable blockdev-add and the new options once again for 1.6, it doesn't really matter that much.

In other words, just as it was in 1.5, the new parser is cool enough to implement the framework now to ease backport efforts, but untested enough that we'd rather defer use of that framework until after we are back out of freeze.

Quoted 1 line

Benoît's series is for 1.7 as well, if I understood Stefan correctly. He

Even though the latest subject line requests for-1.6?
https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg04005.html

Quoted 3 lines

said he was going to merge a bug fix part of it for 1.6 and leave the rest for 1.7. (I haven't been following the throttling series myself, that's why I can't comment in much more detail.)

I guess I also need to comment on that series - we're late enough that bug fixes are okay, but new options are risky; and the tail end of that series adds new options to throttling as part of switching to a new algorithm.

Quoted 4 lines

Quoted 3 lines

If you complete the review before I leave today, I might still send a pull request, but as I'm going to disable blockdev-add and the new options once again for 1.6, it doesn't really matter that much.

Quoted 4 lines

In other words, just as it was in 1.5, the new parser is cool enough to implement the framework now to ease backport efforts, but untested enough that we'd rather defer use of that framework until after we are back out of freeze.

Right. Before actually committing to the interface, I'd like you to have some real libvirt code running on it. I assume that's doable in the 1.7 time frame, right?

This patch will probably conflict with Benoît's work on leaky bucket throttling; can the two of you decide which one should go in first? Are we trying to target both this series and leaky bucket throttling for 1.6?

Quoted 3 lines

This patch will probably conflict with Benoît's work on leaky bucket throttling; can the two of you decide which one should go in first? Are we trying to target both this series and leaky bucket throttling for 1.6?

I will to rebase my serie on top of this.

However if anyone has suggestions for the names of the new options the leaky bucket serie add it would probably avoid an extra code review.

This is what QMP wants to use. The options haven't been enabled in any release yet, so we're still free to change them.

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

---
 block/qcow2.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Option name cleanup before it becomes a QMP API.

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

---
 blockdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

The old 'cache' option really encodes three different boolean flags into a cache mode name, without providing all combinations. Make them three separate options instead and translate the old option to the new ones for drive_init().

The specific boolean options take precedence if the old cache option is specified as well, so the following options are equivalent:

(quoted diff) +0 -2

-drive file=x,cache=none,cache.no-flush=true
-drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true

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

---
 blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 10 deletions(-)

+++ b/blockdev.c

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". This operation is applied recursively for nested QDicts.

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

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

qobject/qdict.c +12 -0

+        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);
+            delete = true;
+        }
+
+        if (qobject_type(value) == QTYPE_QDICT) {

Quoted 11 lines

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". This operation is applied recursively for nested QDicts.

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

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

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

Quoted 3 lines

qobject/qdict.c +3 -0

+    while (entry != NULL) {
+
+        next = qdict_next(qdict, entry);

next points to a position in the unmodified qdict...

Quoted 12 lines

qobject/qdict.c +12 -0

+        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);
+            delete = true;
+        }
+
+        if (qobject_type(value) == QTYPE_QDICT) {

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

Quoted 10 lines

qobject/qdict.c +10 -0

+            qdict_do_flatten(qobject_to_qdict(value), target,
+                             new_key ? new_key : entry->key);
+            delete = true;
+        }
+
+        if (delete) {
+            qdict_del(qdict, entry->key);
+
+            /* Restart loop after modifying the iterated QDict */
+            entry = qdict_first(qdict);

...now entry points to the head of the modified qdict, since the modification may have re-arranged where we would iterate next...

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

Quoted 3 lines

qobject/qdict.c +3 -0

+        }
+
+        entry = next;

...oops, we just undid that, and pointed it back to the old qdict iteration location. I think you're missing a continue statement inside 'if (delete)'.

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

---
 blockdev.c       |  45 +++++++++
 qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |  26 +++++
 3 files changed, 364 insertions(+)

blockdev.c +0 -0

@@ -1805,6 +1807,49 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 

qapi-schema.json +2 -0

+++ b/qapi-schema.json

@@ -3761,3 +3761,296 @@
 ##
 { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
   'returns': ['RxFilterInfo'] }
+

qapi-schema.json +7 -0

+
+##
+# @BlockdevOptionsBase
+#
+# Options that are available for all block devices, independent of the block
+# driver.
+#

qapi-schema.json +17 -0

+#
+# Driver specific block device options for the NBD protocol. Either path or host
+# must be specified.
+#
+# @path:        path to a unix domain socket
+# @host:        host name for a TCP connection
+# @port:        port number for a TCP connection (default: 10809)
+# @export:      NBD export name
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsNBD',
+  'data': { '*path': 'str', '*host': 'str', '*port': 'int', '*export': 'str' } }
+
+##
+# @BlockdevOptionsSSH
+#

qapi-schema.json +10 -0

+#
+# Driver specific block device options for qcow2.
+#
+# @file:        reference to or definition of the data source block device
+#
+# @backing:     reference to or definition of the backing file block device
+#               (if missing, taken from the image file content)
+#
+# @lazy-refcounts: whether to enable the lazy refcounts feature (default is
+#                  taken from the image file)

qapi-schema.json +11 -0

+
+##
+# @BlockdevOptions
+#
+# Options for creating a block device.
+#
+# Since: 1.6
+##
+{ 'union': 'BlockdevOptions',
+  'base': 'BlockdevOptionsBase',
+  'discriminator': 'driver',

qapi-schema.json +9 -0

+# TODO rbd: Wait for structured options
+# TODO sheepdog: Wait for structured options
+# TODO ssh: Should take InetSocketAddress for 'host'?
+      'vvfat':      'BlockdevOptionsVVFAT',
+
+      'bochs':      'BlockdevOptionsGenericFormat',
+      'cloop':      'BlockdevOptionsGenericFormat',
+      'cow':        'BlockdevOptionsGenericCOWFormat',
+      'dmg':        'BlockdevOptionsGenericFormat',

qapi-schema.json +4 -0

+
+##
+# @BlockdevRef
+#

Overall, I like where this is headed, but I'm not quite sure it is ready for commit as-is; looking forward to v2. Given my positive review on the rest of the series, I think you could get away with a pull request on the front half of the series and only respinning this patch, instead of needing (re-)review of the entire series.

Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:

Quoted 5 lines

Overall, I like where this is headed, but I'm not quite sure it is ready for commit as-is; looking forward to v2. Given my positive review on the rest of the series, I think you could get away with a pull request on the front half of the series and only respinning this patch, instead of needing (re-)review of the entire series.

Yup, that's exactly what I'm going to do now. I had considered merging the whole series, but I think you're right that committing it now might be a bit early. My main concern were potential conflicts in the front "half" (17/18) of the series when I'd wait too long with committing.

Quoted 7 lines · On Fri, 26 Jul 2013 20:14:06 +0200 Kevin Wolf <[email protected]> wrote:

Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:

Quoted 5 lines

Overall, I like where this is headed, but I'm not quite sure it is ready for commit as-is; looking forward to v2. Given my positive review on the rest of the series, I think you could get away with a pull request on the front half of the series and only respinning this patch, instead of needing (re-)review of the entire series.

Quoted 4 lines

Yup, that's exactly what I'm going to do now. I had considered merging the whole series, but I think you're right that committing it now might be a bit early. My main concern were potential conflicts in the front "half" (17/18) of the series when I'd wait too long with committing.

Just to double check I got it as I'm still catching up with email after vacation: you did exactly what was suggested by Eric, you merged 1-17 and will post the blockdev-add command for 1.7?

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

---
  blockdev.c       |  45 +++++++++
  qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  qmp-commands.hx  |  26 +++++
  3 files changed, 364 insertions(+)

blockdev.c +45 -0

index ef55b1a..214173b 100644
@@ -38,6 +38,8 @@
  #include "qemu/option.h"
  #include "qemu/config-file.h"
  #include "qapi/qmp/types.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
  #include "sysemu/sysemu.h"
  #include "block/block_int.h"
  #include "qmp-commands.h"

@@ -1805,6 +1807,49 @@ void qmp_block_job_complete(const char *device, Error **errp)
      block_job_complete(job, errp);
  }
 
+void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
+{
+    QmpOutputVisitor *ov = qmp_output_visitor_new();
+    QObject *obj;
+    QDict *qdict;
+
+    if (!options->has_id) {
+        error_setg(errp, "Block device needs an ID");
+        return;
+    }
+
+    /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
+     * cache.direct=off instead of silently switching to aio=threads, except if
+     * called from drive_init.
+     *
+     * For now, simply forbidding the combination for all drivers will do. */
+    if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
+        if (!options->has_cache || !options->cache->direct) {
+            error_setg(errp, "aio=native requires cache.direct=true");
+            return;
+        }
+    }
+
+    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
+                               &options, NULL, errp);
+    obj = qmp_output_get_qobject(ov);
+    qdict = qobject_to_qdict(obj);
+
+    qdict_flatten(qdict);
+
+    Error *local_err = NULL;
+    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    } else {
+        blockdev_init(opts, IF_NONE);
+    }
+
+    qobject_decref(obj);
+    qmp_output_visitor_cleanup(ov);
+}
+
  static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
  {
      BlockJobInfoList **prev = opaque;

qapi-schema.json +155 -0

index 592bb9c..bfe6d32 100644
@@ -3761,3 +3761,296 @@
  ##
  { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
    'returns': ['RxFilterInfo'] }
+
+
+##
+# @BlockdevDiscardOptions
+#
+# Determines how to handle discard requests.
+#
+# @ignore:      Ignore the request
+# @unmap:       Forward as an unmap request
+#
+# Since: 1.6
+##
+{ 'enum': 'BlockdevDiscardOptions',
+  'data': [ 'ignore', 'unmap' ] }
+
+##
+# @BlockdevDiscardOptions
+#
+# Selects the AIO backend to handle I/O requests
+#
+# @threads:     Use qemu's thread pool
+# @native:      Use native AIO backend (only Linux and Windows)
+#
+# Since: 1.6
+##
+{ 'enum': 'BlockdevAIOOptions',
+  'data': [ 'threads', 'native' ] }
+
+##
+# @BlockdevCacheOptions
+#
+# Includes cache-related options for block devices
+#
+# @writeback:   enables writeback mode for any caches (default: true)
+# @direct:      enables use of O_DIRECT (bypass the host page cache;
+#               default: false)
+# @no-flush:    ignore any flush requests for the device (default: false)
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevCacheOptions',
+  'data': { '*writeback': 'bool',
+            '*direct': 'bool',
+            '*no-flush': 'bool' } }
+
+##
+# @BlockdevThrottlingOptions
+#
+# Includes block device options related to I/O throttling. Leaving an option out
+# means the same as assigning 0 and applies no throttling.
+#
+# @bps-total:   limit total bytes per second
+# @bps-read:    limit read bytes per second
+# @bps-write:   limit written bytes per second
+# @iops-total:  limit total I/O operations per second
+# @iops-read:   limit read operations per second
+# @iops-write:  limit write operations per second
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevThrottlingOptions',
+  'data': { '*bps-total': 'int',
+            '*bps-read': 'int',
+            '*bps-write': 'int',
+            '*iops-total': 'int',
+            '*iops-read': 'int',
+            '*iops-write': 'int' } }
+
+##
+# @BlockdevOptionsBase
+#
+# Options that are available for all block devices, independent of the block
+# driver.
+#
+# @driver:      block driver name
+# @id:          id by which the new block device can be referred to
+# @discard:     discard-related options
+# @cache:       cache-related options
+# @aio:         AIO backend (default: threads)
+# @rerror:      how to handle read errors on the device (default: report)
+# @werror:      how to handle write errors on the device (default: enospc)
+# @throttling:  I/O throttling related options
+# @read-only:   whether the block device should be read-only (default: false)
+# @copy-on-read: whether to enable copy on read for the device (default: false)
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsBase',
+  'data': { 'driver': 'str',
+            '*id': 'str',
+            '*discard': 'BlockdevDiscardOptions',
+            '*cache': 'BlockdevCacheOptions',
+            '*aio':  'BlockdevAIOOptions',
+            '*rerror': 'BlockdevOnError',
+            '*werror': 'BlockdevOnError',
+            '*throttling': 'BlockdevThrottlingOptions',
+            '*read-only': 'bool',
+            '*copy-on-read': 'bool' } }
+
+##
+# @BlockdevOptionsFile
+#
+# Driver specific block device options for the file backend and similar
+# protocols.
+#
+# @filename:    path to the image file
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsFile',
+  'data': { 'filename': 'str' } }
+
+##
+# @BlockdevOptionsFile
+#
+# Driver specific block device options for the NBD protocol. Either path or host
+# must be specified.
+#
+# @path:        path to a unix domain socket
+# @host:        host name for a TCP connection
+# @port:        port number for a TCP connection (default: 10809)
+# @export:      NBD export name
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsNBD',
+  'data': { '*path': 'str', '*host': 'str', '*port': 'int', '*export': 'str' } }
+
+##
+# @BlockdevOptionsSSH
+#
+# Driver specific block device options for the SSH protocol.
+#
+# @host:        host name for a TCP connection
+# @port:        port number for a TCP connection (default: 22)
+# @path:        path to the image on the remote host
+# @user:        SSH user name
+#
+# @host-key-check:
+# TODO Should this be split into multiple fields for QMP?
+# TODO Driver takes host_key_check with underscores currently
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsSSH',
+  'data': { 'host': 'str', '*port': 'int', 'path': 'str', '*user': 'str',
+            '*host-key-check': 'str' } }
+
+##
+# @BlockdevOptionsVVFAT
+#
+# Driver specific block device options for the vvfat protocol.
+#
+# @dir:         directory to be exported as FAT image
+# @fat-type:    FAT type: 12, 16 or 32

qapi-schema.json +128 -0

+##
+# @BlockdevOptionsGenericFormat
+#
+# Driver specific block device options for image format that have no option
+# besides their data source.
+#
+# @file:        reference to or definition of the data source block device
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsGenericFormat',
+  'data': { 'file': 'BlockdevRef' } }
+
+##
+# @BlockdevOptionsGenericCOWFormat
+#
+# Driver specific block device options for image format that have no option
+# besides their data source and an optional backing file.
+#
+# @file:        reference to or definition of the data source block device
+# @backing:     reference to or definition of the backing file block device
+#               (if missing, taken from the image file content)
+#
+# Since: 1.6
+##
+# TODO Add inheritance and base on BlockdevOptionsGenericFormat
+{ 'type': 'BlockdevOptionsGenericCOWFormat',
+  'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } }
+
+##
+# @BlockdevOptionsGenericCOWFormat
+#
+# Driver specific block device options for qcow2.
+#
+# @file:        reference to or definition of the data source block device
+#
+# @backing:     reference to or definition of the backing file block device
+#               (if missing, taken from the image file content)
+#
+# @lazy-refcounts: whether to enable the lazy refcounts feature (default is
+#                  taken from the image file)
+#
+# @pass-discard-request: whether discard requests to the qcow2 device should
+#                        be forwarded to the data source
+#
+# @pass-discard-snapshot: whether discard requests for the data source should
+#                         be issued when a snapshot operation (e.g. deleting
+#                         a snapshot) frees clusters in the qcow2 file
+#
+# @pass-discard-other: whether discard requests for the data source should be
+#                      issued on other occasions where a cluster gets freed
+#
+# Since: 1.6
+##
+# TODO Add inheritance and base on BlockdevOptionsGenericCOWFormat
+{ 'type': 'BlockdevOptionsQcow2',
+  'data': { 'file': 'BlockdevRef',
+            '*backing': 'BlockdevRef',
+            '*lazy-refcounts': 'bool',
+            '*pass-discard-request': 'bool',
+            '*pass-discard-snapshot': 'bool',
+            '*pass-discard-other': 'bool' } }
+
+##
+# @BlockdevOptions
+#
+# Options for creating a block device.
+#
+# Since: 1.6
+##
+{ 'union': 'BlockdevOptions',
+  'base': 'BlockdevOptionsBase',
+  'discriminator': 'driver',
+  'data': {
+      'file':       'BlockdevOptionsFile',
+      'http':       'BlockdevOptionsFile',
+      'https':      'BlockdevOptionsFile',
+      'ftp':        'BlockdevOptionsFile',
+      'ftps':       'BlockdevOptionsFile',
+      'tftp':       'BlockdevOptionsFile',
+# TODO gluster: Wait for structured options
+# TODO iscsi: Wait for structured options
+# TODO nbd: Should take InetSocketAddress for 'host'?
+# TODO rbd: Wait for structured options
+# TODO sheepdog: Wait for structured options
+# TODO ssh: Should take InetSocketAddress for 'host'?
+      'vvfat':      'BlockdevOptionsVVFAT',
+
+      'bochs':      'BlockdevOptionsGenericFormat',
+      'cloop':      'BlockdevOptionsGenericFormat',
+      'cow':        'BlockdevOptionsGenericCOWFormat',
+      'dmg':        'BlockdevOptionsGenericFormat',
+      'parallels':  'BlockdevOptionsGenericFormat',
+      'qcow':       'BlockdevOptionsGenericCOWFormat',
+      'qcow2':      'BlockdevOptionsQcow2',
+      'qed':        'BlockdevOptionsGenericCOWFormat',
+      'raw':        'BlockdevOptionsGenericFormat',
+      'vdi':        'BlockdevOptionsGenericFormat',
+      'vhdx':       'BlockdevOptionsGenericFormat',
+      'vmdk':       'BlockdevOptionsGenericCOWFormat',
+      'vpc':        'BlockdevOptionsGenericFormat'
+  } }
+
+##
+# @BlockdevRef
+#
+# Reference to a block device.
+#
+# @definition:      defines a new block device inline
+# @reference:       references the ID of an existing block device
+#
+# Since: 1.6
+##
+{ 'union': 'BlockdevRef',
+  'discriminator': {},
+  'data': { 'definition': 'BlockdevOptions'
+            'reference': 'str' } }
+
+##
+# @blockdev-add:
+#
+# Creates a new block device.
+#
+# @options: block device options for the new device
+#
+# Since: 1.6
+##
+{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }

qmp-commands.hx +26 -0

index 65a9e26..96b0956 100644
@@ -3113,3 +3113,29 @@ Example:
     }
 
  EQMP
+
+    {
+        .name       = "blockdev-add",
+        .args_type  = "options:q",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
+    },
+
+SQMP
+blockdev-add
+------------
+
+Add a block device.
+
+Arguments:
+
+- "options": block driver options
+
+Example:
+
+-> { "execute": "blockdev-add",
+    "arguments": { "options" : { "driver": "qcow2",
+                                 "file": { "driver": "file",
+                                           "filename": "test.qcow2" } } } }
+<- { "return": {} }
+
+EQMP