* [Qemu-devel] [PATCH 01/18] qapi-types.py: Split off generate_struct_fields()
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-25 23:06 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions Kevin Wolf
` (16 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-types.py | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index ddcfed9..e1239e1 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -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('''
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
2013-07-23 13:03 ` [Qemu-devel] [PATCH 01/18] qapi-types.py: Split off generate_struct_fields() Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-25 23:12 ` Eric Blake
2013-08-21 3:38 ` Amos Kong
2013-07-23 13:03 ` [Qemu-devel] [PATCH 03/18] qapi-visit.py: Split off generate_visit_struct_fields() Kevin Wolf
` (15 subsequent siblings)
17 siblings, 2 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
---
scripts/qapi-types.py | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e1239e1..9882b95 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -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'])
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-07-23 13:03 ` [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-25 23:12 ` Eric Blake
2013-08-21 3:38 ` Amos Kong
1 sibling, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-25 23:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-07-23 13:03 ` [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions Kevin Wolf
2013-07-25 23:12 ` Eric Blake
@ 2013-08-21 3:38 ` Amos Kong
2013-08-27 15:58 ` Kevin Wolf
1 sibling, 1 reply; 61+ messages in thread
From: Amos Kong @ 2013-08-21 3:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: lcapitulino, qemu-devel, stefanha, armbru
On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index e1239e1..9882b95 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -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'])
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]$
> +
> + 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'])
> --
> 1.8.1.4
>
--
Amos.
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-21 3:38 ` Amos Kong
@ 2013-08-27 15:58 ` Kevin Wolf
2013-08-29 13:52 ` Luiz Capitulino
0 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-08-27 15:58 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, armbru
Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:
> On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> > 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 <kwolf@redhat.com>
> > ---
> > scripts/qapi-types.py | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index e1239e1..9882b95 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -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'])
>
>
> 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]$
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
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-27 15:58 ` Kevin Wolf
@ 2013-08-29 13:52 ` Luiz Capitulino
2013-08-29 16:06 ` Kevin Wolf
0 siblings, 1 reply; 61+ messages in thread
From: Luiz Capitulino @ 2013-08-29 13:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Amos Kong, qemu-devel, stefanha, armbru
On Tue, 27 Aug 2013 17:58:59 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:
> > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> > > 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 <kwolf@redhat.com>
> > > ---
> > > scripts/qapi-types.py | 16 ++++++++++++++--
> > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > > index e1239e1..9882b95 100644
> > > --- a/scripts/qapi-types.py
> > > +++ b/scripts/qapi-types.py
> > > @@ -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'])
> >
> >
> > 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]$
>
> 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?
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 13:52 ` Luiz Capitulino
@ 2013-08-29 16:06 ` Kevin Wolf
2013-08-29 16:33 ` Luiz Capitulino
0 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-08-29 16:06 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amos Kong, qemu-devel, stefanha, armbru
Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:
> On Tue, 27 Aug 2013 17:58:59 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
> > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:
> > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> > > > 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 <kwolf@redhat.com>
> > > > ---
> > > > scripts/qapi-types.py | 16 ++++++++++++++--
> > > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > > > index e1239e1..9882b95 100644
> > > > --- a/scripts/qapi-types.py
> > > > +++ b/scripts/qapi-types.py
> > > > @@ -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'])
> > >
> > >
> > > 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]$
> >
> > 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
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 16:06 ` Kevin Wolf
@ 2013-08-29 16:33 ` Luiz Capitulino
2013-08-29 16:50 ` Kevin Wolf
2013-08-29 17:02 ` Eric Blake
0 siblings, 2 replies; 61+ messages in thread
From: Luiz Capitulino @ 2013-08-29 16:33 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Amos Kong, qemu-devel, stefanha, armbru
On Thu, 29 Aug 2013 18:06:50 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:
> > On Tue, 27 Aug 2013 17:58:59 +0200
> > Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:
> > > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> > > > > 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 <kwolf@redhat.com>
> > > > > ---
> > > > > scripts/qapi-types.py | 16 ++++++++++++++--
> > > > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > > > > index e1239e1..9882b95 100644
> > > > > --- a/scripts/qapi-types.py
> > > > > +++ b/scripts/qapi-types.py
> > > > > @@ -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'])
> > > >
> > > >
> > > > 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]$
> > >
> > > 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.
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.
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 16:33 ` Luiz Capitulino
@ 2013-08-29 16:50 ` Kevin Wolf
2013-08-29 17:02 ` Eric Blake
1 sibling, 0 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-08-29 16:50 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amos Kong, qemu-devel, stefanha, armbru
Am 29.08.2013 um 18:33 hat Luiz Capitulino geschrieben:
> On Thu, 29 Aug 2013 18:06:50 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
> > Am 29.08.2013 um 15:52 hat Luiz Capitulino geschrieben:
> > > On Tue, 27 Aug 2013 17:58:59 +0200
> > > Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > > Am 21.08.2013 um 05:38 hat Amos Kong geschrieben:
> > > > > On Tue, Jul 23, 2013 at 03:03:10PM +0200, Kevin Wolf wrote:
> > > > > > 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 <kwolf@redhat.com>
> > > > > > @@ -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]$
> > > >
> > > > 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.
>
> 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
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 16:33 ` Luiz Capitulino
2013-08-29 16:50 ` Kevin Wolf
@ 2013-08-29 17:02 ` Eric Blake
2013-08-29 18:36 ` Luiz Capitulino
1 sibling, 1 reply; 61+ messages in thread
From: Eric Blake @ 2013-08-29 17:02 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Kevin Wolf, Amos Kong, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 958 bytes --]
On 08/29/2013 10:33 AM, Luiz Capitulino wrote:
>>>
>>> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 17:02 ` Eric Blake
@ 2013-08-29 18:36 ` Luiz Capitulino
2013-08-30 7:30 ` Wenchao Xia
0 siblings, 1 reply; 61+ messages in thread
From: Luiz Capitulino @ 2013-08-29 18:36 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Amos Kong, qemu-devel, stefanha, armbru
On Thu, 29 Aug 2013 11:02:26 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 08/29/2013 10:33 AM, Luiz Capitulino wrote:
>
> >>>
> >>> 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.
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.
> 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?
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions
2013-08-29 18:36 ` Luiz Capitulino
@ 2013-08-30 7:30 ` Wenchao Xia
0 siblings, 0 replies; 61+ messages in thread
From: Wenchao Xia @ 2013-08-30 7:30 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Kevin Wolf, qemu-devel, armbru, stefanha, Amos Kong
于 2013-8-30 2:36, Luiz Capitulino 写道:
> On Thu, 29 Aug 2013 11:02:26 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 08/29/2013 10:33 AM, Luiz Capitulino wrote:
>>
>>>>>
>>>>> 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.
>
> 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.
>
>> 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.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 03/18] qapi-visit.py: Split off generate_visit_struct_fields()
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
2013-07-23 13:03 ` [Qemu-devel] [PATCH 01/18] qapi-types.py: Split off generate_struct_fields() Kevin Wolf
2013-07-23 13:03 ` [Qemu-devel] [PATCH 02/18] qapi-types.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-25 23:15 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions Kevin Wolf
` (14 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-visit.py | 62 ++++++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 6cac05a..a337d80 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -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;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (2 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 03/18] qapi-visit.py: Split off generate_visit_struct_fields() Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-25 23:19 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types Kevin Wolf
` (13 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
This implements the visitor part of base types for unions. Parsed into
QMP, this example schema definiton...
{ 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }
{ 'type': 'BlockOptionsQcow2, 'data': { 'lazy-refcounts': 'bool' } }
{ 'union': 'BlockOptions',
'base': 'BlockOptionsBase',
'data': {
'raw': 'BlockOptionsRaw'
'qcow2': 'BlockOptionsQcow2'
} }
...would describe the following JSON object:
{ "type": "qcow2",
"read-only": true,
"data": { "lazy-refcounts": false } }
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-visit.py | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a337d80..db6fa44 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -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)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions
2013-07-23 13:03 ` [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-25 23:19 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-25 23:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> This implements the visitor part of base types for unions. Parsed into
> QMP, this example schema definiton...
s/definiton/definition/
>
> { 'type': 'BlockOptionsBase', 'data': { 'read-only': 'bool' } }
> { 'type': 'BlockOptionsQcow2, 'data': { 'lazy-refcounts': 'bool' } }
>
> { 'union': 'BlockOptions',
> 'base': 'BlockOptionsBase',
> 'data': {
> 'raw': 'BlockOptionsRaw'
> 'qcow2': 'BlockOptionsQcow2'
> } }
>
> ...would describe the following JSON object:
>
> { "type": "qcow2",
> "read-only": true,
> "data": { "lazy-refcounts": false } }
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> scripts/qapi-visit.py | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (3 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 04/18] qapi-visit.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 13:06 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs Kevin Wolf
` (12 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
docs/qapi-code-gen.txt | 58 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 53 insertions(+), 5 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index cccb11e..555ca66 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -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 ==
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types
2013-07-23 13:03 ` [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types Kevin Wolf
@ 2013-07-26 13:06 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 13:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> 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/
> 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
s/who's/whose/
With those fixes,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (4 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 05/18] docs: Document QAPI union types Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 13:12 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator Kevin Wolf
` (11 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
These can be used when an embedded struct is parsed and members not
belonging to the struct may be present in the input (e.g. parsing a
flat namespace QMP union, where fields from both the base and one
of the alternative types are mixed in the JSON object)
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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(+)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 5159964..5c1297f 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -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);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 28c21d8..bd24f85 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -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);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 401ee6e..9b4d51b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -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)) {
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 67fb127..59c5cac 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (5 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 06/18] qapi: Add visitor for implicit structs Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 13:40 ` Eric Blake
2013-07-26 19:16 ` [Qemu-devel] [PATCH v2 07/17] " Kevin Wolf
2013-07-23 13:03 ` [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object() Kevin Wolf
` (10 subsequent siblings)
17 siblings, 2 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
---
docs/qapi-code-gen.txt | 22 +++++++++++++
scripts/qapi-types.py | 6 ++++
scripts/qapi-visit.py | 86 ++++++++++++++++++++++++++++++++++++--------------
3 files changed, 91 insertions(+), 23 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 555ca66..c187fda 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -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
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9882b95..db2f533 100644
--- a/scripts/qapi-types.py
+++ 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
if base:
struct = find_struct(base)
+ if discriminator:
+ del struct['data'][discriminator]
ret += generate_struct_fields(struct['data'])
+ else:
+ assert not discriminator
ret += mcgen('''
};
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index db6fa44..cd33e44 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,8 +17,26 @@ import os
import getopt
import errno
-def generate_visit_struct_fields(field_prefix, members):
+def generate_visit_struct_fields(name, field_prefix, members):
+ substructs = []
ret = ''
+ full_name = name if not field_prefix else "%s_%s" % (name, field_prefix)
+
+ for argname, argentry, optional, structured in parse_args(members):
+ if structured:
+ ret += generate_visit_struct_fields(name, field_prefix + argname, argentry)
+
+ ret += mcgen('''
+
+static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
+{
+ Error *err = NULL;
+''',
+ name=name, full_name=full_name)
+ push_indent()
+
+ if len(field_prefix):
+ field_prefix = field_prefix + "."
for argname, argentry, optional, structured in parse_args(members):
if optional:
@@ -31,7 +49,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
push_indent()
if structured:
- ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
+ ret += generate_visit_struct_body(full_name, argname, argentry)
else:
ret += mcgen('''
visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
@@ -47,6 +65,12 @@ visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s",
visit_end_optional(m, &err);
''')
+ pop_indent()
+ ret += mcgen('''
+
+ error_propagate(errp, err);
+}
+''')
return ret
@@ -56,8 +80,9 @@ if (!error_is_set(errp)) {
''')
push_indent()
+ full_name = name if not field_prefix else "%s_%s" % (field_prefix, name)
+
if len(field_prefix):
- field_prefix = field_prefix + "."
ret += mcgen('''
Error **errp = &err; /* from outer scope */
Error *err = NULL;
@@ -74,20 +99,14 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
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;
-}
-''')
+ visit_type_%(name)s_fields(m, obj, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ }
+''',
+ name=full_name)
pop_indent()
- pop_indent()
ret += mcgen('''
/* Always call end_struct if start_struct succeeded. */
visit_end_struct(m, &err);
@@ -98,7 +117,9 @@ if (!err) {
return ret
def generate_visit_struct(name, members):
- ret = mcgen('''
+ ret = generate_visit_struct_fields(name, "", members)
+
+ ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
{
@@ -157,9 +178,17 @@ def generate_visit_union(expr):
members = expr['data']
base = 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]
+ ret += generate_visit_struct_fields(name, "", base_fields)
+
ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
@@ -179,23 +208,34 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
push_indent()
if base:
- struct = find_struct(base)
- push_indent()
- ret += generate_visit_struct_fields("", struct['data'])
- pop_indent()
+ ret += mcgen('''
+ visit_type_%(name)s_fields(m, obj, &err);
+''',
+ name=name)
pop_indent()
ret += mcgen('''
- visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+ visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
if (!err) {
switch ((*obj)->kind) {
''',
- name=name)
+ name=name, type="type" if not discriminator else discriminator)
for key in members:
+ if not discriminator:
+ fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);'
+ else:
+ fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err);
+ if (!err) {
+ visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_implicit_struct(m, &err);
+ }'''
+
ret += mcgen('''
case %(abbrev)s_KIND_%(enum)s:
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+ ''' + fmt + '''
break;
''',
abbrev = de_camel_case(name).upper(),
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
2013-07-23 13:03 ` [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator Kevin Wolf
@ 2013-07-26 13:40 ` Eric Blake
2013-07-26 15:01 ` Kevin Wolf
2013-07-26 19:16 ` [Qemu-devel] [PATCH v2 07/17] " Kevin Wolf
1 sibling, 1 reply; 61+ messages in thread
From: Eric Blake @ 2013-07-26 13:40 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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" }
MUCH nicer!
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/qapi-code-gen.txt | 22 +++++++++++++
> scripts/qapi-types.py | 6 ++++
> scripts/qapi-visit.py | 86 ++++++++++++++++++++++++++++++++++++--------------
> 3 files changed, 91 insertions(+), 23 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 555ca66..c187fda 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -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.
> +The above example can then be modified as follows:
> +
> + { 'type': 'BlockdevCommonOptions',
> + 'data': { 'driver': 'str' 'readonly': 'bool' } }
Missing a comma.
> +++ 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
>
> 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...
> ret += generate_struct_fields(struct['data'])
> + else:
> + assert not discriminator
>
> ret += mcgen('''
> };
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index db6fa44..cd33e44 100644
> --- a/scripts/qapi-visit.py
> @@ -157,9 +178,17 @@ def generate_visit_union(expr):
> members = expr['data']
>
> base = 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:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
2013-07-26 13:40 ` Eric Blake
@ 2013-07-26 15:01 ` Kevin Wolf
2013-07-26 15:13 ` Eric Blake
0 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 15:01 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 26.07.2013 um 15:40 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > 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" }
>
> MUCH nicer!
>
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > docs/qapi-code-gen.txt | 22 +++++++++++++
> > scripts/qapi-types.py | 6 ++++
> > scripts/qapi-visit.py | 86 ++++++++++++++++++++++++++++++++++++--------------
> > 3 files changed, 91 insertions(+), 23 deletions(-)
>
> >
> > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> > index 555ca66..c187fda 100644
> > --- a/docs/qapi-code-gen.txt
> > +++ b/docs/qapi-code-gen.txt
> > @@ -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.
There's nothing to add to this, I would certainly support such a future
patch.
> > +The above example can then be modified as follows:
> > +
> > + { 'type': 'BlockdevCommonOptions',
> > + 'data': { 'driver': 'str' 'readonly': 'bool' } }
>
> Missing a comma.
>
> > +++ 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
> >
> > 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...
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('''
};
> I think my findings are easy fixes; so I'm okay if you fix them and then
> add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks, Eric.
Kevin
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator
2013-07-26 15:01 ` Kevin Wolf
@ 2013-07-26 15:13 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 15:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]
On 07/26/2013 09:01 AM, Kevin Wolf wrote:
>>> 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...
>
> 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...
>
> 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
Yes, that makes more sense.
>> I think my findings are easy fixes; so I'm okay if you fix them and then
>> add:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
Which means this is indeed a valid review.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH v2 07/17] qapi: Flat unions with arbitrary discriminator
2013-07-23 13:03 ` [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator Kevin Wolf
2013-07-26 13:40 ` Eric Blake
@ 2013-07-26 19:16 ` Kevin Wolf
2013-07-26 19:36 ` Eric Blake
1 sibling, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 19:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
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.
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index f6f8d33..11f19cf 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -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
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 07bd311..84d46fb 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -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('''
};
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index db6fa44..87d5f15 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,8 +17,30 @@ import os
import getopt
import errno
-def generate_visit_struct_fields(field_prefix, members):
+def generate_visit_struct_fields(name, field_prefix, fn_prefix, members):
+ substructs = []
ret = ''
+ full_name = name if not fn_prefix else "%s_%s" % (name, fn_prefix)
+
+ for argname, argentry, optional, structured in parse_args(members):
+ if structured:
+ if not fn_prefix:
+ nested_fn_prefix = argname
+ else:
+ nested_fn_prefix = "%s_%s" % (fn_prefix, argname)
+
+ nested_field_prefix = "%s%s." % (field_prefix, argname)
+ ret += generate_visit_struct_fields(name, nested_field_prefix,
+ nested_fn_prefix, argentry)
+
+ ret += mcgen('''
+
+static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
+{
+ Error *err = NULL;
+''',
+ name=name, full_name=full_name)
+ push_indent()
for argname, argentry, optional, structured in parse_args(members):
if optional:
@@ -31,7 +53,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
push_indent()
if structured:
- ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
+ ret += generate_visit_struct_body(full_name, argname, argentry)
else:
ret += mcgen('''
visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
@@ -47,6 +69,12 @@ visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s",
visit_end_optional(m, &err);
''')
+ pop_indent()
+ ret += mcgen('''
+
+ error_propagate(errp, err);
+}
+''')
return ret
@@ -56,8 +84,9 @@ if (!error_is_set(errp)) {
''')
push_indent()
+ full_name = name if not field_prefix else "%s_%s" % (field_prefix, name)
+
if len(field_prefix):
- field_prefix = field_prefix + "."
ret += mcgen('''
Error **errp = &err; /* from outer scope */
Error *err = NULL;
@@ -74,20 +103,14 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
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;
-}
-''')
+ visit_type_%(name)s_fields(m, obj, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ }
+''',
+ name=full_name)
pop_indent()
- pop_indent()
ret += mcgen('''
/* Always call end_struct if start_struct succeeded. */
visit_end_struct(m, &err);
@@ -98,7 +121,9 @@ if (!err) {
return ret
def generate_visit_struct(name, members):
- ret = mcgen('''
+ ret = generate_visit_struct_fields(name, "", "", members)
+
+ ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
{
@@ -157,9 +182,17 @@ def generate_visit_union(expr):
members = expr['data']
base = 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]
+ ret += generate_visit_struct_fields(name, "", "", base_fields)
+
ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
@@ -179,23 +212,34 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
push_indent()
if base:
- struct = find_struct(base)
- push_indent()
- ret += generate_visit_struct_fields("", struct['data'])
- pop_indent()
+ ret += mcgen('''
+ visit_type_%(name)s_fields(m, obj, &err);
+''',
+ name=name)
pop_indent()
ret += mcgen('''
- visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+ visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
if (!err) {
switch ((*obj)->kind) {
''',
- name=name)
+ name=name, type="type" if not discriminator else discriminator)
for key in members:
+ if not discriminator:
+ fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);'
+ else:
+ fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err);
+ if (!err) {
+ visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_implicit_struct(m, &err);
+ }'''
+
ret += mcgen('''
case %(abbrev)s_KIND_%(enum)s:
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+ ''' + fmt + '''
break;
''',
abbrev = de_camel_case(name).upper(),
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/17] qapi: Flat unions with arbitrary discriminator
2013-07-26 19:16 ` [Qemu-devel] [PATCH v2 07/17] " Kevin Wolf
@ 2013-07-26 19:36 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 19:36 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]
On 07/26/2013 01:16 PM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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.
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;
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object()
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (6 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 15:13 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types Kevin Wolf
` (9 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
This allows to just look at the next element without actually consuming
it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/qmp-input-visitor.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 59c5cac..70864a1 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (7 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 08/18] qapi: Add consume argument to qmp_input_get_object() Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 15:26 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions Kevin Wolf
` (8 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi.py | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index baf1321..3a54c7f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -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)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (8 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 09/18] qapi.py: Maintain a list of union types Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 15:42 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level Kevin Wolf
` (7 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
The discriminator for anonymous unions is the data type. This allows to
have a union type that allows both of these:
{ 'file': 'my_existing_block_device_id' }
{ 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }
Unions like this are specified in the schema with an empty dict as
discriminator. For this example you could take:
{ 'union': 'BlockRef',
'discriminator': {},
'data': { 'definition': 'BlockOptions'
'reference': 'str' } }
{ 'type': 'ExampleObject',
'data: { 'file': 'BlockRef' } }
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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(+)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c187fda..ee8a13e 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -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
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 9124649..d0bbc7c 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -44,6 +44,7 @@ typedef enum {
QTYPE_QFLOAT,
QTYPE_QBOOL,
QTYPE_QERROR,
+ QTYPE_MAX,
} qtype_code;
struct QObject;
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 5c1297f..f3fa420 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,6 +32,8 @@ struct Visitor
void (*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);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index bd24f85..48a2a2e 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -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);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 9b4d51b..d6a4012 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -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)
{
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 70864a1..bf42c04 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -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);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 19085a1..6cf2511 100644
--- a/qobject/qjson.c
+++ 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();
}
}
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index db2f533..57dff53 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -157,6 +157,40 @@ typedef enum %(name)s
return lookup_decl + enum_decl
+def generate_anon_union_qtypes(expr):
+
+ name = expr['union']
+ members = expr['data']
+
+ ret = mcgen('''
+const int %(name)s_qtypes[QTYPE_MAX] = {
+''',
+ name=name)
+
+ for key in members:
+ qapi_type = members[key]
+ if builtin_type_qtypes.has_key(qapi_type):
+ qtype = builtin_type_qtypes[qapi_type]
+ elif find_struct(qapi_type):
+ qtype = "QTYPE_QDICT"
+ elif find_union(qapi_type):
+ qtype = "QTYPE_QDICT"
+ else:
+ assert False, "Invalid anonymous union member"
+
+ ret += mcgen('''
+ [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
+''',
+ qtype = qtype,
+ abbrev = de_camel_case(name).upper(),
+ enum = c_fun(de_camel_case(key),False).upper())
+
+ ret += mcgen('''
+};
+''')
+ return ret
+
+
def generate_union(expr):
name = expr['union']
@@ -196,6 +230,12 @@ struct %(name)s
ret += mcgen('''
};
''')
+ if discriminator == {}:
+ ret += mcgen('''
+extern const int %(name)s_qtypes[];
+''',
+ name=name)
+
return ret
@@ -348,6 +388,8 @@ for expr in exprs:
ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
+ if expr.get('discriminator') == {}:
+ fdef.write(generate_anon_union_qtypes(expr))
else:
continue
fdecl.write(ret)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index cd33e44..28539e2 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,49 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
''',
name=name)
+def generate_visit_anon_union(name, members):
+ ret = mcgen('''
+
+void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+{
+ Error *err = NULL;
+
+ if (!error_is_set(errp)) {
+ visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err);
+ visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err);
+ switch ((*obj)->kind) {
+''',
+ name=name)
+
+ for key in members:
+ assert (members[key] in builtin_types
+ or find_struct(members[key])
+ or find_union(members[key])), "Invalid anonymous union member"
+
+ ret += mcgen('''
+ case %(abbrev)s_KIND_%(enum)s:
+ visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
+ break;
+''',
+ abbrev = de_camel_case(name).upper(),
+ enum = c_fun(de_camel_case(key),False).upper(),
+ c_type = type_name(members[key]),
+ c_name = c_fun(key))
+
+ ret += mcgen('''
+ default:
+ abort();
+ }
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_implicit_struct(m, &err);
+ }
+}
+''')
+
+ return ret
+
+
def generate_visit_union(expr):
name = expr['union']
@@ -180,6 +223,10 @@ def generate_visit_union(expr):
base = expr.get('base')
discriminator = expr.get('discriminator')
+ if discriminator == {}:
+ assert not base
+ return generate_visit_anon_union(name, members)
+
ret = generate_visit_enum('%sKind' % name, members.keys())
if base:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3a54c7f..38c808e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -17,6 +17,21 @@ builtin_types = [
'uint8', 'uint16', 'uint32', 'uint64'
]
+builtin_type_qtypes = {
+ 'str': 'QTYPE_QSTRING',
+ 'int': 'QTYPE_QINT',
+ 'number': 'QTYPE_QFLOAT',
+ 'bool': 'QTYPE_QBOOL',
+ 'int8': 'QTYPE_QINT',
+ 'int16': 'QTYPE_QINT',
+ 'int32': 'QTYPE_QINT',
+ 'int64': 'QTYPE_QINT',
+ 'uint8': 'QTYPE_QINT',
+ 'uint16': 'QTYPE_QINT',
+ 'uint32': 'QTYPE_QINT',
+ 'uint64': 'QTYPE_QINT',
+}
+
def tokenize(data):
while len(data):
ch = data[0]
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions
2013-07-23 13:03 ` [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions Kevin Wolf
@ 2013-07-26 15:42 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 15:42 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> The discriminator for anonymous unions is the data type. This allows to
> have a union type that allows both of these:
>
> { 'file': 'my_existing_block_device_id' }
> { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }
>
> Unions like this are specified in the schema with an empty dict as
> discriminator. For this example you could take:
>
> { 'union': 'BlockRef',
> 'discriminator': {},
> 'data': { 'definition': 'BlockOptions'
Missing comma.
> 'reference': 'str' } }
> { 'type': 'ExampleObject',
> 'data: { 'file': 'BlockRef' } }
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> +++ b/docs/qapi-code-gen.txt
> @@ -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
s/disriminator/discriminator/
> +generated C code. Anonymous unions cannot have a base type.
> +
> + { 'union': 'BlockRef',
> + 'discriminator': {},
> + 'data': { 'definition': 'BlockdevOptions'
Missing comma.
> +++ 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:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (9 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 10/18] qapi: Anonymous unions Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:00 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 12/18] QemuOpts: Add qemu_opt_unset() Kevin Wolf
` (6 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
---
block.c | 7 +++++++
blockdev.c | 20 ++++++++++----------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 03fe754..d2cd9ed 100644
--- a/block.c
+++ b/block.c
@@ -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);
}
diff --git a/blockdev.c b/blockdev.c
index c5abd65..8ff8ed3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level
2013-07-23 13:03 ` [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level Kevin Wolf
@ 2013-07-26 16:00 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 602 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> block.c | 7 +++++++
> blockdev.c | 20 ++++++++++----------
> 2 files changed, 17 insertions(+), 10 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 12/18] QemuOpts: Add qemu_opt_unset()
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (10 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 11/18] block: Allow "driver" option on the top level Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:04 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP Kevin Wolf
` (5 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/qemu/option.h | 1 +
util/qemu-option.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index a83c700..13f5e72 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -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);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index e0ef426..5d686c8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -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)
{
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (11 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 12/18] QemuOpts: Add qemu_opt_unset() Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:10 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options Kevin Wolf
` (4 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
---
blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 8ff8ed3..5403188 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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",
},{
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-23 13:03 ` [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP Kevin Wolf
@ 2013-07-26 16:10 ` Eric Blake
2013-07-26 16:26 ` Kevin Wolf
2013-07-26 19:35 ` Benoît Canet
0 siblings, 2 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:10 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, Benoît Canet, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> 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?
> @@ -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}.
> io_limits.bps[BLOCK_IO_LIMIT_READ] =
> - qemu_opt_get_number(opts, "bps_rd", 0);
> + qemu_opt_get_number(opts, "throttling.bps-read", 0);
> io_limits.bps[BLOCK_IO_LIMIT_WRITE] =
> - qemu_opt_get_number(opts, "bps_wr", 0);
> + qemu_opt_get_number(opts, "throttling.bps-write", 0);
Thank you for spelling out the names; QMP doesn't need the abbreviations
that the command line has.
> +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?
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 16:10 ` Eric Blake
@ 2013-07-26 16:26 ` Kevin Wolf
2013-07-26 16:44 ` Eric Blake
2013-07-26 19:35 ` Benoît Canet
1 sibling, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 16:26 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, Benoît Canet, qemu-devel, stefanha, lcapitulino
Am 26.07.2013 um 18:10 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > 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 <kwolf@redhat.com>
> > ---
> > 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?
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.)
> > @@ -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}.
>
> > io_limits.bps[BLOCK_IO_LIMIT_READ] =
> > - qemu_opt_get_number(opts, "bps_rd", 0);
> > + qemu_opt_get_number(opts, "throttling.bps-read", 0);
> > io_limits.bps[BLOCK_IO_LIMIT_WRITE] =
> > - qemu_opt_get_number(opts, "bps_wr", 0);
> > + qemu_opt_get_number(opts, "throttling.bps-write", 0);
>
> Thank you for spelling out the names; QMP doesn't need the abbreviations
> that the command line has.
>
> > +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
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 16:26 ` Kevin Wolf
@ 2013-07-26 16:44 ` Eric Blake
2013-07-26 16:54 ` Kevin Wolf
0 siblings, 1 reply; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, Benoît Canet, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]
On 07/26/2013 10:26 AM, Kevin Wolf wrote:
>> 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.
>
> 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
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 16:44 ` Eric Blake
@ 2013-07-26 16:54 ` Kevin Wolf
2013-07-26 17:01 ` Eric Blake
0 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 16:54 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, Benoît Canet, qemu-devel, stefanha, lcapitulino
Am 26.07.2013 um 18:44 hat Eric Blake geschrieben:
> On 07/26/2013 10:26 AM, Kevin Wolf wrote:
>
> >> 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.
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?
> > 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.
> > 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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 16:54 ` Kevin Wolf
@ 2013-07-26 17:01 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 17:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, Benoît Canet, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
On 07/26/2013 10:54 AM, Kevin Wolf wrote:
>>> 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?
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 16:10 ` Eric Blake
2013-07-26 16:26 ` Kevin Wolf
@ 2013-07-26 19:35 ` Benoît Canet
2013-07-26 19:54 ` Eric Blake
1 sibling, 1 reply; 61+ messages in thread
From: Benoît Canet @ 2013-07-26 19:35 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, armbru, qemu-devel, stefanha, lcapitulino
> 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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP
2013-07-26 19:35 ` Benoît Canet
@ 2013-07-26 19:54 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 19:54 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]
On 07/26/2013 01:35 PM, Benoît Canet wrote:
>> 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.]
>
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (12 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:17 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 15/18] blockdev: Rename 'readonly' option to 'read-only' Kevin Wolf
` (3 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
This is what QMP wants to use. The options haven't been enabled in any
release yet, so we're still free to change them.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..dba9771 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options
2013-07-23 13:03 ` [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options Kevin Wolf
@ 2013-07-26 16:17 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 528 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 15/18] blockdev: Rename 'readonly' option to 'read-only'
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (13 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 14/18] qcow2: Use dashes instead of underscores in options Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:20 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option Kevin Wolf
` (2 subsequent siblings)
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Option name cleanup before it becomes a QMP API.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 5403188..3b05e29 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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",
},{
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (14 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 15/18] blockdev: Rename 'readonly' option to 'read-only' Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:30 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 17/18] Implement qdict_flatten() Kevin Wolf
2013-07-23 13:03 ` [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command Kevin Wolf
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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:
-drive file=x,cache=none,cache.no-flush=true
-drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3b05e29..ef55b1a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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,
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option
2013-07-23 13:03 ` [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option Kevin Wolf
@ 2013-07-26 16:30 ` Eric Blake
0 siblings, 0 replies; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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:
>
> -drive file=x,cache=none,cache.no-flush=true
> -drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 47 insertions(+), 10 deletions(-)
>
> +++ b/blockdev.c
> @@ -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.
> @@ -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.]
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (15 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 16/18] blockdev: Split up 'cache' option Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 16:40 ` Eric Blake
2013-07-23 13:03 ` [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command Kevin Wolf
17 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
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 <kwolf@redhat.com>
---
include/qapi/qmp/qdict.h | 1 +
qobject/qdict.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 685b2e3..d6855d1 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -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 */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index ed381f9..108db47 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -476,3 +476,53 @@ static void qdict_destroy_obj(QObject *obj)
g_free(qdict);
}
+
+static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
+{
+ QObject *value;
+ const QDictEntry *entry, *next;
+ const char *new_key;
+ bool delete;
+
+ entry = qdict_first(qdict);
+
+ while (entry != NULL) {
+
+ next = qdict_next(qdict, entry);
+ value = qdict_entry_value(entry);
+ new_key = NULL;
+ delete = false;
+
+ if (prefix) {
+ qobject_incref(value);
+ new_key = g_strdup_printf("%s.%s", prefix, entry->key);
+ qdict_put_obj(target, new_key, value);
+ delete = true;
+ }
+
+ if (qobject_type(value) == QTYPE_QDICT) {
+ 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);
+ }
+
+ entry = next;
+ }
+}
+
+/**
+ * qdict_flatten(): For each nested QDict with key x, all fields with key y
+ * are moved to this QDict and their key is renamed to "x.y". This operation
+ * is applied recursively for nested QDicts.
+ */
+void qdict_flatten(QDict *qdict)
+{
+ qdict_do_flatten(qdict, qdict, NULL);
+}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()
2013-07-23 13:03 ` [Qemu-devel] [PATCH 17/18] Implement qdict_flatten() Kevin Wolf
@ 2013-07-26 16:40 ` Eric Blake
2013-07-26 17:03 ` Kevin Wolf
0 siblings, 1 reply; 61+ messages in thread
From: Eric Blake @ 2013-07-26 16:40 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> include/qapi/qmp/qdict.h | 1 +
> qobject/qdict.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
> + while (entry != NULL) {
> +
> + next = qdict_next(qdict, entry);
next points to a position in the unmodified qdict...
> + 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) {
> + 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...
> + }
> +
> + 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:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 17/18] Implement qdict_flatten()
2013-07-26 16:40 ` Eric Blake
@ 2013-07-26 17:03 ` Kevin Wolf
0 siblings, 0 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 17:03 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 26.07.2013 um 18:40 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > 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 <kwolf@redhat.com>
> > ---
> > include/qapi/qmp/qdict.h | 1 +
> > qobject/qdict.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 51 insertions(+)
>
>
> > + while (entry != NULL) {
> > +
> > + next = qdict_next(qdict, entry);
>
> next points to a position in the unmodified qdict...
>
> > + 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) {
> > + 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...
>
> > + }
> > +
> > + 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
^ permalink raw reply [flat|nested] 61+ messages in thread
* [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-23 13:03 [Qemu-devel] [PATCH 00/18] 'blockdev-add' QMP command Kevin Wolf
` (16 preceding siblings ...)
2013-07-23 13:03 ` [Qemu-devel] [PATCH 17/18] Implement qdict_flatten() Kevin Wolf
@ 2013-07-23 13:03 ` Kevin Wolf
2013-07-26 17:45 ` Eric Blake
2013-08-30 7:41 ` Wenchao Xia
17 siblings, 2 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-07-23 13:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 45 +++++++++
qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 26 +++++
3 files changed, 364 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index ef55b1a..214173b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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;
diff --git a/qapi-schema.json b/qapi-schema.json
index 592bb9c..bfe6d32 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -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
+# @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' } }
+
+##
+# @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' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 65a9e26..96b0956 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -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
--
1.8.1.4
^ permalink raw reply related [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-23 13:03 ` [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command Kevin Wolf
@ 2013-07-26 17:45 ` Eric Blake
2013-07-26 18:14 ` Kevin Wolf
2013-08-30 7:41 ` Wenchao Xia
1 sibling, 1 reply; 61+ messages in thread
From: Eric Blake @ 2013-07-26 17:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 13093 bytes --]
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?
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 45 +++++++++
> qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 26 +++++
> 3 files changed, 364 insertions(+)
> @@ -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;
> + }
[1]
> +
> + /* 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/
> + * 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.
> + 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?
> +++ b/qapi-schema.json
> @@ -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
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.)
> +{ '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.
> +
> +##
> +# @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)
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]
> +#
> +# 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?
> +{ 'type': 'BlockdevOptionsFile',
> + 'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsFile
copy-and-paste; this should be BlockdevOptionsNBD
> +#
> +# 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
We probably ought to polish that before actually committing to this QMP
interface.
> +# @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'?
> +{ '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.
> +#
> +# 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).
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat
Copy-and-paste, should be BlockdevOptionsQcow2
> +#
> +# 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
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 :)
> +
> +##
> +# @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'?
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.
> +# 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'
> + } }
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.
> +
> +##
> +# @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'
missing comma. If Markus' re-write of the qapi parser gets applied
first, this wouldn't have happened :)
> + '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.
> +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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 61+ messages in thread* Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-26 17:45 ` Eric Blake
@ 2013-07-26 18:14 ` Kevin Wolf
2013-07-30 17:44 ` Luiz Capitulino
0 siblings, 1 reply; 61+ messages in thread
From: Kevin Wolf @ 2013-07-26 18:14 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> 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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-26 18:14 ` Kevin Wolf
@ 2013-07-30 17:44 ` Luiz Capitulino
2013-07-31 8:09 ` Kevin Wolf
0 siblings, 1 reply; 61+ messages in thread
From: Luiz Capitulino @ 2013-07-30 17:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On Fri, 26 Jul 2013 20:14:06 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> > 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?
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-30 17:44 ` Luiz Capitulino
@ 2013-07-31 8:09 ` Kevin Wolf
0 siblings, 0 replies; 61+ messages in thread
From: Kevin Wolf @ 2013-07-31 8:09 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel, stefanha, armbru
Am 30.07.2013 um 19:44 hat Luiz Capitulino geschrieben:
> On Fri, 26 Jul 2013 20:14:06 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
> > Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> > > 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?
Correct. Patches 1-17 are in master now, and blockdev-add itself will
be reposted during the 1.7 cycle.
Kevin
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command
2013-07-23 13:03 ` [Qemu-devel] [PATCH 18/18] blockdev: 'blockdev-add' QMP command Kevin Wolf
2013-07-26 17:45 ` Eric Blake
@ 2013-08-30 7:41 ` Wenchao Xia
1 sibling, 0 replies; 61+ messages in thread
From: Wenchao Xia @ 2013-08-30 7:41 UTC (permalink / raw)
To: Kevin Wolf; +Cc: lcapitulino, qemu-devel, stefanha, armbru
于 2013-7-23 21:03, Kevin Wolf 写道:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 45 +++++++++
> qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 26 +++++
> 3 files changed, 364 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index ef55b1a..214173b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -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;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 592bb9c..bfe6d32 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -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
> +# @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
....
> +##
> +# @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' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 65a9e26..96b0956 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -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
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 61+ messages in thread