* [Qemu-devel] [RFC PATCH 01/11] qapi-types.py: Split off generate_struct_fields()
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 11:45 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions Kevin Wolf
` (10 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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] 32+ messages in thread* [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 01/11] qapi-types.py: Split off generate_struct_fields() Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 11:57 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 03/11] qapi-visit.py: Split off generate_visit_struct_fields() Kevin Wolf
` (9 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-types.py | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e1239e1..960065b 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -157,7 +157,16 @@ typedef enum %(name)s
return lookup_decl + enum_decl
-def generate_union(name, typeinfo):
+def generate_union(expr):
+
+ name = expr['union']
+ typeinfo = expr['data']
+
+ if expr.has_key('base'):
+ base = expr['base']
+ else:
+ base = None
+
ret = mcgen('''
struct %(name)s
{
@@ -176,6 +185,13 @@ struct %(name)s
ret += mcgen('''
};
+''')
+
+ if base:
+ struct = find_struct(base)
+ ret += generate_struct_fields(struct['data'])
+
+ ret += mcgen('''
};
''')
@@ -359,7 +375,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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-11 11:57 ` Eric Blake
2013-07-11 12:46 ` Eric Blake
0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2013-07-11 11:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]
On 07/09/2013 03:53 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.
Is it worth listing an example of intended use in the commit message?
If I understand correctly, then this qapi-schema.json file:
{ 'type': 'Base',
'data': { 'main': 'str' } }
{ 'type': 'Extra1',
'data': { 'foo': 'str' } }
{ 'type': 'Extra2',
'data': { 'bar': 'str' } }
{ 'union': 'Type',
'base': 'BaseType',
'data': { 'variant1': 'Extra1',
'variant2': 'Extra2' } }
{ 'command': 'test',
'data': { 'arg': 'Type' } }
would then be used over the wire as:
{ "execute": "test",
"arguments": { "arg": { "type": "variant1",
"data": { "main": "hello", "foo": "world" } } } }
{ "execute": "test",
"arguments": { "arg": { "type": "variant2",
"data": { "main": "hi", "bar": "there" } } } }
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
I'm slowly learning python, so my review skills are still weak the more
complicated the changes are; but this seems easy enough to understand.
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions
2013-07-11 11:57 ` Eric Blake
@ 2013-07-11 12:46 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-11 12:46 UTC (permalink / raw)
Cc: Kevin Wolf, armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]
On 07/11/2013 05:57 AM, Eric Blake wrote:
> On 07/09/2013 03:53 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.
>
> Is it worth listing an example of intended use in the commit message?
> If I understand correctly, then this qapi-schema.json file:
>
> { 'type': 'Base',
> 'data': { 'main': 'str' } }
> { 'type': 'Extra1',
> 'data': { 'foo': 'str' } }
> { 'type': 'Extra2',
> 'data': { 'bar': 'str' } }
> { 'union': 'Type',
> 'base': 'BaseType',
> 'data': { 'variant1': 'Extra1',
> 'variant2': 'Extra2' } }
> { 'command': 'test',
> 'data': { 'arg': 'Type' } }
>
> would then be used over the wire as:
>
> { "execute": "test",
> "arguments": { "arg": { "type": "variant1",
> "data": { "main": "hello", "foo": "world" } } } }
> { "execute": "test",
> "arguments": { "arg": { "type": "variant2",
> "data": { "main": "hi", "bar": "there" } } } }
I just read patch 6/11, and it looks like I'm a bit off; it looks like
the usage syntax with a base type is actually:
{ "execute": "test",
"arguments": { "arg": { "type": "variant1", "main": "hello",
"data": { "foo": "world" } } } }
{ "execute": "test",
"arguments": { "arg": { "type": "variant2", "main": "hi",
"data": { "bar": "there" } } } }
That is, a union type is automatically granted two top-level keys "type"
and "data", and a base type grants it additional top-level keys; nested
keys under "data" are still tied to just the type listed in the union.
See why a commit message can make a difference in understanding? :)
--
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] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 03/11] qapi-visit.py: Split off generate_visit_struct_fields()
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 01/11] qapi-types.py: Split off generate_struct_fields() Kevin Wolf
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 02/11] qapi-types.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 12:18 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 04/11] qapi-visit.py: Implement 'base' for unions Kevin Wolf
` (8 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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] 32+ messages in thread* [Qemu-devel] [RFC PATCH 04/11] qapi-visit.py: Implement 'base' for unions
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (2 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 03/11] qapi-visit.py: Split off generate_visit_struct_fields() Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 12:21 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs Kevin Wolf
` (7 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-visit.py | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a337d80..3b2e693 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -151,7 +151,16 @@ 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']
+
+ if expr.has_key('base'):
+ base = expr['base']
+ else:
+ base = None
+
ret = generate_visit_enum('%sKind' % name, members.keys())
ret += mcgen('''
@@ -164,14 +173,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 +391,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] 32+ messages in thread* [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (3 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 04/11] qapi-visit.py: Implement 'base' for unions Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 12:41 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator Kevin Wolf
` (6 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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 (parsing flat
namespect QMP union with discriminator)
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs Kevin Wolf
@ 2013-07-11 12:41 ` Eric Blake
2013-07-11 12:51 ` Eric Blake
0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2013-07-11 12:41 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]
On 07/09/2013 03:53 AM, Kevin Wolf wrote:
> These can be used when an embedded struct is parsed and members not
> belonging to the struct may be present in the input (parsing flat
> namespect QMP union with discriminator)
namespect? Not sure if you meant 'namespaced'?
Again, a comment demonstrating in sample QMP on what this is attempting
would be helpful in the commit message. Without looking at later
patches yet, I'm guessing from your description that your intent is to
reach a point where:
{ 'type': 'Extra1',
'data': { 'foo': 'str' } }
{ 'type': 'Extra2',
'data': { 'bar': 'str' } }
{ 'union': 'Type',
'data': { 'variant1': 'Extra1',
'variant2': 'Extra2' } }
{ 'command': 'test',
'data': { 'arg': 'Type' } }
which currently requires this over the wire:
{ "execute": "test",
"arguments": { "arg": { "type": "variant1",
"data": { "foo": "hello" } } } }
can be shortened to:
{ "execute": "test",
"arguments": { "arg": { "type": "variant1", "foo": "hello" } } }
by allowing the struct passed as "arg" to inline the contents of the
union rather than require another nesting level through "data". This
patch sets up the entry points so that a future patch can parse the
struct of arg as an implicit struct, and not choke over the key "foo",
instead of the current visitor of a union type that expects exactly the
two keys "type" and "data".
>
> 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(+)
Although the commit message left me floundering, the code itself seems
reasonable - you are just adding two new entry points to allow a new
type of parse. It remains to later patches to see how the new parsers
are useful, but that doesn't stop me from giving:
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs
2013-07-11 12:41 ` Eric Blake
@ 2013-07-11 12:51 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-11 12:51 UTC (permalink / raw)
Cc: Kevin Wolf, armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 907 bytes --]
On 07/11/2013 06:41 AM, Eric Blake wrote:
> On 07/09/2013 03:53 AM, Kevin Wolf wrote:
>> These can be used when an embedded struct is parsed and members not
>> belonging to the struct may be present in the input (parsing flat
>> namespect QMP union with discriminator)
>
> namespect? Not sure if you meant 'namespaced'?
>
> Again, a comment demonstrating in sample QMP on what this is attempting
> would be helpful in the commit message. Without looking at later
> patches yet,
Now that I've read the commit message of patch 6/11, may I suggest
something along the lines of this wording:
Add a new visitor that allows parsing of an implicit struct, which is
created by inlining the namespace of the nested union 'data' struct into
the top-level struct of the union itself.
--
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] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (4 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 14:16 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 07/11] qapi: Add consume argument to qmp_input_get_object() Kevin Wolf
` (5 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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>
---
scripts/qapi-types.py | 10 +++---
scripts/qapi-visit.py | 91 ++++++++++++++++++++++++++++++++++++---------------
2 files changed, 70 insertions(+), 31 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 960065b..db2f533 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -162,10 +162,8 @@ def generate_union(expr):
name = expr['union']
typeinfo = expr['data']
- if expr.has_key('base'):
- base = expr['base']
- else:
- base = None
+ base = expr.get('base')
+ discriminator = expr.get('discriminator')
ret = mcgen('''
struct %(name)s
@@ -189,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 3b2e693..bff284f 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)
{
@@ -156,13 +177,18 @@ def generate_visit_union(expr):
name = expr['union']
members = expr['data']
- if expr.has_key('base'):
- base = expr['base']
- else:
- base = None
+ struct = None
+ base = expr.get('base')
+ discriminator = expr.get('discriminator')
ret = generate_visit_enum('%sKind' % name, members.keys())
+ if base:
+ struct = find_struct(base)
+ if discriminator:
+ del struct['data'][discriminator]
+ ret += generate_visit_struct_fields(name, "", struct['data'])
+
ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
@@ -182,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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator Kevin Wolf
@ 2013-07-11 14:16 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-11 14:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 4085 bytes --]
On 07/09/2013 03:53 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" }
[prior to reading the patch body]
I assume that the corresponding qapi-schema.json change that matches the
above wire context is:
{ 'type': 'Base',
'data': { 'read-only': 'bool' } }
{ 'type': 'File',
'data': { 'filename': 'str' } }
{ 'union': 'Example',
'base': 'Base',
'discriminator': 'driver',
'data': { 'file': 'File' } }
with the new 'discriminator' element stating that an alternate name
(rather than the default 'type' name) is in use. Do you allow flattened
unions always, or only when 'discriminator' was present?
Aren't you really flattening TWO structs? That is, extending the above,
we are going from:
{ 'command': 'test', 'data': { 'driver': 'Example' } }
which is previously called as:
{ "execute": "test", "arguments": {
"driver": { "type": "file", "read-only": true,
"data": { "filename": "test" } } } }
to the shorter:
{ "execute": "test", "arguments": {
"driver": "file", "read-only": true, "filename": "test" } }
[...now after reading the patch body]
Oh - you ONLY support a discriminator IF you pass a 'base' class, where
the discriminator has to be one of the fields named in the base class,
necessarily of type 'str'. In other words, the qapi-schema.json needs
to look more like:
{ 'type': 'Base',
'data': { 'read-only': 'bool', 'driver': 'str' } }
{ 'type': 'File',
'data': { 'filename': 'str' } }
{ 'union': 'Example',
'base': 'Base',
'discriminator': 'driver',
'data': { 'file': 'File' } }
If I'm off (or even if I'm right), more details in the commit message
about the qapi description that leads to the final QMP wire contents
would be helpful.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 10 +++---
> scripts/qapi-visit.py | 91 ++++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 70 insertions(+), 31 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 960065b..db2f533 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -162,10 +162,8 @@ def generate_union(expr):
> name = expr['union']
> typeinfo = expr['data']
>
> - if expr.has_key('base'):
> - base = expr['base']
> - else:
> - base = None
> + base = expr.get('base')
Should this simplification be squashed into patch 2/11? (Patch 4/11 has
a similar construct that might also be simplified.)
> + discriminator = expr.get('discriminator')
>
> ret = mcgen('''
> struct %(name)s
> @@ -189,7 +187,11 @@ struct %(name)s
>
> if base:
> struct = find_struct(base)
> + if discriminator:
> + del struct['data'][discriminator]
Here my lack of python expertise shows, in what may be a dumb question:
Does find_struct(base) return a reference to the original object (oops -
modifying 'struct' here means that the next call to find_struct(base)
sees a smaller object), or a fully-cloned object (phew - our deletion is
to our local copy, but future callers get a fresh clone that still see
the discriminator)?
Meanwhile, should you assert that the discriminator is of type 'str'?
Actually, I suppose it might be nice to support a discriminator of an
enum type (to document the set of valid strings); but the point still
remains that the discriminator field is probably not going to work as a
bool, integer, or struct type.
--
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] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 07/11] qapi: Add consume argument to qmp_input_get_object()
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (5 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 19:17 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions Kevin Wolf
` (4 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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] 32+ messages in thread* [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (6 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 07/11] qapi: Add consume argument to qmp_input_get_object() Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 19:47 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten() Kevin Wolf
` (3 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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>
---
include/qapi/qmp/qobject.h | 1 +
include/qapi/visitor-impl.h | 2 ++
include/qapi/visitor.h | 3 +++
qapi/qapi-visit-core.c | 9 +++++++++
qapi/qmp-input-visitor.c | 14 ++++++++++++++
qobject/qjson.c | 2 ++
scripts/qapi-types.py | 42 ++++++++++++++++++++++++++++++++++++++++
scripts/qapi-visit.py | 47 +++++++++++++++++++++++++++++++++++++++++++++
scripts/qapi.py | 28 +++++++++++++++++++++++++++
9 files changed, 148 insertions(+)
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 bff284f..25ef4d7 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']
@@ -181,6 +224,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 baf1321..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]
@@ -105,6 +120,7 @@ def parse_schema(fp):
if expr_eval.has_key('enum'):
add_enum(expr_eval['enum'])
elif expr_eval.has_key('union'):
+ add_union(expr_eval)
add_enum('%sKind' % expr_eval['union'])
elif expr_eval.has_key('type'):
add_struct(expr_eval)
@@ -188,6 +204,7 @@ def type_name(name):
enum_types = []
struct_types = []
+union_types = []
def add_struct(definition):
global struct_types
@@ -200,6 +217,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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions Kevin Wolf
@ 2013-07-11 19:47 ` Eric Blake
2013-07-12 8:55 ` Kevin Wolf
0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2013-07-11 19:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]
On 07/09/2013 03:53 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'
> 'reference': 'str' } }
> { 'type': 'ExampleObject',
> 'data: { 'file': 'BlockRef' } }
Yay - a commit message that shows the new QMP wire format, and the
qapi-schema.json that generated it.
[Without reading the patch yet]
I take it the 'data' of such a union must be completely distinguishable
by type - the visitor for the 'file' key tries each subtype in turn
('BlockOptions' or 'str') until one does not have a parse error. And
this changes the earlier patch that required 'discriminator' to be used
only when 'base' was also present. Can it distinguish between two
structs, or is the union only allowed to have one struct option and all
other options are primitive types? Should we consider the use of arrays
as a union member type?
Hmm, I wonder if this new anonymous union can be useful in helping us
write the self-description of introspection in Amos' patches.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/qapi/qmp/qobject.h | 1 +
> include/qapi/visitor-impl.h | 2 ++
> include/qapi/visitor.h | 3 +++
> qapi/qapi-visit-core.c | 9 +++++++++
> qapi/qmp-input-visitor.c | 14 ++++++++++++++
> qobject/qjson.c | 2 ++
> scripts/qapi-types.py | 42 ++++++++++++++++++++++++++++++++++++++++
> scripts/qapi-visit.py | 47 +++++++++++++++++++++++++++++++++++++++++++++
> scripts/qapi.py | 28 +++++++++++++++++++++++++++
> 9 files changed, 148 insertions(+)
> +++ b/scripts/qapi-visit.py
> @@ -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))
Inconsistent spacing around '='
> +
> + ret += mcgen('''
> + default:
> + abort();
Does this mean I can cause qemu to abort if I pass bogus information on
the wire? Using your commit message example,
{ 'file': false }
would hit the default case, right?
> +++ 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',
What happens if I try to write a union in qapi-schema.json that has both
a 'int8' and 'uint32' branch? Since both of those resolve to the
QTYPE_QINT visitor, does that end up causing the C code to generate a
switch statement with duplicate labels?
> +}
> +
> def tokenize(data):
> while len(data):
> ch = data[0]
> @@ -105,6 +120,7 @@ def parse_schema(fp):
> if expr_eval.has_key('enum'):
> add_enum(expr_eval['enum'])
> elif expr_eval.has_key('union'):
> + add_union(expr_eval)
> add_enum('%sKind' % expr_eval['union'])
> elif expr_eval.has_key('type'):
> add_struct(expr_eval)
> @@ -188,6 +204,7 @@ def type_name(name):
>
> enum_types = []
> struct_types = []
> +union_types = []
Is it worth splitting the tracking of the union_types of
qapi-schema.json into an independent patch?
Does it make sense to allow a union type (possibly only an anonymous
union type) in place of a struct type as a top-level type reference?
That is, just as we now allow { 'command':'foo', 'data': 'StructType' },
would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?
--
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions
2013-07-11 19:47 ` Eric Blake
@ 2013-07-12 8:55 ` Kevin Wolf
2013-07-12 14:15 ` Eric Blake
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-12 8:55 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 11.07.2013 um 21:47 hat Eric Blake geschrieben:
> On 07/09/2013 03:53 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'
> > 'reference': 'str' } }
> > { 'type': 'ExampleObject',
> > 'data: { 'file': 'BlockRef' } }
>
> Yay - a commit message that shows the new QMP wire format, and the
> qapi-schema.json that generated it.
Yeah, sorry about these missing examples and detailed documentation. I
wanted to get an RFC out quickly and probably underestimated how
important it is even for reviewing an RFC.
However, I think keeping this information only in the commit messages
isn't right either. Do we have any of the schema syntax documented
anywhere? Should I create something in docs/(specs/)?
> [Without reading the patch yet]
> I take it the 'data' of such a union must be completely distinguishable
> by type - the visitor for the 'file' key tries each subtype in turn
> ('BlockOptions' or 'str') until one does not have a parse error.
Not exactly. It asks the visitor for the type of the next item and then
uses a lookup table to get the right discriminator enum value for it.
> And
> this changes the earlier patch that required 'discriminator' to be used
> only when 'base' was also present. Can it distinguish between two
> structs, or is the union only allowed to have one struct option and all
> other options are primitive types? Should we consider the use of arrays
> as a union member type?
As the types it uses for lookup are QObject types and all structs map to
QDict, only one struct is allowed.
This makes sense anyway, because allowing different structs would mean
that you potentially get ambiguities when structs look alike (perhaps
only differing in optional fields or something like that).
As soon as we find a good use case for anonymous unions of two structs,
I might change my opinion, though. ;-)
Arrays are a logical extension of this, though I don't have any use for
them now. I wouldn't expect them to be too hard to implement.
> Hmm, I wonder if this new anonymous union can be useful in helping us
> write the self-description of introspection in Amos' patches.
Hm, what's the exact problem there? I know I read that someone (Paolo?)
wrote that it was impossible, but I didn't immediately understand why
and didn't feel like thinking a lot about either.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > include/qapi/qmp/qobject.h | 1 +
> > include/qapi/visitor-impl.h | 2 ++
> > include/qapi/visitor.h | 3 +++
> > qapi/qapi-visit-core.c | 9 +++++++++
> > qapi/qmp-input-visitor.c | 14 ++++++++++++++
> > qobject/qjson.c | 2 ++
> > scripts/qapi-types.py | 42 ++++++++++++++++++++++++++++++++++++++++
> > scripts/qapi-visit.py | 47 +++++++++++++++++++++++++++++++++++++++++++++
> > scripts/qapi.py | 28 +++++++++++++++++++++++++++
> > 9 files changed, 148 insertions(+)
>
> > +++ b/scripts/qapi-visit.py
> > @@ -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))
>
> Inconsistent spacing around '='
Thanks, will fix. (It's just copied from generate_visit_union(), though)
> > +
> > + ret += mcgen('''
> > + default:
> > + abort();
>
> Does this mean I can cause qemu to abort if I pass bogus information on
> the wire? Using your commit message example,
>
> { 'file': false }
>
> would hit the default case, right?
No, it wouldn't, but that case fails in an interesting way anyway. :-)
We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which
isn't defined and therefore 0. This happens to be the enum value for
BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message
like "invalid argument type: got bool, expected dict".
> > +++ 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',
>
> What happens if I try to write a union in qapi-schema.json that has both
> a 'int8' and 'uint32' branch? Since both of those resolve to the
> QTYPE_QINT visitor, does that end up causing the C code to generate a
> switch statement with duplicate labels?
No, the C code gets a lookup table having multiple initialisers for
QTYPE_INT, which means that the last one wins (C11 6.7.9, paragraph 19).
It should be obvious that you're not supposed to do that, so I don't
think it's necessary to assert it in the generator.
> > +}
> > +
> > def tokenize(data):
> > while len(data):
> > ch = data[0]
> > @@ -105,6 +120,7 @@ def parse_schema(fp):
> > if expr_eval.has_key('enum'):
> > add_enum(expr_eval['enum'])
> > elif expr_eval.has_key('union'):
> > + add_union(expr_eval)
> > add_enum('%sKind' % expr_eval['union'])
> > elif expr_eval.has_key('type'):
> > add_struct(expr_eval)
> > @@ -188,6 +204,7 @@ def type_name(name):
> >
> > enum_types = []
> > struct_types = []
> > +union_types = []
>
> Is it worth splitting the tracking of the union_types of
> qapi-schema.json into an independent patch?
If you prefer, I can do that. In fact, I considered it myself and didn't
have a strong opinion either way, so I just did something.
> Does it make sense to allow a union type (possibly only an anonymous
> union type) in place of a struct type as a top-level type reference?
> That is, just as we now allow { 'command':'foo', 'data': 'StructType' },
> would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?
Sounds sensible to me. But do we have a use case for it?
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions
2013-07-12 8:55 ` Kevin Wolf
@ 2013-07-12 14:15 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-12 14:15 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 5998 bytes --]
On 07/12/2013 02:55 AM, Kevin Wolf wrote:
> Am 11.07.2013 um 21:47 hat Eric Blake geschrieben:
>> On 07/09/2013 03:53 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'
>>> 'reference': 'str' } }
>>> { 'type': 'ExampleObject',
>>> 'data: { 'file': 'BlockRef' } }
>>
>> Yay - a commit message that shows the new QMP wire format, and the
>> qapi-schema.json that generated it.
>
> Yeah, sorry about these missing examples and detailed documentation. I
> wanted to get an RFC out quickly and probably underestimated how
> important it is even for reviewing an RFC.
The higher volume the list, the more important documentation is :)
>
> However, I think keeping this information only in the commit messages
> isn't right either. Do we have any of the schema syntax documented
> anywhere? Should I create something in docs/(specs/)?
We have qemu/docs/qapi-code-gen.txt - and you are right - that file
NEEDS to be updated with this series :)
>
>> [Without reading the patch yet]
>> I take it the 'data' of such a union must be completely distinguishable
>> by type - the visitor for the 'file' key tries each subtype in turn
>> ('BlockOptions' or 'str') until one does not have a parse error.
>
> Not exactly. It asks the visitor for the type of the next item and then
> uses a lookup table to get the right discriminator enum value for it.
More efficient than trying every visitor; but same idea of the lookup
table being used to say which visitor will succeed.
>
>> And
>> this changes the earlier patch that required 'discriminator' to be used
>> only when 'base' was also present. Can it distinguish between two
>> structs, or is the union only allowed to have one struct option and all
>> other options are primitive types? Should we consider the use of arrays
>> as a union member type?
>
> As the types it uses for lookup are QObject types and all structs map to
> QDict, only one struct is allowed.
Worth documenting.
>
> This makes sense anyway, because allowing different structs would mean
> that you potentially get ambiguities when structs look alike (perhaps
> only differing in optional fields or something like that).
>
> As soon as we find a good use case for anonymous unions of two structs,
> I might change my opinion, though. ;-)
>
> Arrays are a logical extension of this, though I don't have any use for
> them now. I wouldn't expect them to be too hard to implement.
I don't mind leaving them out for now; as you said, it should be easy
enough to add at the point where the schema makes it worth doing.
>
>> Hmm, I wonder if this new anonymous union can be useful in helping us
>> write the self-description of introspection in Amos' patches.
>
> Hm, what's the exact problem there? I know I read that someone (Paolo?)
> wrote that it was impossible, but I didn't immediately understand why
> and didn't feel like thinking a lot about either.
Indeed: if we write a self-describing schema for introspection, then
'enum' uses 'data':['str'], while 'type', 'union', and 'command' use
'data':'QDict'. If we make an anonymous union with discriminator based
on enum/type/union/command, then the type of data is determined based on
the discriminator, with an array involved.
But it was more than just what data type is tied to 'data', it was also
the fact that qapi-schema.json contains free-form dictionaries
(arbitrary keys, each with a type), while QAPI tries to describe the
wire format of all possible keys, where only values are arbitrary
strings. Hence I've been arguing all along that we need a
post-processing step that turns qapi-schema's 'key':'Type' into the QMP
wire struct { 'name':'key', 'type':'Type' }, so that 'name' and 'type'
are fixed keys. I think both issues are at hand - how to represent each
field (whether as short-hand straight from the .json file or a long-hand
struct) and how to distinguish between qapi metatypes (which are
effectively a union between 'type'/'command'/...).
>>> +
>>> + ret += mcgen('''
>>> + default:
>>> + abort();
>>
>> Does this mean I can cause qemu to abort if I pass bogus information on
>> the wire? Using your commit message example,
>>
>> { 'file': false }
>>
>> would hit the default case, right?
>
> No, it wouldn't, but that case fails in an interesting way anyway. :-)
>
> We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which
> isn't defined and therefore 0. This happens to be the enum value for
> BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message
> like "invalid argument type: got bool, expected dict".
As long as it is an error message and not a qemu crash, I'm okay. I
should be able to throw valid JSON but garbage contents at the parser,
without the parser falling over.
>
>> Does it make sense to allow a union type (possibly only an anonymous
>> union type) in place of a struct type as a top-level type reference?
>> That is, just as we now allow { 'command':'foo', 'data': 'StructType' },
>> would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?
>
> Sounds sensible to me. But do we have a use case for it?
Not sure yet; again, one of those things that should be easy enough to
defer until we have a use case, and where introspection may be such a
use case.
--
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] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten()
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (7 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 20:25 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level Kevin Wolf
` (2 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/qapi/qmp/qdict.h | 1 +
qobject/qdict.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 685b2e3..b261570 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(QObject *obj);
#endif /* QDICT_H */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index ed381f9..1c38943 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -476,3 +476,50 @@ static void qdict_destroy_obj(QObject *obj)
g_free(qdict);
}
+
+static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
+{
+ QObject *value;
+ const QDictEntry *entry, *next;
+ const char *new_key;
+ bool delete;
+
+ entry = qdict_first(qdict);
+
+ while (entry != NULL) {
+
+ 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);
+ }
+
+ entry = next;
+ }
+}
+
+/**
+ * qdict_flatten(): For each nested QDict with key x, all fields with key y
+ * are moved to this QDict and their key is renamed to "x.y".
+ */
+void qdict_flatten(QObject *obj)
+{
+ QDict *qdict = qobject_to_qdict(obj);
+ qdict_do_flatten(qdict, qdict, NULL);
+}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten()
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten() Kevin Wolf
@ 2013-07-11 20:25 ` Eric Blake
2013-07-16 8:59 ` Kevin Wolf
0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2013-07-11 20:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2607 bytes --]
On 07/09/2013 03:53 AM, Kevin Wolf wrote:
Worth repeating this comment from the code into the commit message?
> + * 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".
Otherwise, I had to read nearly the entire patch to learn what I was
supposed to be reviewing.
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/qapi/qmp/qdict.h | 1 +
> qobject/qdict.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> +++ b/qobject/qdict.c
> @@ -476,3 +476,50 @@ static void qdict_destroy_obj(QObject *obj)
>
> g_free(qdict);
> }
> +
> +static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
> +{
> + QObject *value;
> + const QDictEntry *entry, *next;
> + const char *new_key;
> + bool delete;
> +
> + entry = qdict_first(qdict);
> +
> + while (entry != NULL) {
> +
> + next = qdict_next(qdict, entry);
> + value = qdict_entry_value(entry);
> + new_key = NULL;
> + delete = false;
> +
> + if (prefix) {
> + qobject_incref(value);
> + new_key = g_strdup_printf("%s.%s", prefix, entry->key);
> + qdict_put_obj(target, new_key, value);
You are calling this function with the same parameter for both qdict and
target. Doesn't that mean you are modifying qdict while iterating it?
Is that safe? [/me re-reads] - oh, you recursively call this function,
and this modification of target happens _only_ if prefix is non-null,
which happens only:
> + delete = true;
> + }
> +
> + if (qobject_type(value) == QTYPE_QDICT) {
> + qdict_do_flatten(qobject_to_qdict(value), target,
> + new_key ? new_key : entry->key);
when passing two different dicts into the function. Maybe add an
assert(!prefix || qdict != target).
> + delete = true;
> + }
> +
> + if (delete) {
> + qdict_del(qdict, entry->key);
> + }
> +
> + entry = next;
> + }
> +}
> +
> +/**
> + * qdict_flatten(): For each nested QDict with key x, all fields with key y
> + * are moved to this QDict and their key is renamed to "x.y".
> + */
> +void qdict_flatten(QObject *obj)
> +{
> + QDict *qdict = qobject_to_qdict(obj);
> + qdict_do_flatten(qdict, qdict, NULL);
> +}
>
--
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten()
2013-07-11 20:25 ` Eric Blake
@ 2013-07-16 8:59 ` Kevin Wolf
0 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2013-07-16 8:59 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 11.07.2013 um 22:25 hat Eric Blake geschrieben:
> On 07/09/2013 03:53 AM, Kevin Wolf wrote:
>
> Worth repeating this comment from the code into the commit message?
>
> > + * 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".
>
> Otherwise, I had to read nearly the entire patch to learn what I was
> supposed to be reviewing.
Okay, will do that.
> > +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);
>
> You are calling this function with the same parameter for both qdict and
> target. Doesn't that mean you are modifying qdict while iterating it?
> Is that safe? [/me re-reads] - oh, you recursively call this function,
> and this modification of target happens _only_ if prefix is non-null,
> which happens only:
>
> > + delete = true;
> > + }
> > +
> > + if (qobject_type(value) == QTYPE_QDICT) {
> > + qdict_do_flatten(qobject_to_qdict(value), target,
> > + new_key ? new_key : entry->key);
>
> when passing two different dicts into the function. Maybe add an
> assert(!prefix || qdict != target).
Your point still stands: The recursively called function modifies target
(which is qdict on the top level) by adding new keys. I guess when it
returns I need to restart the loop from the beginning in order to be
safe.
Kevin
> > + delete = true;
> > + }
> > +
> > + if (delete) {
> > + qdict_del(qdict, entry->key);
> > + }
> > +
> > + entry = next;
> > + }
> > +}
> > +
> > +/**
> > + * qdict_flatten(): For each nested QDict with key x, all fields with key y
> > + * are moved to this QDict and their key is renamed to "x.y".
> > + */
> > +void qdict_flatten(QObject *obj)
> > +{
> > + QDict *qdict = qobject_to_qdict(obj);
> > + qdict_do_flatten(qdict, qdict, NULL);
> > +}
> >
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (8 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten() Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 22:30 ` Eric Blake
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command Kevin Wolf
2013-07-12 9:55 ` [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Laszlo Ersek
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 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 6af907e..9ac278f 100644
--- a/block.c
+++ b/block.c
@@ -976,6 +976,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) {
@@ -1072,6 +1073,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 b3a57e0..e71c4ee 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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level Kevin Wolf
@ 2013-07-11 22:30 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-11 22:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 602 bytes --]
On 07/09/2013 03:53 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] 32+ messages in thread
* [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (9 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level Kevin Wolf
@ 2013-07-09 9:53 ` Kevin Wolf
2013-07-11 22:45 ` Eric Blake
2013-07-12 9:55 ` [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Laszlo Ersek
11 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-09 9:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, lcapitulino
This is just a quick hack to test things
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 32 ++++++++++++++++++++++++++++++++
qapi-schema.json | 29 +++++++++++++++++++++++++++++
qmp-commands.hx | 6 ++++++
3 files changed, 67 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index e71c4ee..e3a4fb8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error **errp)
block_job_complete(job, errp);
}
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+
+void qmp_blockdev_add(BlockOptions *options, Error **errp)
+{
+ QString *str;
+ QmpOutputVisitor *ov = qmp_output_visitor_new();
+ QObject *obj;
+ visit_type_BlockOptions(qmp_output_get_visitor(ov),
+ &options, NULL, errp);
+ obj = qmp_output_get_qobject(ov);
+ str = qobject_to_json_pretty(obj);
+ assert(str != NULL);
+ fprintf(stderr, "\n>>>>>>%s\n<<<<<<\n", qstring_get_str(str));
+ qdict_flatten(obj);
+ str = qobject_to_json_pretty(obj);
+ fprintf(stderr, "\n----->%s\n<-----\n", qstring_get_str(str));
+
+ Error *local_err = NULL;
+ QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qobject_to_qdict(obj), &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ } else {
+ drive_init(opts, IF_NONE);
+ }
+
+ qobject_decref(obj);
+ qmp_output_visitor_cleanup(ov);
+ QDECREF(str);
+}
+
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 a90aeb1..9f1cc8d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3702,3 +3702,32 @@
'*cpuid-input-ecx': 'int',
'cpuid-register': 'X86CPURegister32',
'features': 'int' } }
+
+
+{ 'type': 'BlockOptionsBase',
+ 'data': { 'driver': 'str', '*read-only': 'bool' } }
+
+{ 'type': 'BlockOptionsFile',
+ 'data': { 'filename': 'str' } }
+
+{ 'type': 'BlockOptionsRaw',
+ 'data': { 'file': 'BlockRef' } }
+
+{ 'type': 'BlockOptionsQcow2',
+ 'data': { '*lazy-refcounts': 'bool', 'file': 'BlockRef' } }
+
+{ 'union': 'BlockOptions',
+ 'base': 'BlockOptionsBase',
+ 'discriminator': 'driver',
+ 'data': {
+ 'file': 'BlockOptionsFile'
+ 'raw': 'BlockOptionsRaw'
+ 'qcow2': 'BlockOptionsQcow2'
+ } }
+
+{ 'union': 'BlockRef',
+ 'discriminator': {},
+ 'data': { 'definition': 'BlockOptions'
+ 'reference': 'str' } }
+
+{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..fe32ae7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3043,3 +3043,9 @@ Example:
<- { "return": {} }
EQMP
+
+ {
+ .name = "blockdev-add",
+ .args_type = "options:q",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
+ },
--
1.8.1.4
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command Kevin Wolf
@ 2013-07-11 22:45 ` Eric Blake
2013-07-12 9:40 ` Kevin Wolf
0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2013-07-11 22:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 5021 bytes --]
On 07/09/2013 03:53 AM, Kevin Wolf wrote:
> This is just a quick hack to test things
The rest of the series is mostly good to go, but not worth pushing until
this is flushed out. But I love where it's headed!
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 32 ++++++++++++++++++++++++++++++++
> qapi-schema.json | 29 +++++++++++++++++++++++++++++
> qmp-commands.hx | 6 ++++++
> 3 files changed, 67 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index e71c4ee..e3a4fb8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error **errp)
> block_job_complete(job, errp);
> }
>
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> +
> +void qmp_blockdev_add(BlockOptions *options, Error **errp)
> +{
> + QString *str;
> + QmpOutputVisitor *ov = qmp_output_visitor_new();
> + QObject *obj;
> + visit_type_BlockOptions(qmp_output_get_visitor(ov),
> + &options, NULL, errp);
> + obj = qmp_output_get_qobject(ov);
> + str = qobject_to_json_pretty(obj);
> + assert(str != NULL);
> + fprintf(stderr, "\n>>>>>>%s\n<<<<<<\n", qstring_get_str(str));
> + qdict_flatten(obj);
> + str = qobject_to_json_pretty(obj);
> + fprintf(stderr, "\n----->%s\n<-----\n", qstring_get_str(str));
Proof that it's a hack, with embedded debug messages :)
> +
> + Error *local_err = NULL;
> + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qobject_to_qdict(obj), &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + } else {
> + drive_init(opts, IF_NONE);
> + }
> +
> + qobject_decref(obj);
> + qmp_output_visitor_cleanup(ov);
> + QDECREF(str);
> +}
Rather elegant, now that the conversion between QMP and command line is
all hidden behind common visitors, all described by a nice QAPI
structure. Which is really what we've been wanting :)
> +
> 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 a90aeb1..9f1cc8d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3702,3 +3702,32 @@
> '*cpuid-input-ecx': 'int',
> 'cpuid-register': 'X86CPURegister32',
> 'features': 'int' } }
> +
> +
> +{ 'type': 'BlockOptionsBase',
> + 'data': { 'driver': 'str', '*read-only': 'bool' } }
Needs docs, of course; but as for the struct itself, it looks okay.
> +
> +{ 'type': 'BlockOptionsFile',
> + 'data': { 'filename': 'str' } }
> +
> +{ 'type': 'BlockOptionsRaw',
> + 'data': { 'file': 'BlockRef' } }
> +
> +{ 'type': 'BlockOptionsQcow2',
> + 'data': { '*lazy-refcounts': 'bool', 'file': 'BlockRef' } }
> +
> +{ 'union': 'BlockOptions',
> + 'base': 'BlockOptionsBase',
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockOptionsFile'
> + 'raw': 'BlockOptionsRaw'
> + 'qcow2': 'BlockOptionsQcow2'
Missing two ','; I'm surprised we haven't patched the qapi parser to be
stricter on that front (especially once that Amos' patches for
introspection demand valid JSON). [And I sooooo wish that JSON had
followed C99's lead of allowing trailing comma]
> + } }
> +
> +{ 'union': 'BlockRef',
> + 'discriminator': {},
> + 'data': { 'definition': 'BlockOptions'
> + 'reference': 'str' } }
Demonstrating both named and anonymous discriminated unions, which
serves as a good exercise of the earlier patches.
> +
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }
Sounds nice - and seems like it should be easy enough to extend BlockRef
and/or BlockOptions to have a way to pass an fd even for backing files,
getting to (my) end goal of using fd passing to get SELinux labeling for
NFS files out of the box from libvirt.
Should this command return anything? For example, returning a
BlockDeviceInfo (with its recursive listing of the entire backing chain)
might be useful to check that qemu's view of the world matches what the
caller passed in. Particularly important if we are able to let the user
choose whether to pass the full chain or to just pass the top-most image
and let qemu chase down the metadata in that image to open additional
files for the rest of the chain.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 362f0e1..fe32ae7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3043,3 +3043,9 @@ Example:
> <- { "return": {} }
>
> EQMP
> +
> + {
> + .name = "blockdev-add",
> + .args_type = "options:q",
> + .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> + },
>
--
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] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command
2013-07-11 22:45 ` Eric Blake
@ 2013-07-12 9:40 ` Kevin Wolf
2013-07-12 14:27 ` Eric Blake
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2013-07-12 9:40 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, qemu-devel, stefanha, lcapitulino
Am 12.07.2013 um 00:45 hat Eric Blake geschrieben:
> On 07/09/2013 03:53 AM, Kevin Wolf wrote:
> > This is just a quick hack to test things
>
> The rest of the series is mostly good to go, but not worth pushing until
> this is flushed out. But I love where it's headed!
Glad to hear this!
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > blockdev.c | 32 ++++++++++++++++++++++++++++++++
> > qapi-schema.json | 29 +++++++++++++++++++++++++++++
> > qmp-commands.hx | 6 ++++++
> > 3 files changed, 67 insertions(+)
>
>
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index e71c4ee..e3a4fb8 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error **errp)
> > block_job_complete(job, errp);
> > }
> >
> > +#include "qapi-visit.h"
> > +#include "qapi/qmp-output-visitor.h"
> > +
> > +void qmp_blockdev_add(BlockOptions *options, Error **errp)
> > +{
> > + QString *str;
> > + QmpOutputVisitor *ov = qmp_output_visitor_new();
> > + QObject *obj;
> > + visit_type_BlockOptions(qmp_output_get_visitor(ov),
> > + &options, NULL, errp);
> > + obj = qmp_output_get_qobject(ov);
> > + str = qobject_to_json_pretty(obj);
> > + assert(str != NULL);
> > + fprintf(stderr, "\n>>>>>>%s\n<<<<<<\n", qstring_get_str(str));
> > + qdict_flatten(obj);
> > + str = qobject_to_json_pretty(obj);
> > + fprintf(stderr, "\n----->%s\n<-----\n", qstring_get_str(str));
>
> Proof that it's a hack, with embedded debug messages :)
Sometimes you have to remind the reader that this is just an RFC. ;-)
> > +
> > + Error *local_err = NULL;
> > + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qobject_to_qdict(obj), &local_err);
> > + if (error_is_set(&local_err)) {
> > + qerror_report_err(local_err);
> > + error_free(local_err);
> > + } else {
> > + drive_init(opts, IF_NONE);
> > + }
> > +
> > + qobject_decref(obj);
> > + qmp_output_visitor_cleanup(ov);
> > + QDECREF(str);
> > +}
>
> Rather elegant, now that the conversion between QMP and command line is
> all hidden behind common visitors, all described by a nice QAPI
> structure. Which is really what we've been wanting :)
Yes, thinking a bit more about this, I'd actually consider this approach
mergable now. It's kind of backwards because -drive should really be
using the new blockdev-add infrastructure instead of the other way
round, but if we're careful enough that no bad -drive features leak into
blockdev-add, something like this code might be a reasonable first step
before we refactor things.
In the end we should also probably pass around the QAPI C structs
instead of converting struct -> QDict -> QOpts -> QDict, but that
shouldn't matter for any external interfaces, so we can do this later as
well.
> > +
> > 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 a90aeb1..9f1cc8d 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3702,3 +3702,32 @@
> > '*cpuid-input-ecx': 'int',
> > 'cpuid-register': 'X86CPURegister32',
> > 'features': 'int' } }
> > +
> > +
> > +{ 'type': 'BlockOptionsBase',
> > + 'data': { 'driver': 'str', '*read-only': 'bool' } }
>
> Needs docs, of course; but as for the struct itself, it looks okay.
>
> > +
> > +{ 'type': 'BlockOptionsFile',
> > + 'data': { 'filename': 'str' } }
> > +
> > +{ 'type': 'BlockOptionsRaw',
> > + 'data': { 'file': 'BlockRef' } }
> > +
> > +{ 'type': 'BlockOptionsQcow2',
> > + 'data': { '*lazy-refcounts': 'bool', 'file': 'BlockRef' } }
> > +
> > +{ 'union': 'BlockOptions',
> > + 'base': 'BlockOptionsBase',
> > + 'discriminator': 'driver',
> > + 'data': {
> > + 'file': 'BlockOptionsFile'
> > + 'raw': 'BlockOptionsRaw'
> > + 'qcow2': 'BlockOptionsQcow2'
>
> Missing two ','; I'm surprised we haven't patched the qapi parser to be
> stricter on that front (especially once that Amos' patches for
> introspection demand valid JSON). [And I sooooo wish that JSON had
> followed C99's lead of allowing trailing comma]
Interesting that this even works. I'll fix that, of course.
And I wholeheartedly agree about trailing commas.
> > + } }
> > +
> > +{ 'union': 'BlockRef',
> > + 'discriminator': {},
> > + 'data': { 'definition': 'BlockOptions'
> > + 'reference': 'str' } }
>
> Demonstrating both named and anonymous discriminated unions, which
> serves as a good exercise of the earlier patches.
>
> > +
> > +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }
>
> Sounds nice - and seems like it should be easy enough to extend BlockRef
> and/or BlockOptions to have a way to pass an fd even for backing files,
> getting to (my) end goal of using fd passing to get SELinux labeling for
> NFS files out of the box from libvirt.
In fact, I think it might be a schema-only patch at this point. :-)
> Should this command return anything? For example, returning a
> BlockDeviceInfo (with its recursive listing of the entire backing chain)
> might be useful to check that qemu's view of the world matches what the
> caller passed in. Particularly important if we are able to let the user
> choose whether to pass the full chain or to just pass the top-most image
> and let qemu chase down the metadata in that image to open additional
> files for the rest of the chain.
Does it matter for you to have this immediately as a return value from
blockdev-add, or wouldn't a following query-block on this device achieve
the same?
I'm not against adding a return value when it's useful, but I don't think
we should just return arbitrary query-* results if we can't think of
anything else to return.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command
2013-07-12 9:40 ` Kevin Wolf
@ 2013-07-12 14:27 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2013-07-12 14:27 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]
On 07/12/2013 03:40 AM, Kevin Wolf wrote:
>>> +
>>> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }
>>
>> Sounds nice - and seems like it should be easy enough to extend BlockRef
>> and/or BlockOptions to have a way to pass an fd even for backing files,
>> getting to (my) end goal of using fd passing to get SELinux labeling for
>> NFS files out of the box from libvirt.
>
> In fact, I think it might be a schema-only patch at this point. :-)
>
>> Should this command return anything? For example, returning a
>> BlockDeviceInfo (with its recursive listing of the entire backing chain)
>> might be useful to check that qemu's view of the world matches what the
>> caller passed in. Particularly important if we are able to let the user
>> choose whether to pass the full chain or to just pass the top-most image
>> and let qemu chase down the metadata in that image to open additional
>> files for the rest of the chain.
>
> Does it matter for you to have this immediately as a return value from
> blockdev-add, or wouldn't a following query-block on this device achieve
> the same?
Yeah, that will probably work. We have a couple *-add that return
useful values, but that was so we didn't have to introduce a separate
query at the time. But here, we already have the query, and for a
deeply nested struct, that's a lot of information to generate for
callers that don't care, and callers that do care can do a double call.
>
> I'm not against adding a return value when it's useful, but I don't think
> we should just return arbitrary query-* results if we can't think of
> anything else to return.
It should always be possible to upgrade from {} to actual struct
contents in a later release, should the need arise.
--
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] 32+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add
2013-07-09 9:53 [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Kevin Wolf
` (10 preceding siblings ...)
2013-07-09 9:53 ` [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command Kevin Wolf
@ 2013-07-12 9:55 ` Laszlo Ersek
2013-07-12 10:53 ` Kevin Wolf
11 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-07-12 9:55 UTC (permalink / raw)
To: Kevin Wolf; +Cc: lcapitulino, qemu-devel, stefanha, armbru
On 07/09/13 11:53, Kevin Wolf wrote:
> The goal of this series is to make QAPI ready to handle mostly unions in a less
> verbose way so that a future -blockdev command line option can be a
> direct mapping of the structure used by a blockdev-add QMP command. This
> series implements everything that I think is needed for this on the QAPI
> side. The block layer side is still lacking, but good enough that I
> could actually add a block device using the new command; the problem is
> here mostly that many -drive options aren't supported yet.
Apologies for arriving late to the party...
Do you think this could be handled by OptsVisitor? See the message part
of commit eb7ee2cb. (Directly following commits in the history
demonstrate use of the visitor.)
Using OptsVisitor, you'd receive a flat structure, specific to the
discriminator value selected. You'd have to transform that flat struct
into the qapi object tree that blockdev-add already takes; the benefit
is that the usual qemu-opt fishing and basic syntax validation would be
covered.
Your requirements are probably more complex than what OptsVisitor can
handle, but I'd like you to read the commit message and say, "yes, this
is insufficient for me". :)
Thanks,
Laszlo
>
> To give you an idea of what the interface looks like in the end, this is
> what you can pass now in theory (the block layer just won't like the
> read-only option and error out...):
>
> { "execute": "blockdev-add",
> "arguments": {
> "options": {
> "driver": "qcow2",
> "read-only": true,
> "lazy-refcounts": true,
> "file": {
> "driver": "file",
> "read-only": false,
> "filename": "/home/kwolf/images/hd.img"
> }
> }
> }
> }
>
> Kevin Wolf (11):
> qapi-types.py: Split off generate_struct_fields()
> qapi-types.py: Implement 'base' for unions
> qapi-visit.py: Split off generate_visit_struct_fields()
> qapi-visit.py: Implement 'base' for unions
> qapi: Add visitor for implicit structs
> qapi: Flat unions with arbitrary discriminator
> qapi: Add consume argument to qmp_input_get_object()
> qapi: Anonymous unions
> Implement qdict_flatten()
> block: Allow "driver" option on the top level
> [WIP] block: Implement 'blockdev-add' QMP command
>
> block.c | 7 ++
> blockdev.c | 52 ++++++++++---
> include/qapi/qmp/qdict.h | 1 +
> include/qapi/qmp/qobject.h | 1 +
> include/qapi/visitor-impl.h | 6 ++
> include/qapi/visitor.h | 6 ++
> qapi-schema.json | 29 +++++++
> qapi/qapi-visit-core.c | 25 +++++++
> qapi/qmp-input-visitor.c | 47 +++++++++---
> qmp-commands.hx | 6 ++
> qobject/qdict.c | 47 ++++++++++++
> qobject/qjson.c | 2 +
> scripts/qapi-types.py | 83 ++++++++++++++++++--
> scripts/qapi-visit.py | 179 ++++++++++++++++++++++++++++++++++++--------
> scripts/qapi.py | 28 +++++++
> 15 files changed, 459 insertions(+), 60 deletions(-)
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add
2013-07-12 9:55 ` [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add Laszlo Ersek
@ 2013-07-12 10:53 ` Kevin Wolf
0 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2013-07-12 10:53 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: lcapitulino, qemu-devel, stefanha, armbru
Am 12.07.2013 um 11:55 hat Laszlo Ersek geschrieben:
> On 07/09/13 11:53, Kevin Wolf wrote:
> > The goal of this series is to make QAPI ready to handle mostly unions in a less
> > verbose way so that a future -blockdev command line option can be a
> > direct mapping of the structure used by a blockdev-add QMP command. This
> > series implements everything that I think is needed for this on the QAPI
> > side. The block layer side is still lacking, but good enough that I
> > could actually add a block device using the new command; the problem is
> > here mostly that many -drive options aren't supported yet.
>
> Apologies for arriving late to the party...
>
> Do you think this could be handled by OptsVisitor? See the message part
> of commit eb7ee2cb. (Directly following commits in the history
> demonstrate use of the visitor.)
>
> Using OptsVisitor, you'd receive a flat structure, specific to the
> discriminator value selected. You'd have to transform that flat struct
> into the qapi object tree that blockdev-add already takes; the benefit
> is that the usual qemu-opt fishing and basic syntax validation would be
> covered.
>
> Your requirements are probably more complex than what OptsVisitor can
> handle, but I'd like you to read the commit message and say, "yes, this
> is insufficient for me". :)
I think OptsVisitor is about something else than this series. Here I'm
really adding a QMP part, while -drive is an existing command line
option that isn't changed and -blockdev isn't added yet.
But anyway, thanks for mentioning it, I wasn't aware that it exists. It
certainly doesn't replace this series, but it might prove useful in the
few lines that currently convert the QAPI struct to the QemuOpts that
drive_init() currently takes. I'm doing a detour through QDict to create
the QemuOpts currently and this may be something I can save (or maybe
not because I need to flatten the structure by turning e.g. a field
'filename' in the nested struct 'backing' into 'backing.filename')
When we later make the command line option use the QMP command, like it
should, instead of the other way round, OptsVisitor might be the way to
parse the command line. I'll keep it at the back of my mind, and if
necessary I can try to extend it when I get to this.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread