'blockdev-add' QMP command
cover 'blockdev-add' QMP command
Kevin Wolf (18):
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
docs: Document QAPI union types
qapi: Add visitor for implicit structs
qapi: Flat unions with arbitrary discriminator
qapi: Add consume argument to qmp_input_get_object() qapi.py: Maintain a list of union types
qapi: Anonymous unions
block: Allow "driver" option on the top level
QemuOpts: Add qemu_opt_unset()
blockdev: Rename I/O throttling options for QMP
qcow2: Use dashes instead of underscores in options
blockdev: Rename 'readonly' option to 'read-only'
blockdev: Split up 'cache' option
Implement qdict_flatten()
blockdev: 'blockdev-add' QMP command
block.c | 7 ++ block/qcow2.h | 8 +- blockdev.c | 184 ++++++++++++++++++++++------ docs/qapi-code-gen.txt | 105 +++++++++++++++- include/qapi/qmp/qdict.h | 1 + include/qapi/qmp/qobject.h | 1 + include/qapi/visitor-impl.h | 6 + include/qapi/visitor.h | 6 + include/qemu/option.h | 1 + qapi-schema.json | 293 ++++++++++++++++++++++++++++++++++++++++++++ qapi/qapi-visit-core.c | 25 ++++ qapi/qmp-input-visitor.c | 47 +++++-- qmp-commands.hx | 26 ++++ qobject/qdict.c | 50 ++++++++ qobject/qjson.c | 2 + scripts/qapi-types.py | 83 +++++++++++-- scripts/qapi-visit.py | 179 ++++++++++++++++++++++----- scripts/qapi.py | 28 +++++ util/qemu-option.c | 14 +++ 19 files changed, 970 insertions(+), 96 deletions(-)
Signature · 1 line
1.8.1.4
PATCH 1/18 qapi-types.py: Split off generate_struct_fields()
--- scripts/qapi-types.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
scripts/qapi-types.py +13 -6
@@ -57,12 +57,8 @@ typedef struct %(name)sList ''', name=name) -def generate_struct(structname, fieldname, members): - ret = mcgen(''' -struct %(name)s -{ -''', - name=structname) +def generate_struct_fields(members): + ret = '' for argname, argentry, optional, structured in parse_args(members): if optional:
@@ -80,6 +76,17 @@ struct %(name)s ''', c_type=c_type(argentry), c_name=c_var(argname)) + return ret + +def generate_struct(structname, fieldname, members): + ret = mcgen(''' +struct %(name)s +{ +''', + name=structname) + + ret += generate_struct_fields(members) + if len(fieldname): fieldname = " " + fieldname ret += mcgen('''
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 01/18] qapi-types.py: Split off generate_struct_fields()
Quoted 4 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi-types.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 2/18 qapi-types.py: Implement 'base' for unions
In reply to Kevin WolfThe 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;
};--- scripts/qapi-types.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
scripts/qapi-types.py +14 -2
@@ -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 {
@@ -176,6 +181,13 @@ struct %(name)s ret += mcgen(''' }; +''') + + if base: + struct = find_struct(base) + ret += generate_struct_fields(struct['data']) + + 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
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Quoted 28 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.
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; };
Quoted 4 lines
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi-types.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
In reply to Kevin WolfQuoted 52 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.
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@@ -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 17 lines
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
Signature · 1 line
Amos.
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
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@@ -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.
If you need it before that, you can apply the patch manually from the mailing list, its subject line is:
qapi-types.py: Split off generate_struct_fields()
Kevin
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Quoted 66 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@@ -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 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?
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:
Quoted 73 lines
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@@ -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 6 lines
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?
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.
Kevin
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Quoted 78 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 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@@ -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 5 lines
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.
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.
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Quoted 10 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.
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
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?
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
Quoted 21 lines · Luiz Capitulino
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.
Quoted 8 lines
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?
Guess it is for a better way to enable Fam Zheng's point in time
block snapshot, which need to create a temporary BDS. This series provide a more general way to manage the BDS life cycle?
I just noticed this series, it seems an example as a block layer
interface design, looking forward to V2.
Signature · 3 lines
Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
In reply to Luiz CapitulinoAm 29.08.2013 um 18:33 hat Luiz Capitulino geschrieben:
Quoted 63 lines
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 6 lines
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.
About as hard as with any other dependency. But I won't oppose any request to include it in a stable release, adding a missing function that is called is clearly a bug fix.
Kevin
PATCH 3/18 qapi-visit.py: Split off generate_visit_struct_fields()
In reply to Kevin Wolf--- scripts/qapi-visit.py | 62 ++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 28 deletions(-)
scripts/qapi-visit.py +34 -28
@@ -17,34 +17,9 @@ import os import getopt import errno -def generate_visit_struct_body(field_prefix, name, members): - ret = mcgen(''' -if (!error_is_set(errp)) { -''') - push_indent() - - if len(field_prefix): - field_prefix = field_prefix + "." - ret += mcgen(''' -Error **errp = &err; /* from outer scope */ -Error *err = NULL; -visit_start_struct(m, NULL, "", "%(name)s", 0, &err); -''', - name=name) - else: - ret += mcgen(''' -Error *err = NULL; -visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); -''', - name=name) +def generate_visit_struct_fields(field_prefix, members): + ret = '' - ret += mcgen(''' -if (!err) { - if (!obj || *obj) { -''') - - push_indent() - push_indent() for argname, argentry, optional, structured in parse_args(members): if optional: ret += mcgen('''
@@ -72,9 +47,40 @@ visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", visit_end_optional(m, &err); ''') - pop_indent() + return ret + + +def generate_visit_struct_body(field_prefix, name, members): + ret = mcgen(''' +if (!error_is_set(errp)) { +''') + push_indent() + + if len(field_prefix): + field_prefix = field_prefix + "." + ret += mcgen(''' +Error **errp = &err; /* from outer scope */ +Error *err = NULL; +visit_start_struct(m, NULL, "", "%(name)s", 0, &err); +''', + name=name) + else: + ret += mcgen(''' +Error *err = NULL; +visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); +''', + name=name) + ret += mcgen(''' +if (!err) { + if (!obj || *obj) { +''') + push_indent() + push_indent() + ret += generate_visit_struct_fields(field_prefix, members) + pop_indent() + ret += mcgen(''' error_propagate(errp, err); err = NULL; }
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 03/18] qapi-visit.py: Split off generate_visit_struct_fields()
Quoted 5 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi-visit.py | 62 ++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 28 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 4/18 qapi-visit.py: Implement 'base' for unions
In reply to Kevin WolfThis 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 } }--- scripts/qapi-visit.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
scripts/qapi-visit.py +25 -5
@@ -151,7 +151,13 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e ''', name=name) -def generate_visit_union(name, members): +def generate_visit_union(expr): + + name = expr['union'] + members = expr['data'] + + base = expr.get('base') + ret = generate_visit_enum('%sKind' % name, members.keys()) ret += mcgen('''
@@ -164,14 +170,28 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); if (!err) { if (obj && *obj) { - visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err); - if (!err) { - switch ((*obj)->kind) { ''', name=name) + push_indent() push_indent() + push_indent() + + if base: + struct = find_struct(base) + push_indent() + ret += generate_visit_struct_fields("", struct['data']) + pop_indent() + + pop_indent() + ret += mcgen(''' + visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err); + if (!err) { + switch ((*obj)->kind) { +''', + name=name) + for key in members: ret += mcgen(''' case %(abbrev)s_KIND_%(enum)s:
@@ -368,7 +388,7 @@ for expr in exprs: ret = generate_declaration(expr['type'], expr['data']) fdecl.write(ret) elif expr.has_key('union'): - ret = generate_visit_union(expr['union'], expr['data']) + ret = generate_visit_union(expr) ret += generate_visit_list(expr['union'], expr['data']) fdef.write(ret)
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions
Quoted 2 lines · Kevin Wolf
This implements the visitor part of base types for unions. Parsed into QMP, this example schema definiton...
s/definiton/definition/
Quoted 13 lines
{ '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:
Quoted 8 lines
{ "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(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 5/18 docs: Document QAPI union types
In reply to Kevin Wolf--- docs/qapi-code-gen.txt | 58 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 5 deletions(-)
docs/qapi-code-gen.txt +53 -5
@@ -34,7 +34,13 @@ OrderedDicts so that ordering is preserved. There are two basic syntaxes used, type definitions and command definitions. The first syntax defines a type and is represented by a dictionary. There are -two kinds of types that are supported: complex user-defined types, and enums. +three kinds of user-defined types that are supported: complex types, +enumeration types and union types. + +Generally speaking, types definitions should always use CamelCase for the type +names. Command names should be all lower case with words separated by a hyphen. + +=== Complex types === A complex type is a dictionary containing a single key who's value is a dictionary. This corresponds to a struct in C or an Object in JSON. An
@@ -47,13 +53,57 @@ The use of '*' as a prefix to the name means the member is optional. Optional members should always be added to the end of the dictionary to preserve backwards compatibility. +=== Enumeration types === + An enumeration type is a dictionary containing a single key who's value is a list of strings. An example enumeration is: { 'enum': 'MyEnum', 'data': [ 'value1', 'value2', 'value3' ] } -Generally speaking, complex types and enums should always use CamelCase for -the type names. +=== Union types === + +Union types are used to let the user choose between several different data +types. A union type is defined using a dictionary as explained in the +following paragraphs. + + +A simple union type defines a mapping from discriminator values to data types +like in this example: + + { 'type': 'FileOptions', 'data': { 'filename': 'str' } } + { 'type': 'Qcow2Options', + 'data': { 'backing-file': 'str', 'lazy-refcounts': 'bool' } } + + { 'union': 'BlockdevOptions', + 'data': { 'file': 'FileOptions', + 'qcow2': 'Qcow2Options' } } + +In the QMP wire format, a simple union is represented by a dictionary that +contains the 'type' field as a discriminator, and a 'data' field that is of the +specified data type corresponding to the discriminator value: + + { "type": "qcow2", "data" : { "backing-file": "/some/place/my-image", + "lazy-refcounts": true } } + + +A union definition can specify a complex type as its base. In this case, the +fields of the complex type are included as top-level fields of the union +dictionary in the QMP wire format. An example definition is: + + { 'type': 'BlockdevCommonOptions', 'data': { 'readonly': 'bool' } } + { 'union': 'BlockdevOptions', + 'base': 'BlockdevCommonOptions', + 'data': { 'raw': 'RawOptions', + 'qcow2': 'Qcow2Options' } } + +And it looks like this on the wire: + + { "type": "qcow2", + "readonly": false, + "data" : { "backing-file": "/some/place/my-image", + "lazy-refcounts": true } } + +=== Commands === Commands are defined by using a list containing three members. The first member is the command name, the second member is a dictionary containing
@@ -65,8 +115,6 @@ An example command is: 'data': { 'arg1': 'str', '*arg2': 'str' }, 'returns': 'str' } -Command names should be all lower case with words separated by a hyphen. - == Code generation ==
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types
Quoted 8 lines · Kevin Wolf
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
pre-existing, but as long as you are touching this, fix the grammar:
s/who's/whose/
docs/qapi-code-gen.txt The use of '*' as a prefix to the name means the member is optional. Optional · line 58
Quoted 8 lines
dictionary. This corresponds to a struct in C or an Object in JSON. An
docs/qapi-code-gen.txt +2
-0@@ -47,13 +53,57 @@ The use of '*' as a prefix to the name means the member is optional. Optional members should always be added to the end of the dictionary to preserve backwards compatibility. +=== Enumeration types === + An enumeration type is a dictionary containing a single key who's value is a
s/who's/whose/
With those fixes,
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 6/18 qapi: Add visitor for implicit structs
In reply to Kevin WolfThese 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)
--- 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(+)
include/qapi/visitor-impl.h +4 -0
@@ -22,6 +22,10 @@ struct Visitor const char *name, size_t size, Error **errp);void (*end_struct)(Visitor *v,Error **errp); +void (*start_implicit_struct)(Visitor *v,void **obj,size_t size, +Error **errp); +void (*end_implicit_struct)(Visitor *v,Error **errp); +void (*start_list)(Visitor *v,const char *name,Error **errp); GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);void (*end_list)(Visitor *v,Error **errp);
include/qapi/visitor.h +3 -0
@@ -33,6 +33,9 @@ void visit_end_handle(Visitor *v, Error **errp);void visit_start_struct (Visitor *v,void **obj,const char *kind,const char *name,size_t size,Error **errp);void visit_end_struct (Visitor *v,Error **errp); +void visit_start_implicit_struct (Visitor *v,void **obj,size_t size, +Error **errp); +void visit_end_implicit_struct (Visitor *v,Error **errp);void visit_start_list (Visitor *v,const char *name,Error **errp);GenericList *visit_next_list (Visitor *v,GenericList **list,Error **errp);void visit_end_list (Visitor *v,Error **errp);
qapi/qapi-visit-core.c +16 -0
@@ -45,6 +45,22 @@ void visit_end_struct(Visitor *v, Error **errp) v->end_struct (v, errp); } +void visit_start_implicit_struct (Visitor *v,void **obj,size_t size, +Error **errp) +{ +if (!error_is_set (errp) && v->start_implicit_struct) { + v->start_implicit_struct (v, obj, size, errp); + } +} + +void visit_end_implicit_struct (Visitor *v,Error **errp) +{ +assert (!error_is_set (errp)); +if (v->end_implicit_struct) { + v->end_implicit_struct (v, errp); + } +} +void visit_start_list (Visitor *v,const char *name,Error **errp) {if (!error_is_set (errp)) {
qapi/qmp-input-visitor.c +14 -0
@@ -144,6 +144,18 @@ static void qmp_input_end_struct(Visitor *v, Error **errp)qmp_input_pop (qiv, errp); } +static void qmp_input_start_implicit_struct (Visitor *v,void **obj, +size_t size,Error **errp) +{ +if (obj) { + *obj =g_malloc0 (size); + } +} + +static void qmp_input_end_implicit_struct (Visitor *v,Error **errp) +{ +} +static void qmp_input_start_list (Visitor *v,const char *name,Error **errp) {QmpInputVisitor *qiv =to_qiv (v);
@@ -293,6 +305,8 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.start_struct = qmp_input_start_struct; v->visitor.end_struct = qmp_input_end_struct; + v->visitor.start_implicit_struct = qmp_input_start_implicit_struct; + v->visitor.end_implicit_struct = qmp_input_end_implicit_struct; v->visitor.start_list = qmp_input_start_list; v->visitor.next_list = qmp_input_next_list; v->visitor.end_list = qmp_input_end_list;
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs
Quoted 5 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 (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)
Quoted 7 lines
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(+)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 7/18 qapi: Flat unions with arbitrary discriminator
In reply to Kevin WolfInstead 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" }--- 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 +22 -0
@@ -103,6 +103,28 @@ And it looks like this on the wire: "data" : { "backing-file": "/some/place/my-image", "lazy-refcounts": true } } + +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. +The above example can then be modified as follows: + + { 'type': 'BlockdevCommonOptions', + 'data': { 'driver': 'str' 'readonly': 'bool' } } + { 'union': 'BlockdevOptions', + 'base': 'BlockdevCommonOptions', + 'discriminator': 'driver', + 'data': { 'raw': 'RawOptions', + 'qcow2': 'Qcow2Options' } } + +Resulting in this JSON object: + + { "driver": "qcow2", + "readonly": false, + "backing-file": "/some/place/my-image", + "lazy-refcounts": true } + + === Commands === Commands are defined by using a list containing three members. The first
scripts/qapi-types.py +6 -0
@@ -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 if base: struct = find_struct(base) + if discriminator: + del struct['data'][discriminator] ret += generate_struct_fields(struct['data']) + else: + assert not discriminator ret += mcgen(''' };
scripts/qapi-visit.py +63 -23
151 lines not on this page · raw message
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
Quoted 9 lines · Kevin Wolf
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 14 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
docs/qapi-code-gen.txt And it looks like this on the wire: · line 109
Quoted 7 lines
docs/qapi-code-gen.txt +4
-0@@ -103,6 +103,28 @@ And it looks like this on the wire: "data" : { "backing-file": "/some/place/my-image", "lazy-refcounts": true } } + +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 12 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
docs/qapi-code-gen.txt struct %(name)s · line 191
Quoted 5 lines
docs/qapi-code-gen.txt +2
-0@@ -185,7 +187,11 @@ struct %(name)s 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...
Quoted 13 lines
docs/qapi-code-gen.txt +2
-0ret += generate_struct_fields(struct['data']) + else: + assert not discriminator ret += mcgen(''' };scripts/qapi-visit.py +0
-1--- a/scripts/qapi-visit.py@@ -157,9 +178,17 @@ def generate_visit_union(expr): members = expr['data']
scripts/qapi-visit.py def generate_visit_union(expr): · line 189
Quoted 10 lines
scripts/qapi-visit.py +6
-0base = expr.get('base') + discriminator = expr.get('discriminator') ret = generate_visit_enum('%sKind' % name, members.keys()) + if base: + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator]
...here, you explicitly took a copy to be safe. So I suspect you are missing a .copy() above.
I think my findings are easy fixes; so I'm okay if you fix them and then add:
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
Am 26.07.2013 um 15:40 hat Eric Blake geschrieben:
Quoted 37 lines · Kevin Wolf
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@@ -103,6 +103,28 @@ And it looks like this on the wire: "data" : { "backing-file": "/some/place/my-image", "lazy-refcounts": true } }
Quoted 16 lines
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.
There's nothing to add to this, I would certainly support such a future patch.
Quoted 20 lines
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
Quoted 11 lines
docs/qapi-code-gen.txt struct %(name)s · line 191
Quoted 4 lines
docs/qapi-code-gen.txt +2
-0if 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...
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:
@@ -184,8 +186,13 @@ struct %(name)s
''')
if base:
- struct = find_struct(base) - ret += generate_struct_fields(struct['data']) + base_fields = find_struct(base)['data'] + if discriminator:
+ base_fields = base_fields.copy()
+ del base_fields[discriminator]
+ ret += generate_struct_fields(base_fields)
+ else:
+ assert not discriminator
ret += mcgen('''};
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]>
Thanks, Eric.
Kevin
Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
Quoted 12 lines · Kevin Wolf
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...
Quoted 4 lines
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.
Ah, so this is a case of the blind leading the blind...
Quoted 6 lines
I've changed this hunk now to match the other one:
(quoted diff) +0
-0@@ -184,8 +186,13 @@ struct %(name)s ''')
Review struct %(name)s · line 195
Quoted 10 lines
(quoted diff) +7
-2if base: - struct = find_struct(base) - ret += generate_struct_fields(struct['data']) + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator] + ret += generate_struct_fields(base_fields) + else: + assert not discriminator
Yes, that makes more sense.
Quoted 4 lines
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]>
Which means this is indeed a valid review.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v2 7/17 qapi: Flat unions with arbitrary discriminator
In reply to Kevin WolfInstead 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" }--- 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.
docs/qapi-code-gen.txt +22 -0
@@ -103,6 +103,28 @@ And it looks like this on the wire: "data" : { "backing-file": "/some/place/my-image", "lazy-refcounts": true } } + +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. +The above example can then be modified as follows: + + { 'type': 'BlockdevCommonOptions', + 'data': { 'driver': 'str', 'readonly': 'bool' } } + { 'union': 'BlockdevOptions', + 'base': 'BlockdevCommonOptions', + 'discriminator': 'driver', + 'data': { 'raw': 'RawOptions', + 'qcow2': 'Qcow2Options' } } + +Resulting in this JSON object: + + { "driver": "qcow2", + "readonly": false, + "backing-file": "/some/place/my-image", + "lazy-refcounts": true } + + === Commands === Commands are defined by using a list containing three members. The first
scripts/qapi-types.py +9 -2
@@ -154,7 +154,9 @@ def generate_union(expr): name = expr['union'] typeinfo = expr['data'] + base = expr.get('base') + discriminator = expr.get('discriminator') ret = mcgen(''' struct %(name)s
@@ -177,8 +179,13 @@ struct %(name)s ''') if base: - struct = find_struct(base) - ret += generate_struct_fields(struct['data']) + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator] + ret += generate_struct_fields(base_fields) + else: + assert not discriminator ret += mcgen(''' };
scripts/qapi-visit.py +67 -23
155 lines not on this page · raw message
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH v2 07/17] qapi: Flat unions with arbitrary discriminator
Quoted 25 lines · Kevin Wolf
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(-)
Quoted 5 lines
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, andfoo_barbarinstead offoo_bar_bazfor functions. Fixed that.
Serves me right for doing just a visual code review instead of 'git am && make check'; but now you know why I only gave Reviewed-by instead of the stronger Acked-by.
At any rate, this version looks better;
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 8/18 qapi: Add consume argument to qmp_input_get_object()
In reply to Kevin WolfThis allows to just look at the next element without actually consuming it.
--- qapi/qmp-input-visitor.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
qapi/qmp-input-visitor.c +10 -9
@@ -41,13 +41,14 @@ static QmpInputVisitor *to_qiv(Visitor *v) }static QObject *qmp_input_get_object (QmpInputVisitor *qiv, -const char *name) +const char *name, +bool consume) {QObject *qobj = qiv->stack[qiv->nb_stack -1 ].obj;if (qobj) {if (name &&qobject_type (qobj) ==QTYPE_QDICT ) { -if (qiv->stack[qiv->nb_stack -1 ].h) { +if (qiv->stack[qiv->nb_stack -1 ].h && consume) {g_hash_table_remove (qiv->stack[qiv->nb_stack -1 ].h, name); }return qdict_get (qobject_to_qdict (qobj), name);
@@ -117,7 +118,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind, const char *name, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);Error *err =NULL ;if (!qobj ||qobject_type (qobj) !=QTYPE_QDICT ) {
@@ -159,7 +160,7 @@ static void qmp_input_end_implicit_struct(Visitor *v, Error **errp)static void qmp_input_start_list (Visitor *v,const char *name,Error **errp) {QmpInputVisitor *qiv =to_qiv (v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj ||qobject_type (qobj) !=QTYPE_QLIST ) { error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -211,7 +212,7 @@ static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) {QmpInputVisitor *qiv =to_qiv (v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj ||qobject_type (qobj) !=QTYPE_QINT ) { error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -226,7 +227,7 @@ static void qmp_input_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) {QmpInputVisitor *qiv =to_qiv (v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj ||qobject_type (qobj) !=QTYPE_QBOOL ) { error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -241,7 +242,7 @@ static void qmp_input_type_str(Visitor *v, char **obj, const char *name, Error **errp) {QmpInputVisitor *qiv =to_qiv (v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj ||qobject_type (qobj) !=QTYPE_QSTRING ) { error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -256,7 +257,7 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name, Error **errp) {QmpInputVisitor *qiv =to_qiv (v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj || (qobject_type (qobj) !=QTYPE_QFLOAT &&qobject_type (qobj) !=QTYPE_QINT )) {
@@ -276,7 +277,7 @@ static void qmp_input_start_optional(Visitor *v, bool *present, const char *name, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); -QObject *qobj =qmp_input_get_object (qiv, name); +QObject *qobj =qmp_input_get_object (qiv, name, true);if (!qobj) { *present = false;
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object()
Quoted 7 lines · Kevin Wolf
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(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 9/18 qapi.py: Maintain a list of union types
In reply to Kevin Wolf--- scripts/qapi.py | 13 +++++++++++++ 1 file changed, 13 insertions(+)
scripts/qapi.py +13 -0
@@ -105,6 +105,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 +189,7 @@ def type_name(name): enum_types = [] struct_types = [] +union_types = [] def add_struct(definition): global struct_types
@@ -200,6 +202,17 @@ def find_struct(name): return struct return None +def add_union(definition): + global union_types + union_types.append(definition) + +def find_union(name): + global union_types + for union in union_types: + if union['union'] == name: + return union + return None + def add_enum(name): global enum_types enum_types.append(name)
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types
Quoted 4 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi.py | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 10/18 qapi: Anonymous unions
In reply to Kevin WolfThe 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' } }--- docs/qapi-code-gen.txt | 25 ++++++++++++++++++++++++ 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 | 15 +++++++++++++++ 10 files changed, 160 insertions(+)
docs/qapi-code-gen.txt +25 -0
@@ -125,6 +125,31 @@ Resulting in this JSON object: "lazy-refcounts": true } +A special type of unions are anonymous unions. They don't form a dictionary in +the wire format but allow the direct use of different types in their place. As +they aren't structured, they don't have any explicit discriminator but use +the (QObject) data type of their value as an implicit discriminator. This means +that they are restricted to using only one discriminator value per QObject +type. For example, you cannot have two different complex types in an anonymous +union, or two different integer types. + +Anonymous unions are declared using an empty dictionary as their discriminator. +The disriminator values never appear on the wire, they are only used in the +generated C code. Anonymous unions cannot have a base type. + + { 'union': 'BlockRef', + 'discriminator': {}, + 'data': { 'definition': 'BlockdevOptions' + 'reference': 'str' } } + +This example allows using both of the following example objects: + + { "file": "my_existing_block_device_id" } + { "file": { "driver": "file", + "readonly": false, + 'filename': "/tmp/mydisk.qcow2" } } + + === Commands === Commands are defined by using a list containing three members. The first
include/qapi/qmp/qobject.h +1 -0
@@ -44,6 +44,7 @@ typedef enum {QTYPE_QFLOAT ,QTYPE_QBOOL ,QTYPE_QERROR , +QTYPE_MAX , } qtype_code;struct QObject ;
include/qapi/visitor-impl.h +2 -0
@@ -32,6 +32,8 @@ struct Visitorvoid (*type_enum)(Visitor *v,int *obj,const char *strings[],const char *kind,const char *name,Error **errp); +void (*get_next_type)(Visitor *v,int *kind,const int *qobjects, +const char *name,Error **errp);void (*type_int)(Visitor *v,int64_t *obj,const char *name,Error **errp);void (*type_bool)(Visitor *v,bool *obj,const char *name,Error **errp);
include/qapi/visitor.h +3 -0
@@ -13,6 +13,7 @@#ifndef QAPI_VISITOR_CORE_H #define QAPI_VISITOR_CORE_H +#include "qapi/qmp/qobject.h" #include "qapi/error.h" #include <stdlib.h>
@@ -42,6 +43,8 @@ void visit_end_list(Visitor *v, Error **errp);void visit_start_optional (Visitor *v,bool *present,const char *name,Error **errp);void visit_end_optional (Visitor *v,Error **errp); +void visit_get_next_type (Visitor *v,int *obj,const int *qtypes, +const char *name,Error **errp);void visit_type_enum (Visitor *v,int *obj,const char *strings[],const char *kind,const char *name,Error **errp);void visit_type_int (Visitor *v,int64_t *obj,const char *name,Error **errp);
qapi/qapi-visit-core.c +9 -0
@@ -12,6 +12,7 @@*/ #include "qemu-common.h" +#include "qapi/qmp/qobject.h" #include "qapi/qmp/qerror.h" #include "qapi/visitor.h" #include "qapi/visitor-impl.h"
@@ -98,6 +99,14 @@ void visit_end_optional(Visitor *v, Error **errp) } } +void visit_get_next_type (Visitor *v,int *obj,const int *qtypes, +const char *name,Error **errp) +{ +if (!error_is_set (errp) && v->get_next_type) { + v->get_next_type (v, obj, qtypes, name, errp); + } +} +void visit_type_enum (Visitor *v,int *obj,const char *strings[],const char *kind,const char *name,Error **errp) {
qapi/qmp-input-visitor.c +14 -0
@@ -208,6 +208,19 @@ static void qmp_input_end_list(Visitor *v, Error **errp)qmp_input_pop (qiv, errp); } +static void qmp_input_get_next_type (Visitor *v,int *kind,const int *qobjects, +const char *name,Error **errp) +{ +QmpInputVisitor *qiv =to_qiv (v); +QObject *qobj =qmp_input_get_object (qiv, name, false); + +if (!qobj) { +error_set (errp,QERR_MISSING_PARAMETER , name ? name :"null" ); +return ; + } + *kind = qobjects[qobject_type (qobj)]; +} +static void qmp_input_type_int (Visitor *v,int64_t *obj,const char *name,Error **errp) {
@@ -317,6 +330,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.type_str = qmp_input_type_str; v->visitor.type_number = qmp_input_type_number; v->visitor.start_optional = qmp_input_start_optional; + v->visitor.get_next_type = qmp_input_get_next_type;qmp_input_push (v, obj,NULL );qobject_incref (obj);
qobject/qjson.c +2 -0
@@ -260,6 +260,8 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)/* XXX: should QError be emitted? */ case QTYPE_NONE :break ; +case QTYPE_MAX : +abort (); } }
scripts/qapi-types.py +42 -0
60 lines not on this page · raw message
scripts/qapi-visit.py +47 -0
59 lines not on this page · raw message
scripts/qapi.py +15 -0
@@ -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', + 'uint16': 'QTYPE_QINT', + 'uint32': 'QTYPE_QINT', + 'uint64': 'QTYPE_QINT', +} + def tokenize(data): while len(data): ch = data[0]
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions
Quoted 6 lines · Kevin Wolf
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 } }
Quoted 6 lines
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'
Missing comma.
Quoted 12 lines
'reference': 'str' } } { 'type': 'ExampleObject', 'data: { 'file': 'BlockRef' } }Signed-off-by Kevin Wolf <[email protected]>---
+++
b/docs/qapi-code-gen.txtdocs/qapi-code-gen.txt +0
-0@@ -125,6 +125,31 @@ Resulting in this JSON object: "lazy-refcounts": true }
docs/qapi-code-gen.txt Resulting in this JSON object: · line 137
Quoted 10 lines
docs/qapi-code-gen.txt +10
-0+A special type of unions are anonymous unions. They don't form a dictionary in +the wire format but allow the direct use of different types in their place. As +they aren't structured, they don't have any explicit discriminator but use +the (QObject) data type of their value as an implicit discriminator. This means +that they are restricted to using only one discriminator value per QObject +type. For example, you cannot have two different complex types in an anonymous +union, or two different integer types. + +Anonymous unions are declared using an empty dictionary as their discriminator. +The disriminator values never appear on the wire, they are only used in the
s/disriminator/discriminator/
docs/qapi-code-gen.txt Resulting in this JSON object: · line 142
Quoted 5 lines
docs/qapi-code-gen.txt +5
-0+generated C code. Anonymous unions cannot have a base type. + + { 'union': 'BlockRef', + 'discriminator': {}, + 'data': { 'definition': 'BlockdevOptions'
Missing comma.
qobject/qjson.c static void to_json(const QObject *obj, QString *str, int pretty, int indent) · line 264
Quoted 7 lines
qobject/qjson.c +3
-0+++ b/qobject/qjson.c@@ -260,6 +260,8 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)/* XXX: should QError be emitted? */ case QTYPE_NONE :break ; +case QTYPE_MAX : +abort ();
There's another recent patch proposing the use of g_assert_not_reached():
https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg04560.html
but I don't know if it is worth switching abort() over to this function (and even if it was, there's enough other use of abort in the qapi code that it would be better to switch it all as a separate patch).
Fix the typos, then you can add:
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 11/18 block: Allow "driver" option on the top level
In reply to Kevin WolfThis 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.
--- block.c | 7 +++++++ blockdev.c | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-)
block.c +7 -0
@@ -983,6 +983,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,char tmp_filename[PATH_MAX +1 ];BlockDriverState *file =NULL ;QDict *file_options =NULL ; +const char *drvname;/* NULL means an empty set of options */ if (options ==NULL ) {
@@ -1079,6 +1080,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, }/* Find the right image format driver */ + drvname =qdict_get_try_str (options,"driver" ); +if (drvname) { + drv =bdrv_find_whitelisted_format (drvname, !(flags &BDRV_O_RDWR )); +qdict_del (options,"driver" ); + } +if (!drv) { ret =find_image_format (file, filename, &drv); }
blockdev.c +10 -10
@@ -322,7 +322,6 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)enum {MEDIA_DISK ,MEDIA_CDROM } media;int bus_id, unit_id;int cyls, heads, secs, translation; -BlockDriver *drv =NULL ;int max_devs;int index;int ro =0 ;
@@ -338,6 +337,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)QemuOpts *opts;QDict *bs_opts;const char *id; +bool has_driver_specific_opts; translation =BIOS_ATA_TRANSLATION_AUTO ; media =MEDIA_DISK ;
@@ -365,6 +365,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)qdict_del (bs_opts,"id" ); } + has_driver_specific_opts = !!qdict_size (bs_opts); +/* extract parameters */ bus_id =qemu_opt_get_number (opts,"bus" ,0 ); unit_id =qemu_opt_get_number (opts,"unit" ,-1 );
@@ -477,11 +479,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)error_printf ("\n" );return NULL ; } - drv =bdrv_find_whitelisted_format (buf, ro); -if (!drv) { -error_report ("'%s' invalid format" , buf); -return NULL ; - } + +qdict_put (bs_opts,"driver" ,qstring_from_str (buf)); }/* disk I/O throttling */
@@ -658,7 +657,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)abort (); }if (!file || !*file) { -if (qdict_size (bs_opts)) { +if (has_driver_specific_opts) { file =NULL ; }else {return dinfo;
@@ -695,13 +694,13 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)error_report ("warning: disabling copy_on_read on readonly drive" ); } - ret =bdrv_open (dinfo->bdrv, file, bs_opts, bdrv_flags, drv); - bs_opts =NULL ; +QINCREF (bs_opts); + ret =bdrv_open (dinfo->bdrv, file, bs_opts, bdrv_flags,NULL );if (ret <0 ) {if (ret == -EMEDIUMTYPE ) {error_report ("could not open disk image %s: not in %s format" , - file ?: dinfo->id, drv->format_name); + file ?: dinfo->id,qdict_get_str (bs_opts,"driver" )); }else {error_report ("could not open disk image %s: %s" , file ?: dinfo->id,strerror (-ret));
@@ -712,6 +711,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)if (bdrv_key_required (dinfo->bdrv)) autostart =0 ; +QDECREF (bs_opts);qemu_opts_del (opts);return dinfo;
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level
Quoted 9 lines · Kevin Wolf
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(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 12/18 QemuOpts: Add qemu_opt_unset()
In reply to Kevin Wolf--- include/qemu/option.h | 1 + util/qemu-option.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
include/qemu/option.h +1 -0
@@ -120,6 +120,7 @@ bool qemu_opt_has_help_opt(QemuOpts *opts);bool qemu_opt_get_bool (QemuOpts *opts,const char *name,bool defval);uint64_t qemu_opt_get_number (QemuOpts *opts,const char *name,uint64_t defval);uint64_t qemu_opt_get_size (QemuOpts *opts,const char *name,uint64_t defval); +int qemu_opt_unset (QemuOpts *opts,const char *name);int qemu_opt_set (QemuOpts *opts,const char *name,const char *value);void qemu_opt_set_err (QemuOpts *opts,const char *name,const char *value,Error **errp);
util/qemu-option.c +14 -0
@@ -593,6 +593,20 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,return NULL ; } +int qemu_opt_unset (QemuOpts *opts,const char *name) +{ +QemuOpt *opt =qemu_opt_find (opts, name); + +assert (opts_accepts_any (opts)); + +if (opt ==NULL ) { +return -1 ; + }else { +qemu_opt_del (opt); +return 0 ; + } +} +static void opt_set (QemuOpts *opts,const char *name,const char *value,bool prepend,Error **errp) {
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 12/18] QemuOpts: Add qemu_opt_unset()
Quoted 6 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- include/qemu/option.h | 1 + util/qemu-option.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 13/18 blockdev: Rename I/O throttling options for QMP
In reply to Kevin WolfIn 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.
--- blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 13 deletions(-)
blockdev.c +39 -13
@@ -312,7 +312,8 @@ static bool do_check_io_limits(BlockIOLimit *io_limits, Error **errp)return true; } -DriveInfo *drive_init (QemuOpts *all_opts,BlockInterfaceType block_default_type) +static DriveInfo *blockdev_init (QemuOpts *all_opts, +BlockInterfaceType block_default_type) {const char *buf;const char *file =NULL ;
@@ -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 ); 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 ); io_limits.iops[BLOCK_IO_LIMIT_TOTAL ] = -qemu_opt_get_number (opts,"iops" ,0 ); +qemu_opt_get_number (opts,"throttling.iops-total" ,0 ); io_limits.iops[BLOCK_IO_LIMIT_READ ] = -qemu_opt_get_number (opts,"iops_rd" ,0 ); +qemu_opt_get_number (opts,"throttling.iops-read" ,0 ); io_limits.iops[BLOCK_IO_LIMIT_WRITE ] = -qemu_opt_get_number (opts,"iops_wr" ,0 ); +qemu_opt_get_number (opts,"throttling.iops-write" ,0 );if (!do_check_io_limits (&io_limits, &error)) {error_report ("%s" ,error_get_pretty (error));
@@ -726,6 +727,31 @@ err:return NULL ; } +static void qemu_opt_rename (QemuOpts *opts,const char *from,const char *to) +{ +const char *value; + + value =qemu_opt_get (opts, from); +if (value) { +qemu_opt_set (opts, to, value); +qemu_opt_unset (opts, from); + } +} + +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" ); + +qemu_opt_rename (all_opts,"bps" ,"throttling.bps-total" ); +qemu_opt_rename (all_opts,"bps_rd" ,"throttling.bps-read" ); +qemu_opt_rename (all_opts,"bps_wr" ,"throttling.bps-write" ); + +return blockdev_init (all_opts, block_default_type); +} +void do_commit (Monitor *mon,const QDict *qdict) {const char *device =qdict_get_str (qdict,"device" );
@@ -1855,27 +1881,27 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{ - .name = "iops", + .name = "throttling.iops-total", .type = QEMU_OPT_NUMBER, .help = "limit total I/O operations per second", },{ - .name = "iops_rd", + .name = "throttling.iops-read", .type = QEMU_OPT_NUMBER, .help = "limit read operations per second", },{ - .name = "iops_wr", + .name = "throttling.iops-write", .type = QEMU_OPT_NUMBER, .help = "limit write operations per second", },{ - .name = "bps", + .name = "throttling.bps-total", .type = QEMU_OPT_NUMBER, .help = "limit total bytes per second", },{ - .name = "bps_rd", + .name = "throttling.bps-read", .type = QEMU_OPT_NUMBER, .help = "limit read bytes per second", },{ - .name = "bps_wr", + .name = "throttling.bps-write", .type = QEMU_OPT_NUMBER, .help = "limit write bytes per second", },{
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Quoted 10 lines · Kevin Wolf
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
-2io_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?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Am 26.07.2013 um 18:10 hat Eric Blake geschrieben:
Quoted 12 lines · Kevin Wolf
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(-)
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 20 lines
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
-2io_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.
Quoted 12 lines
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?
Yes, of course the command line will accept the new spellings as well. I'll update the commit message.
Kevin
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Quoted 7 lines · Kevin Wolf
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.
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 2 lines
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Am 26.07.2013 um 18:44 hat Eric Blake geschrieben:
Quoted 6 lines · Kevin Wolf
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 8 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.
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?
Quoted 4 lines
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
Yes, it was discussed on IRC, Benoît will target 1.7 now.
Quoted 8 lines
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.
Indeed, adding new options and switching the whole algorithm that late in the cycle is something that I would find a little bit too scary.
Kevin
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Quoted 4 lines · Kevin Wolf
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 8 lines
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?
Yes, I can commit to having libvirt coded up to use QMP fd-passing for backing file chains prior to qemu 1.7, which will pretty much be a full-stack test of everything you've been incrementally adding.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
In reply to Eric BlakeQuoted 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.
Best regards
Benoît
Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
Quoted 5 lines · Benoît Canet
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.
s/serie/series/
[Stupid English, where every rule has an exception. Here, the exception is that "series" is the correct spelling of both singular and plural form. Don't fret, you're not the first non-native speaker to be tripped up by this oddity.]
Quoted 3 lines
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.
As I mentioned on that series, one possibility would be to have:
'*throttling': { 'bps-read': nnn, <up to 6 members, as now> } '*throttling-threshold': { 'bps-read': nnn, ...}
so that instead of naming 6 new members, you are just naming 1 new struct that shares the same 6 member names as the first struct.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 14/18 qcow2: Use dashes instead of underscores in options
In reply to Kevin WolfThis is what QMP wants to use. The options haven't been enabled in any release yet, so we're still free to change them.
--- block/qcow2.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
block/qcow2.h +4 -4
@@ -59,10 +59,10 @@#define DEFAULT_CLUSTER_SIZE 65536 -#define QCOW2_OPT_LAZY_REFCOUNTS "lazy_refcounts" -#define QCOW2_OPT_DISCARD_REQUEST "pass_discard_request" -#define QCOW2_OPT_DISCARD_SNAPSHOT "pass_discard_snapshot" -#define QCOW2_OPT_DISCARD_OTHER "pass_discard_other" +#define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts" +#define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request" +#define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot" +#define QCOW2_OPT_DISCARD_OTHER "pass-discard-other" typedef struct QCowHeader { uint32_t magic;
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options
Quoted 2 lines · Kevin Wolf
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.
Aha - we ARE aiming to get this series pushed in 1.6.
Quoted 5 lines
Signed-off-by Kevin Wolf <[email protected]>--- block/qcow2.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 15/18 blockdev: Rename 'readonly' option to 'read-only'
In reply to Kevin WolfOption name cleanup before it becomes a QMP API.
--- blockdev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
blockdev.c +6 -4
@@ -378,7 +378,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, secs =qemu_opt_get_number (opts,"secs" ,0 ); snapshot =qemu_opt_get_bool (opts,"snapshot" ,0 ); - ro =qemu_opt_get_bool (opts,"readonly" ,0 ); + ro =qemu_opt_get_bool (opts,"read-only" ,0 ); copy_on_read =qemu_opt_get_bool (opts,"copy-on-read" , false); file =qemu_opt_get (opts,"file" );
@@ -684,7 +684,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, }else if (ro ==1 ) {if (type !=IF_SCSI && type !=IF_VIRTIO && type !=IF_FLOPPY && type !=IF_NONE && type !=IF_PFLASH ) { -error_report ("readonly not supported by this bus type" ); +error_report ("read-only not supported by this bus type" ); goto err; } }
@@ -692,7 +692,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, bdrv_flags |= ro ?0 :BDRV_O_RDWR ;if (ro && copy_on_read) { -error_report ("warning: disabling copy_on_read on readonly drive" ); +error_report ("warning: disabling copy_on_read on read-only drive" ); }QINCREF (bs_opts);
@@ -749,6 +749,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)qemu_opt_rename (all_opts,"bps_rd" ,"throttling.bps-read" );qemu_opt_rename (all_opts,"bps_wr" ,"throttling.bps-write" ); +qemu_opt_rename (all_opts,"readonly" ,"read-only" ); +return blockdev_init (all_opts, block_default_type); }
@@ -1877,7 +1879,7 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_STRING, .help = "pci address (virtio only)", },{ - .name = "readonly", + .name = "read-only", .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 15/18] blockdev: Rename 'readonly' option to 'read-only'
Quoted 7 lines · Kevin Wolf
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(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 16/18 blockdev: Split up 'cache' option
In reply to Kevin WolfThe 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:
-drive file=x,cache=none,cache.no-flush=true
-drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true
--- blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)
blockdev.c +47 -10
@@ -452,12 +452,15 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, } } - bdrv_flags |=BDRV_O_CACHE_WB ; -if ((buf =qemu_opt_get (opts,"cache" )) !=NULL ) { -if (bdrv_parse_cache_flags (buf, &bdrv_flags) !=0 ) { -error_report ("invalid cache option" ); -return NULL ; - } + bdrv_flags =0 ; +if (qemu_opt_get_bool (opts,"cache.writeback" , true)) { + bdrv_flags |=BDRV_O_CACHE_WB ; + } +if (qemu_opt_get_bool (opts,"cache.direct" , false)) { + bdrv_flags |=BDRV_O_NOCACHE ; + } +if (qemu_opt_get_bool (opts,"cache.no-flush" , true)) { + bdrv_flags |=BDRV_O_NO_FLUSH ; }#ifdef CONFIG_LINUX_AIO
@@ -740,6 +743,8 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)DriveInfo *drive_init (QemuOpts *all_opts,BlockInterfaceType block_default_type) { +const char *value; +/* 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" );
@@ -751,6 +756,31 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)qemu_opt_rename (all_opts,"readonly" ,"read-only" ); + value =qemu_opt_get (all_opts,"cache" ); +if (value) { +int flags =0 ; + +if (bdrv_parse_cache_flags (value, &flags) !=0 ) { +error_report ("invalid cache option" ); +return NULL ; + } + +/* Specific options take precedence */ +if (!qemu_opt_get (all_opts,"cache.writeback" )) { +qemu_opt_set_bool (all_opts,"cache.writeback" , + !!(flags &BDRV_O_CACHE_WB )); + } +if (!qemu_opt_get (all_opts,"cache.direct" )) { +qemu_opt_set_bool (all_opts,"cache.direct" , + !!(flags &BDRV_O_NOCACHE )); + } +if (!qemu_opt_get (all_opts,"cache.no-flush" )) { +qemu_opt_set_bool (all_opts,"cache.no-flush" , + !!(flags &BDRV_O_NO_FLUSH )); + } +qemu_opt_unset (all_opts,"cache" ); + } +return blockdev_init (all_opts, block_default_type); }
@@ -1850,10 +1880,17 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_STRING, .help = "discard operation (ignore/off, unmap/on)", },{ - .name = "cache", - .type = QEMU_OPT_STRING, - .help = "host cache usage (none, writeback, writethrough, " - "directsync, unsafe)", + .name = "cache.writeback", + .type = QEMU_OPT_BOOL, + .help = "enables writeback mode for any caches", + },{ + .name = "cache.direct", + .type = QEMU_OPT_BOOL, + .help = "enables use of O_DIRECT (bypass the host page cache)", + },{ + .name = "cache.no-flush", + .type = QEMU_OPT_BOOL, + .help = "ignore any flush requests for the device", },{ .name = "aio", .type = QEMU_OPT_STRING,
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option
Quoted 19 lines · Kevin Wolf
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=trueSigned-off-by Kevin Wolf <[email protected]>--- blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-)+++
b/blockdev.c
blockdev.c static DriveInfo *blockdev_init(QemuOpts *all_opts, · line 457
Quoted 6 lines
blockdev.c +0
-3@@ -452,12 +452,15 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, } } - bdrv_flags |=BDRV_O_CACHE_WB ; -if ((buf =qemu_opt_get (opts,"cache" )) !=NULL ) { -if (bdrv_parse_cache_flags (buf, &bdrv_flags) !=0 ) {
The old code calls bdrv_parse_cache_flags() with non-zero bdrv_flags.
blockdev.c DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) · line 763
Quoted 9 lines
blockdev.c +5
-0@@ -751,6 +756,31 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)qemu_opt_rename (all_opts,"readonly" ,"read-only" ); + value =qemu_opt_get (all_opts,"cache" ); +if (value) { +int flags =0 ; + +if (bdrv_parse_cache_flags (value, &flags) !=0 ) {
The new code calls it with flags starting at 0. But looking at bdrv_parse_cache_flags(), the first thing that code did was zero out BDRV_O_CACHE_MASK, which includes BDRV_O_CACHE_WB, so I think that in reality you still end up with the same bits set at the end of that parse call. Phew - that means you are actually getting rid of a dead |= assignment. [And you proved that the block code is a twisty maze of back-compat hacks.]
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 17/18 Implement qdict_flatten()
In reply to Kevin Wolfqdict_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.
--- include/qapi/qmp/qdict.h | 1 + qobject/qdict.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)
include/qapi/qmp/qdict.h +1 -0
@@ -65,5 +65,6 @@ int qdict_get_try_bool(const QDict *qdict, const char *key, int def_value);const char *qdict_get_try_str (const QDict *qdict,const char *key);QDict *qdict_clone_shallow (const QDict *src); +void qdict_flatten (QDict *qdict);#endif /* QDICT_H */
qobject/qdict.c +50 -0
53 lines not on this page · raw message
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()
Quoted 11 lines · Kevin Wolf
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)'.
If you agree with my analysis and incorporate my suggested fix, then I'm okay if you add:
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()
Am 26.07.2013 um 18:40 hat Eric Blake geschrieben:
Quoted 44 lines · Kevin Wolf
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...
Quoted 7 lines
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)'.
Gah, this is getting embarrassing. Who stole my continue? :-)
You're right of course, thanks for catching that. I'll fix it.
Kevin
PATCH 18/18 blockdev: 'blockdev-add' QMP command
In reply to Kevin Wolf--- blockdev.c | 45 +++++++++ qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 26 +++++ 3 files changed, 364 insertions(+)
blockdev.c +45 -0
@@ -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 +293 -0
296 lines not on this page · raw message
qmp-commands.hx +26 -0
@@ -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
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
Is it worth a snippet of QMP wire code in the commit message, so that someone reading 'git log' can more easily spot the impact of this patch?
Quoted 11 lines
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); }
blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1819
Quoted 10 lines
blockdev.c +10
-0+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 ; + }
[1]
blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1822
Quoted 3 lines
blockdev.c +3
-0+ +/* 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
s/off/false/
blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1825
Quoted 3 lines
blockdev.c +3
-0+* called from drive_init. +* +* For now, simply forbidding the combination for all drivers will do. */
Does the TODO mean that more patches will be added later? But I can live with the limitation documented for this patch.
blockdev.c void qmp_block_job_complete(const char *device, Error **errp) · line 1827
Quoted 2 lines
blockdev.c +2
-0+if (options->has_aio && options->aio ==BLOCKDEV_A_I_O_OPTIONS_NATIVE ) { +if (!options->has_cache || !options->cache->direct) {
options->cache->direct is listed as optional in the QAPI; does that mean you need to first check options->cache->has_direct, or have you done a sanity check elsewhere that provides sane defaults in the absence of optional parameters?
Quoted 6 lines
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 line 3774
Quoted 10 lines
qapi-schema.json +10
-0+ +## +# @BlockdevDiscardOptions +# +# Determines how to handle discard requests. +# +# @ignore: Ignore the request +# @unmap: Forward as an unmap request +# +# Since: 1.6
Given the discussion earlier in the series, should this (and all other new listings) mention 1.7? Regarding your plan for committing framework but then backing out the final implementation in time for 1.6, does that mean applying the whole series or just the first 17 patches? (Planning for the timing of a commit doesn't necessarily invalidate the review, but it's nice if we're all on the same page about when things will hit qemu.git.)
qapi-schema.json line 3830
Quoted 7 lines
qapi-schema.json +7
-0+{ 'type': 'BlockdevThrottlingOptions', + 'data': { '*bps-total': 'int', + '*bps-read': 'int', + '*bps-write': 'int', + '*iops-total': 'int', + '*iops-read': 'int', + '*iops-write': 'int' } }
A lot of these types look like they would also be useful in output structs; for example, do we need to modify BlockDeviceInfo? But that could be a separate patch.
Quoted 7 lines
qapi-schema.json +7
-0+ +## +# @BlockdevOptionsBase +# +# Options that are available for all block devices, independent of the block +# driver. +#
qapi-schema.json line 3847
Quoted 10 lines
qapi-schema.json +10
-0+# @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)
Is copy-on-read really applicable as a base option, or is it only appropriate for COW formats such as qcow2 and qed? Everything else looks good, especially since you plan on allowing layering (for example, a different rerror policy for a qcow2 primary image than what is used for its backing file, regardless of whether the backing file is raw or also qcow2).
Historically, we have marked optional members both with '*name' in the actual qapi, but also with '#optional' in the text; is it worth another pass over your patch to add in the missing '#optional' keywords? [Since nothing extracts docs automatically from the .json file yet, I'm okay if such a pass is deferred to a followup patch]
qapi-schema.json line 3853
Quoted 6 lines
qapi-schema.json +6
-0+# +# Since: 1.6 +## +{ 'type': 'BlockdevOptionsBase', + 'data': { 'driver': 'str', + '*id': 'str',
Per [1] above, you are failing if id was not provided. Then why is it marked optional?
qapi-schema.json line 3877
Quoted 5 lines
qapi-schema.json +5
-0+{ 'type': 'BlockdevOptionsFile', + 'data': { 'filename': 'str' } } + +## +# @BlockdevOptionsFile
copy-and-paste; this should be BlockdevOptionsNBD
Quoted 17 lines
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 line 3904
Quoted 10 lines
qapi-schema.json +10
-0+# 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
We probably ought to polish that before actually committing to this QMP interface.
qapi-schema.json line 3921
Quoted 9 lines
qapi-schema.json +9
-0+# @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 +# @floppy: whether to export a floppy imae (true) or partitioned hard disk +# (false; default) +# @rw: whether to allow write operations (default: false)
Does this duplicate the base class already providing 'read-only'? And if not, is there a nicer name than the abbreviation 'rw'?
qapi-schema.json line 3950
Quoted 12 lines
qapi-schema.json +12
-0+{ '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)
Is it possible to request the empty string as @backing in order to force the image to be used as though there were no backing file? I'm not sure if pulling such a stunt would ever make sense, but if we can override a file with no stored content to use a backing file, it seems like there should be a symmetrical way to override a file with a stored backing file to be used without backing.
qapi-schema.json line 3954
Quoted 4 lines
qapi-schema.json +4
-0+# +# Since: 1.6 +## +# TODO Add inheritance and base on BlockdevOptionsGenericFormat
Yep, we discussed that on IRC, and I agreed that deferring complex struct inheritance until later is not a show-stopper for this patch (but WOULD make it easier to add a new field without having to touch multiple types).
qapi-schema.json line 3959
Quoted 5 lines
qapi-schema.json +5
-0+{ 'type': 'BlockdevOptionsGenericCOWFormat', + 'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } } + +## +# @BlockdevOptionsGenericCOWFormat
Copy-and-paste, should be BlockdevOptionsQcow2
Quoted 10 lines
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 line 3979
Quoted 10 lines
qapi-schema.json +10
-0+# +# @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
We also discussed this on IRC, and neither of us could come up with any shorter names; anyone else want to take a stab at it? But at least adding docs helped compared to the scratch proposal you had run by me via pastebin earlier :)
Quoted 11 lines
qapi-schema.json +11
-0+ +## +# @BlockdevOptions +# +# Options for creating a block device. +# +# Since: 1.6 +## +{ 'union': 'BlockdevOptions', + 'base': 'BlockdevOptionsBase', + 'discriminator': 'driver',
qapi-schema.json line 4011
Quoted 10 lines
qapi-schema.json +10
-0+ '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'?
You documented BlockdevOptionsNBD above, and then aren't using it here because of the TODO. It doesn't hurt the qapi code generator to document unused types, but does look a bit fishy, especially since we may want to change the contents of that type. I guess it is things like this which reinforce your desire to get framework in, but to disable the feature until 1.7 has had more time to iron out warts before they become permanent.
Quoted 9 lines
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 line 4030
Quoted 10 lines
qapi-schema.json +10
-0+ 'parallels': 'BlockdevOptionsGenericFormat', + 'qcow': 'BlockdevOptionsGenericCOWFormat', + 'qcow2': 'BlockdevOptionsQcow2', + 'qed': 'BlockdevOptionsGenericCOWFormat', + 'raw': 'BlockdevOptionsGenericFormat', + 'vdi': 'BlockdevOptionsGenericFormat', + 'vhdx': 'BlockdevOptionsGenericFormat', + 'vmdk': 'BlockdevOptionsGenericCOWFormat', + 'vpc': 'BlockdevOptionsGenericFormat' + } }
If someone does go with the suggestion in 7/18 to make a discriminated union key off an enum type instead of an open-coded string, then we would also want to list the enum type that describes all the known drivers for qemu.
Quoted 4 lines
qapi-schema.json +4
-0+ +## +# @BlockdevRef +#
qapi-schema.json line 4044
Quoted 10 lines
qapi-schema.json +10
-0+# 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'
missing comma. If Markus' re-write of the qapi parser gets applied first, this wouldn't have happened :)
qapi-schema.json line 4056
Quoted 12 lines
qapi-schema.json +12
-0+ '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' } }
Looks deceptively simple, with all the complexity isolated into the type definitions above.
qmp-commands.hx Example: · line 3139
Quoted 7 lines
qmp-commands.hx +7
-0+Example: + +-> { "execute": "blockdev-add", + "arguments": { "options" : { "driver": "qcow2", + "file": { "driver": "file", + "filename": "test.qcow2" } } } } +<- { "return": {} }
Is it worth more than one example, showing a bit more of the full power of the schema?
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
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.
I'll reply in more detail to your other points later today or on Monday.
Kevin
Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
Quoted 11 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.
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?
Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
Am 30.07.2013 um 19:44 hat Luiz Capitulino geschrieben:
Quoted 10 lines
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 8 lines
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?
Correct. Patches 1-17 are in master now, and blockdev-add itself will be reposted during the 1.7 cycle.
Kevin
Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
In reply to Kevin WolfQuoted 233 lines · Kevin Wolf
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@@ -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@@ -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 line 3928
Quoted 10 lines
qapi-schema.json +10
-0+# @floppy: whether to export a floppy imae (true) or partitioned hard disk +# (false; default) +# @rw: whether to allow write operations (default: false) +# +# Since: 1.6 +## +{ 'type': 'BlockdevOptionsVVFAT', + 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool', + '*rw': 'bool' } } +
Paid some time to find out the organization of options, I think well
named type can help user understand better, how about rename as:
BlockdevOptionsFile --> BlockdevOptionsProtocolFile BlockdevOptionsNBD --> BlockdevOptionsProtocolNBD BlockdevOptionsSSH --> BlockdevOptionsProtocolSSH BlockdevOptionsVVFAT --> BlockdevOptionsProtocolVVFAT
BlockdevOptionsGenericFormat --> BlockdevOptionsFormatGeneric BlockdevOptionsGenericCOWFormat -->BlockdevOptionsFormatCowGeneric
....
Quoted 163 lines
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@@ -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
Signature · 3 lines
Best Regards Wenchao Xia