[Qemu-devel] [PATCH RFC 00/19] qapi: QMP introspection

[Qemu-devel] [PATCH RFC 00/19] qapi: QMP introspection

[Markus Armbruster]

Dring up your tea, here comes the introspection series.

* PATCH 01-15: As usual when I touch the qapi code generators, I need
  double-digit patches just to get the mess cleaned up enough to admit
  change :)

* PATCH 16-18: Fix the JSON parser to recognize null.  With an axe.

* PATCH 19: Introspection.  This one's completely unpolished and only
  lightly tested.  There's no documentation apart from the commit
  message.  I hope this is good enough to let us discuss the general
  approach and the introspection schema.

Prior work: Amos's "[PATCH v4 0/5] QMP full introspection" from Jan 14
https://lists.nongnu.org/archive/html/qemu-devel/2014-01/msg03013.html

Please note I'll be away from qemu-devel for three weeks.

Markus Armbruster (19):
  tests: Add missing dependencies on $(qapi-py)
  qapi: Fix C identifiers generated for names containing '.'
  qapi: Rename _generate_enum_string() to camel_to_upper()
  qapi: Rename generate_enum_full_value() to c_enum_const()
  qapi: Simplify c_enum_const()
  qapi: Use c_enum_const() in generate_alternate_qtypes()
  qapi: Move camel_to_upper(), c_enum_const() to closely related code
  qapi: qapi-event.py option -b does nothing, drop it
  qapi: qapi-commands.py option --type is unused, drop it
  qapi: Factor parse_command_line() out of the generators
  qapi: Fix generators to report command line errors decently
  qapi: Turn generators' mandatory option -i into an argument
  qapi: Factor open_output(), close_output() out of generators
  qapi: Drop pointless flush() before close()
  qapi: Inline gen_command_decl_prologue(), gen_command_def_prologue()
  qobject: Clean up around qtype_code
  qobject: Add a special null QObject
  json-parser: Fix to recognize null
  qapi: New QMP command query-schema for QMP schema introspection

 .gitignore                 |   1 +
 Makefile                   |  23 ++-
 Makefile.objs              |   1 +
 block/qapi.c               |   3 -
 include/hw/qdev-core.h     |   2 +-
 include/qapi/qmp/qobject.h |  11 +-
 monitor.c                  |   8 +
 qapi-schema.json           |   3 +
 qapi/introspect.json       |  72 ++++++++
 qmp-commands.hx            |  16 ++
 qobject/Makefile.objs      |   2 +-
 qobject/json-parser.c      |   2 +
 qobject/qjson.c            |   6 +-
 qobject/qnull.c            |  29 +++
 scripts/qapi-commands.py   | 185 ++++++-------------
 scripts/qapi-event.py      | 125 +++----------
 scripts/qapi-introspect.py | 430 +++++++++++++++++++++++++++++++++++++++++++++
 scripts/qapi-types.py      | 129 ++++----------
 scripts/qapi-visit.py      | 140 +++++----------
 scripts/qapi.py            | 161 ++++++++++++-----
 tests/.gitignore           |   1 +
 tests/Makefile             |  24 ++-
 22 files changed, 889 insertions(+), 485 deletions(-)
 create mode 100644 qapi/introspect.json
 create mode 100644 qobject/qnull.c
 create mode 100644 scripts/qapi-introspect.py

-- 
1.9.3

[Qemu-devel] [PATCH RFC 01/19] tests: Add missing dependencies on $(qapi-py)

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 9877a5b..64113a9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -295,22 +295,22 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
 	libqemuutil.a libqemustub.a
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
-$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
+$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
 		$(gen-out-type) -o tests -p "test-" -i $<, \
 		"  GEN   $@")
 tests/test-qapi-visit.c tests/test-qapi-visit.h :\
-$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-visit.py
+$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py \
 		$(gen-out-type) -o tests -p "test-" -i $<, \
 		"  GEN   $@")
 tests/test-qmp-commands.h tests/test-qmp-marshal.c :\
-$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
+$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
 		$(gen-out-type) -o tests -p "test-" -i $<, \
 		"  GEN   $@")
 tests/test-qapi-event.c tests/test-qapi-event.h :\
-$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-event.py
+$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \
 		$(gen-out-type) -o tests -p "test-" -i $<, \
 		"  GEN   $@")
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 01/19] tests: Add missing dependencies on $(qapi-py)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH RFC 02/19] qapi: Fix C identifiers generated for names containing '.'

[Markus Armbruster]

c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
QAPI names that get passed to c_var().

Which QAPI names get passed to c_fun(), to c_var(), or to both is not
obvious.  Names of command parameters and struct type members get
passed to c_var().

c_var() strips a leading '*', but this cannot happen.  c_fun()
doesn't.

Fix c_var() to work exactly like c_fun().

Perhaps they should be replaced by a single mapping function.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8e5b4ad..6d102df 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -693,6 +693,8 @@ def camel_case(name):
             new_name += ch.lower()
     return new_name
 
+c_var_trans = string.maketrans('.-', '__')
+
 def c_var(name, protect=True):
     # ANSI X3J11/88-090, 3.1.1
     c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
@@ -722,10 +724,10 @@ def c_var(name, protect=True):
     polluted_words = set(['unix', 'errno'])
     if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
         return "q_" + name
-    return name.replace('-', '_').lstrip("*")
+    return name.translate(c_var_trans)
 
 def c_fun(name, protect=True):
-    return c_var(name, protect).replace('.', '_')
+    return c_var(name, protect)
 
 def c_list_type(name):
     return '%sList' % name
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 02/19] qapi: Fix C identifiers generated for names containing '.'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
> QAPI names that get passed to c_var().
> 
> Which QAPI names get passed to c_fun(), to c_var(), or to both is not
> obvious.  Names of command parameters and struct type members get
> passed to c_var().
> 
> c_var() strips a leading '*', but this cannot happen.  c_fun()
> doesn't.
> 
> Fix c_var() to work exactly like c_fun().
> 
> Perhaps they should be replaced by a single mapping function.

How much harder is that to do?

Also, this commit probably means my qapi testsuite enhancements ought to
add tests that we support downstream extensions (__name.name_blah) in a
variety of situations. But I'm merely adding it to my (growing) list of
post-series additions, and won't hold up v6 adding it.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

If it's not too hard to use a single mapping function, that might look
prettier.  But from the raw perspective of fixing an inconsistency in
the code:

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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 02/19] qapi: Fix C identifiers generated for names containing '.'

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/02/2015 11:28 AM, Markus Armbruster wrote:
>> c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
>> QAPI names that get passed to c_var().
>> 
>> Which QAPI names get passed to c_fun(), to c_var(), or to both is not
>> obvious.  Names of command parameters and struct type members get
>> passed to c_var().
>> 
>> c_var() strips a leading '*', but this cannot happen.  c_fun()
>> doesn't.
>> 
>> Fix c_var() to work exactly like c_fun().
>> 
>> Perhaps they should be replaced by a single mapping function.
>
> How much harder is that to do?

Not hard at all.  I refrained from doing it for another reason, namely
that it would remove "variable" vs. "function" information from the
source.  This distinction may not be useful, and whether it's actually
done consistently in the source is even more doubtful, but I didn't wish
to decide that when I wrote the patch.

You're pursuing it in your "Fix C identifiers generated for names
containing '.'" series now.  I'll review.

> Also, this commit probably means my qapi testsuite enhancements ought to
> add tests that we support downstream extensions (__name.name_blah) in a
> variety of situations. But I'm merely adding it to my (growing) list of
> post-series additions, and won't hold up v6 adding it.
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi.py | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> If it's not too hard to use a single mapping function, that might look
> prettier.  But from the raw perspective of fixing an inconsistency in
> the code:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH RFC 02/19] qapi: Fix C identifiers generated for names containing '.'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4981 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
> QAPI names that get passed to c_var().
> 
> Which QAPI names get passed to c_fun(), to c_var(), or to both is not
> obvious.  Names of command parameters and struct type members get
> passed to c_var().
> 
> c_var() strips a leading '*', but this cannot happen.  c_fun()
> doesn't.
> 
> Fix c_var() to work exactly like c_fun().
> 
> Perhaps they should be replaced by a single mapping function.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

In order to test if we caught everything, I tried adding:


diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 8193dc1..757250a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -107,3 +107,11 @@
   'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } }
 { 'event': 'EVENT_D',
   'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3':
'EnumOne' } }
+
+# test that we correctly compile downstream extensions
+{ 'struct': '__org.qemu_Struct', 'data': { '__org.qemu_member': 'str' } }
+{ 'enum': '__org.qemu_Enum', 'data': [ '__org.qemu_value' ] }
+{ 'event': '__ORG.QEMU_EVENT', 'data': '__org.qemu_Struct' }
+{ 'command': '__org.qemu_command',
+  'data': { 'a': '__org.qemu_Enum', 'b': '__org.qemu_Struct' } }
+{ 'union': '__org.qemu_Union', 'data': { '__org.qemu_branch': 'str' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 93c4963..2ba7730 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -22,10 +22,17 @@
  OrderedDict([('event', 'EVENT_A')]),
  OrderedDict([('event', 'EVENT_B'), ('data', OrderedDict())]),
  OrderedDict([('event', 'EVENT_C'), ('data', OrderedDict([('*a',
'int'), ('*b', 'UserDefOne'), ('c', 'str')]))]),
- OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a',
'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))])]
+ OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a',
'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))]),
+ OrderedDict([('struct', '__org.qemu_Struct'), ('data',
OrderedDict([('__org.qemu_member', 'str')]))]),
+ OrderedDict([('enum', '__org.qemu_Enum'), ('data',
['__org.qemu_value'])]),
+ OrderedDict([('event', '__ORG.QEMU_EVENT'), ('data',
'__org.qemu_Struct')]),
+ OrderedDict([('command', '__org.qemu_command'), ('data',
OrderedDict([('a', '__org.qemu_Enum'), ('b', '__org.qemu_Struct')]))]),
+ OrderedDict([('union', '__org.qemu_Union'), ('data',
OrderedDict([('__org.qemu_branch', 'str')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
+ {'enum_name': '__org.qemu_Enum', 'enum_values': ['__org.qemu_value']},
  {'enum_name': 'UserDefAlternateKind', 'enum_values': None},
- {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
+ {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None},
+ {'enum_name': '__org.qemu_UnionKind', 'enum_values': None}]
 [OrderedDict([('struct', 'NestedEnumsOne'), ('data',
OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3',
'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefZero'), ('data',
OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'),
('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
@@ -37,4 +44,5 @@
  OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1',
'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefUnionBase'), ('data',
OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefOptions'), ('data',
OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16',
['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
- OrderedDict([('struct', 'EventStructOne'), ('data',
OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2',
'EnumOne')]))])]
+ OrderedDict([('struct', 'EventStructOne'), ('data',
OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2',
'EnumOne')]))]),
+ OrderedDict([('struct', '__org.qemu_Struct'), ('data',
OrderedDict([('__org.qemu_member', 'str')]))])]


but that leads to a number of compilation errors, even with your patch
installed, starting with:

In file included from tests/test-qmp-output-visitor.c:17:0:
tests/test-qapi-types.h:388:21: error: expected identifier or ‘(’ before
‘.’ token
 typedef struct __org.qemu_Struct __org.qemu_Struct;

I'm still playing with it to see if I can find ALL of the culprit spots.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 02/19] qapi: Fix C identifiers generated for names containing '.'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On 04/06/2015 05:25 PM, Eric Blake wrote:
> On 04/02/2015 11:28 AM, Markus Armbruster wrote:
>> c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
>> QAPI names that get passed to c_var().
>>
>> Which QAPI names get passed to c_fun(), to c_var(), or to both is not
>> obvious.  Names of command parameters and struct type members get
>> passed to c_var().
>>
>> c_var() strips a leading '*', but this cannot happen.  c_fun()
>> doesn't.
>>
>> Fix c_var() to work exactly like c_fun().
>>
>> Perhaps they should be replaced by a single mapping function.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi.py | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> In order to test if we caught everything, I tried adding:
> 

I took over this one patch, and turned it into a series:

https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01300.html

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH RFC 03/19] qapi: Rename _generate_enum_string() to camel_to_upper()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6d102df..a154272 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -477,7 +477,7 @@ def check_union(expr, expr_info):
 
         # Otherwise, check for conflicts in the generated enum
         else:
-            c_key = _generate_enum_string(key)
+            c_key = camel_to_upper(key)
             if c_key in values:
                 raise QAPIExprError(expr_info,
                                     "Union '%s' member '%s' clashes with '%s'"
@@ -495,7 +495,7 @@ def check_alternate(expr, expr_info):
         check_name(expr_info, "Member of alternate '%s'" % name, key)
 
         # Check for conflicts in the generated enum
-        c_key = _generate_enum_string(key)
+        c_key = camel_to_upper(key)
         if c_key in values:
             raise QAPIExprError(expr_info,
                                 "Alternate '%s' member '%s' clashes with '%s'"
@@ -529,7 +529,7 @@ def check_enum(expr, expr_info):
             raise QAPIExprError(expr_info,
                                 "Enum '%s' member '%s' is not a string"
                                 % (name, member))
-        key = _generate_enum_string(member)
+        key = camel_to_upper(member)
         if key in values:
             raise QAPIExprError(expr_info,
                                 "Enum '%s' member '%s' clashes with '%s'"
@@ -886,7 +886,7 @@ def guardend(name):
 # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
 # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
 # ENUM24_Name -> ENUM24_NAME
-def _generate_enum_string(value):
+def camel_to_upper(value):
     c_fun_str = c_fun(value, False)
     if value.isupper():
         return c_fun_str
@@ -906,6 +906,6 @@ def _generate_enum_string(value):
     return new_name.lstrip('_').upper()
 
 def generate_enum_full_value(enum_name, enum_value):
-    abbrev_string = _generate_enum_string(enum_name)
-    value_string = _generate_enum_string(enum_value)
+    abbrev_string = camel_to_upper(enum_name)
+    value_string = camel_to_upper(enum_value)
     return "%s_%s" % (abbrev_string, value_string)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 03/19] qapi: Rename _generate_enum_string() to camel_to_upper()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 04/19] qapi: Rename generate_enum_full_value() to c_enum_const()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-event.py | 5 ++---
 scripts/qapi-types.py | 6 +++---
 scripts/qapi-visit.py | 4 ++--
 scripts/qapi.py       | 6 +++---
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 47dc041..727bc7d 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -177,7 +177,7 @@ typedef enum %(event_enum_name)s
                       event_enum_name = event_enum_name)
 
     # append automatically generated _MAX value
-    enum_max_value = generate_enum_full_value(event_enum_name, "MAX")
+    enum_max_value = c_enum_const(event_enum_name, "MAX")
     enum_values = event_enum_values + [ enum_max_value ]
 
     i = 0
@@ -343,8 +343,7 @@ for expr in exprs:
         fdecl.write(ret)
 
         # We need an enum value per event
-        event_enum_value = generate_enum_full_value(event_enum_name,
-                                                    event_name)
+        event_enum_value = c_enum_const(event_enum_name, event_name)
         ret = generate_event_implement(api_name, event_name, params)
         fdef.write(ret)
 
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4789528..1d60501 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -118,13 +118,13 @@ const char *%(name)s_lookup[] = {
                          name=name)
     i = 0
     for value in values:
-        index = generate_enum_full_value(name, value)
+        index = c_enum_const(name, value)
         ret += mcgen('''
     [%(index)s] = "%(value)s",
 ''',
                      index = index, value = value)
 
-    max_index = generate_enum_full_value(name, 'MAX')
+    max_index = c_enum_const(name, 'MAX')
     ret += mcgen('''
     [%(max_index)s] = NULL,
 };
@@ -150,7 +150,7 @@ typedef enum %(name)s
 
     i = 0
     for value in enum_values:
-        enum_full_value = generate_enum_full_value(name, value)
+        enum_full_value = c_enum_const(name, value)
         enum_decl += mcgen('''
     %(enum_full_value)s = %(i)d,
 ''',
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 10b08c6..e9aa640 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -239,7 +239,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
             or find_union(members[key])
             or find_enum(members[key])), "Invalid alternate member"
 
-        enum_full_value = generate_enum_full_value(disc_type, key)
+        enum_full_value = c_enum_const(disc_type, key)
         ret += mcgen('''
     case %(enum_full_value)s:
         visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
@@ -340,7 +340,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
         else:
             fmt = 'visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);'
 
-        enum_full_value = generate_enum_full_value(disc_type, key)
+        enum_full_value = c_enum_const(disc_type, key)
         ret += mcgen('''
         case %(enum_full_value)s:
             ''' + fmt + '''
diff --git a/scripts/qapi.py b/scripts/qapi.py
index a154272..e1c5ef2 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -905,7 +905,7 @@ def camel_to_upper(value):
         new_name += c
     return new_name.lstrip('_').upper()
 
-def generate_enum_full_value(enum_name, enum_value):
-    abbrev_string = camel_to_upper(enum_name)
-    value_string = camel_to_upper(enum_value)
+def c_enum_const(type_name, const_name):
+    abbrev_string = camel_to_upper(type_name)
+    value_string = camel_to_upper(const_name)
     return "%s_%s" % (abbrev_string, value_string)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 04/19] qapi: Rename generate_enum_full_value() to c_enum_const()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-event.py | 5 ++---
>  scripts/qapi-types.py | 6 +++---
>  scripts/qapi-visit.py | 4 ++--
>  scripts/qapi.py       | 6 +++---
>  4 files changed, 10 insertions(+), 11 deletions(-)

Nicer name.  Might need a slight rebase on top of my work; I fixed a
place where generating alternate lookups was doing things by hand but
should be using this function (qapi-types.py:generate_alternate_qtypes()).

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: 604 bytes --]

[Qemu-devel] [PATCH RFC 05/19] qapi: Simplify c_enum_const()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index e1c5ef2..3601d1d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -906,6 +906,4 @@ def camel_to_upper(value):
     return new_name.lstrip('_').upper()
 
 def c_enum_const(type_name, const_name):
-    abbrev_string = camel_to_upper(type_name)
-    value_string = camel_to_upper(const_name)
-    return "%s_%s" % (abbrev_string, value_string)
+    return camel_to_upper(type_name + '_' + const_name)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 05/19] qapi: Simplify c_enum_const()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e1c5ef2..3601d1d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -906,6 +906,4 @@ def camel_to_upper(value):
>      return new_name.lstrip('_').upper()
>  
>  def c_enum_const(type_name, const_name):
> -    abbrev_string = camel_to_upper(type_name)
> -    value_string = camel_to_upper(const_name)
> -    return "%s_%s" % (abbrev_string, value_string)
> +    return camel_to_upper(type_name + '_' + const_name)

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: 604 bytes --]

[Qemu-devel] [PATCH RFC 06/19] qapi: Use c_enum_const() in generate_alternate_qtypes()

[Markus Armbruster]

Missed in commit b0b5819.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py |  6 ++----
 scripts/qapi.py       | 11 -----------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 1d60501..7782beb 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -180,11 +180,9 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
         assert qtype, "Invalid alternate member"
 
         ret += mcgen('''
-    [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
+    [ %(qtype)s ] = %(enum_const)s,
 ''',
-        qtype = qtype,
-        abbrev = de_camel_case(name).upper(),
-        enum = c_fun(de_camel_case(key),False).upper())
+        qtype = qtype, enum_const = c_enum_const(name + 'Kind', key))
 
     ret += mcgen('''
 };
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3601d1d..71eee06 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -669,17 +669,6 @@ def parse_args(typeinfo):
         # value of an optional argument.
         yield (argname, argentry, optional)
 
-def de_camel_case(name):
-    new_name = ''
-    for ch in name:
-        if ch.isupper() and new_name:
-            new_name += '_'
-        if ch == '-':
-            new_name += '_'
-        else:
-            new_name += ch.lower()
-    return new_name
-
 def camel_case(name):
     new_name = ''
     first = True
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 06/19] qapi: Use c_enum_const() in generate_alternate_qtypes()

[Eric Blake]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Missed in commit b0b5819.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-types.py |  6 ++----
>  scripts/qapi.py       | 11 -----------
>  2 files changed, 2 insertions(+), 15 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

although I think I touched the same code in my series for downstream
extension support, so we may have a rebase conflict to resolve.

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

[Qemu-devel] [PATCH RFC 07/19] qapi: Move camel_to_upper(), c_enum_const() to closely related code

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 71eee06..c963249 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -682,6 +682,31 @@ def camel_case(name):
             new_name += ch.lower()
     return new_name
 
+# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
+# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
+# ENUM24_Name -> ENUM24_NAME
+def camel_to_upper(value):
+    c_fun_str = c_fun(value, False)
+    if value.isupper():
+        return c_fun_str
+
+    new_name = ''
+    l = len(c_fun_str)
+    for i in range(l):
+        c = c_fun_str[i]
+        # When c is upper and no "_" appears before, do more checks
+        if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
+            # Case 1: next string is lower
+            # Case 2: previous string is digit
+            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
+            c_fun_str[i - 1].isdigit():
+                new_name += '_'
+        new_name += c
+    return new_name.lstrip('_').upper()
+
+def c_enum_const(type_name, const_name):
+    return camel_to_upper(type_name + '_' + const_name)
+
 c_var_trans = string.maketrans('.-', '__')
 
 def c_var(name, protect=True):
@@ -871,28 +896,3 @@ def guardend(name):
 
 ''',
                  name=guardname(name))
-
-# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
-# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
-# ENUM24_Name -> ENUM24_NAME
-def camel_to_upper(value):
-    c_fun_str = c_fun(value, False)
-    if value.isupper():
-        return c_fun_str
-
-    new_name = ''
-    l = len(c_fun_str)
-    for i in range(l):
-        c = c_fun_str[i]
-        # When c is upper and no "_" appears before, do more checks
-        if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
-            # Case 1: next string is lower
-            # Case 2: previous string is digit
-            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
-            c_fun_str[i - 1].isdigit():
-                new_name += '_'
-        new_name += c
-    return new_name.lstrip('_').upper()
-
-def c_enum_const(type_name, const_name):
-    return camel_to_upper(type_name + '_' + const_name)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 07/19] qapi: Move camel_to_upper(), c_enum_const() to closely related code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 08/19] qapi: qapi-event.py option -b does nothing, drop it

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 Makefile              | 2 +-
 scripts/qapi-event.py | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 88bce56..0a5bc09 100644
--- a/Makefile
+++ b/Makefile
@@ -273,7 +273,7 @@ $(qapi-modules) $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 qapi-event.c qapi-event.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \
-		$(gen-out-type) -o "." -b -i $<, \
+		$(gen-out-type) -o "." -i $<, \
 		"  GEN   $@")
 qmp-commands.h qmp-marshal.c :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 727bc7d..731c101 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -220,8 +220,8 @@ const char *%(event_enum_name)s_lookup[] = {
 # Start the real job
 
 try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:i:o:",
-                                   ["source", "header", "builtins", "prefix=",
+    opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:i:o:",
+                                   ["source", "header", "prefix=",
                                     "input-file=", "output-dir="])
 except getopt.GetoptError, err:
     print str(err)
@@ -235,7 +235,6 @@ h_file = 'qapi-event.h'
 
 do_c = False
 do_h = False
-do_builtins = False
 
 for o, a in opts:
     if o in ("-p", "--prefix"):
@@ -248,8 +247,6 @@ for o, a in opts:
         do_c = True
     elif o in ("-h", "--header"):
         do_h = True
-    elif o in ("-b", "--builtins"):
-        do_builtins = True
 
 if not do_c and not do_h:
     do_c = True
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 08/19] qapi: qapi-event.py option -b does nothing, drop it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  Makefile              | 2 +-
>  scripts/qapi-event.py | 7 ++-----
>  2 files changed, 3 insertions(+), 6 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 09/19] qapi: qapi-commands.py option --type is unused, drop it

[Markus Armbruster]

Anything but --type sync (which is the default) suppresses output
entirely, which makes no sense.

Dates back to the initial commit c17d990.  Commit message says
"Currently only generators for synchronous qapi/qmp functions are
supported", so maybe output other than "synchronous qapi/qmp" was
planned at the time, to be selected with --type.

Should other kinds of output ever materialize, we can put the option
back.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 66 +++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 35 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index db81044..c473a7c 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -391,14 +391,13 @@ try:
     opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:i:o:m",
                                    ["source", "header", "prefix=",
                                     "input-file=", "output-dir=",
-                                    "type=", "middle"])
+                                    "middle"])
 except getopt.GetoptError, err:
     print str(err)
     sys.exit(1)
 
 output_dir = ""
 prefix = ""
-dispatch_type = "sync"
 c_file = 'qmp-marshal.c'
 h_file = 'qmp-commands.h'
 middle_mode = False
@@ -413,8 +412,6 @@ for o, a in opts:
         input_file = a
     elif o in ("-o", "--output-dir"):
         output_dir = a + "/"
-    elif o in ("-t", "--type"):
-        dispatch_type = a
     elif o in ("-m", "--middle"):
         middle_mode = True
     elif o in ("-c", "--source"):
@@ -446,40 +443,39 @@ exprs = parse_schema(input_file)
 commands = filter(lambda expr: expr.has_key('command'), exprs)
 commands = filter(lambda expr: not expr.has_key('gen'), commands)
 
-if dispatch_type == "sync":
-    fdecl = maybe_open(do_h, h_file, 'w')
-    fdef = maybe_open(do_c, c_file, 'w')
-    ret = gen_command_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
+fdecl = maybe_open(do_h, h_file, 'w')
+fdef = maybe_open(do_c, c_file, 'w')
+ret = gen_command_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
+fdecl.write(ret)
+ret = gen_command_def_prologue(prefix=prefix)
+fdef.write(ret)
+
+for cmd in commands:
+    arglist = []
+    ret_type = None
+    if cmd.has_key('data'):
+        arglist = cmd['data']
+    if cmd.has_key('returns'):
+        ret_type = cmd['returns']
+    ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
     fdecl.write(ret)
-    ret = gen_command_def_prologue(prefix=prefix)
-    fdef.write(ret)
-
-    for cmd in commands:
-        arglist = []
-        ret_type = None
-        if cmd.has_key('data'):
-            arglist = cmd['data']
-        if cmd.has_key('returns'):
-            ret_type = cmd['returns']
-        ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
-        fdecl.write(ret)
-        if ret_type:
-            ret = gen_marshal_output(cmd['command'], arglist, ret_type, middle_mode) + "\n"
-            fdef.write(ret)
+    if ret_type:
+        ret = gen_marshal_output(cmd['command'], arglist, ret_type, middle_mode) + "\n"
+        fdef.write(ret)
 
-        if middle_mode:
-            fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
+    if middle_mode:
+        fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
 
-        ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
-        fdef.write(ret)
+    ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
+    fdef.write(ret)
 
-    fdecl.write("\n#endif\n");
+fdecl.write("\n#endif\n");
 
-    if not middle_mode:
-        ret = gen_registry(commands)
-        fdef.write(ret)
+if not middle_mode:
+    ret = gen_registry(commands)
+    fdef.write(ret)
 
-    fdef.flush()
-    fdef.close()
-    fdecl.flush()
-    fdecl.close()
+fdef.flush()
+fdef.close()
+fdecl.flush()
+fdecl.close()
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 09/19] qapi: qapi-commands.py option --type is unused, drop it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Anything but --type sync (which is the default) suppresses output
> entirely, which makes no sense.
> 
> Dates back to the initial commit c17d990.  Commit message says
> "Currently only generators for synchronous qapi/qmp functions are
> supported", so maybe output other than "synchronous qapi/qmp" was
> planned at the time, to be selected with --type.
> 
> Should other kinds of output ever materialize, we can put the option
> back.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 66 +++++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 35 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 10/19] qapi: Factor parse_command_line() out of the generators

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 34 +++-------------------------------
 scripts/qapi-event.py    | 32 +-------------------------------
 scripts/qapi-types.py    | 36 ++++--------------------------------
 scripts/qapi-visit.py    | 35 ++++-------------------------------
 scripts/qapi.py          | 40 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 52 insertions(+), 125 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index c473a7c..872bb01 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -15,9 +15,7 @@
 from ordereddict import OrderedDict
 from qapi import *
 import re
-import sys
 import os
-import getopt
 import errno
 
 def type_visitor(name):
@@ -386,42 +384,16 @@ def gen_command_def_prologue(prefix="", proxy=False):
         ret += '#include "%sqmp-commands.h"' % prefix
     return ret + "\n\n"
 
-
-try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:i:o:m",
-                                   ["source", "header", "prefix=",
-                                    "input-file=", "output-dir=",
-                                    "middle"])
-except getopt.GetoptError, err:
-    print str(err)
-    sys.exit(1)
-
-output_dir = ""
-prefix = ""
 c_file = 'qmp-marshal.c'
 h_file = 'qmp-commands.h'
 middle_mode = False
 
-do_c = False
-do_h = False
+(input_file, output_dir, do_c, do_h, prefix, opts) = \
+    parse_command_line("m", ["middle"])
 
 for o, a in opts:
-    if o in ("-p", "--prefix"):
-        prefix = a
-    elif o in ("-i", "--input-file"):
-        input_file = a
-    elif o in ("-o", "--output-dir"):
-        output_dir = a + "/"
-    elif o in ("-m", "--middle"):
+    if o in ("-m", "--middle"):
         middle_mode = True
-    elif o in ("-c", "--source"):
-        do_c = True
-    elif o in ("-h", "--header"):
-        do_h = True
-
-if not do_c and not do_h:
-    do_c = True
-    do_h = True
 
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 731c101..c7bbd54 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -11,9 +11,7 @@
 
 from ordereddict import OrderedDict
 from qapi import *
-import sys
 import os
-import getopt
 import errno
 
 def _generate_event_api_name(event_name, params):
@@ -219,38 +217,10 @@ const char *%(event_enum_name)s_lookup[] = {
 
 # Start the real job
 
-try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:i:o:",
-                                   ["source", "header", "prefix=",
-                                    "input-file=", "output-dir="])
-except getopt.GetoptError, err:
-    print str(err)
-    sys.exit(1)
-
-input_file = ""
-output_dir = ""
-prefix = ""
 c_file = 'qapi-event.c'
 h_file = 'qapi-event.h'
 
-do_c = False
-do_h = False
-
-for o, a in opts:
-    if o in ("-p", "--prefix"):
-        prefix = a
-    elif o in ("-i", "--input-file"):
-        input_file = a
-    elif o in ("-o", "--output-dir"):
-        output_dir = a + "/"
-    elif o in ("-c", "--source"):
-        do_c = True
-    elif o in ("-h", "--header"):
-        do_h = True
-
-if not do_c and not do_h:
-    do_c = True
-    do_h = True
+(input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
 
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7782beb..3b7ceef 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -11,9 +11,7 @@
 
 from ordereddict import OrderedDict
 from qapi import *
-import sys
 import os
-import getopt
 import errno
 
 def generate_fwd_struct(name, members, builtin_type=False):
@@ -273,43 +271,17 @@ void qapi_free_%(type)s(%(c_type)s obj)
                 c_type=c_type(name),type=name)
     return ret
 
-
-try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:i:o:",
-                                   ["source", "header", "builtins",
-                                    "prefix=", "input-file=", "output-dir="])
-except getopt.GetoptError, err:
-    print str(err)
-    sys.exit(1)
-
-output_dir = ""
-input_file = ""
-prefix = ""
 c_file = 'qapi-types.c'
 h_file = 'qapi-types.h'
-
-do_c = False
-do_h = False
 do_builtins = False
 
+(input_file, output_dir, do_c, do_h, prefix, opts) = \
+    parse_command_line("b", ["builtins"])
+
 for o, a in opts:
-    if o in ("-p", "--prefix"):
-        prefix = a
-    elif o in ("-i", "--input-file"):
-        input_file = a
-    elif o in ("-o", "--output-dir"):
-        output_dir = a + "/"
-    elif o in ("-c", "--source"):
-        do_c = True
-    elif o in ("-h", "--header"):
-        do_h = True
-    elif o in ("-b", "--builtins"):
+    if o in ("-b", "--builtins"):
         do_builtins = True
 
-if not do_c and not do_h:
-    do_c = True
-    do_h = True
-
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
 
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e9aa640..ef4a2fd 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,9 +15,7 @@
 from ordereddict import OrderedDict
 from qapi import *
 import re
-import sys
 import os
-import getopt
 import errno
 
 implicit_structs = []
@@ -403,42 +401,17 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **er
 ''',
                 name=name)
 
-try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:i:o:",
-                                   ["source", "header", "builtins", "prefix=",
-                                    "input-file=", "output-dir="])
-except getopt.GetoptError, err:
-    print str(err)
-    sys.exit(1)
-
-input_file = ""
-output_dir = ""
-prefix = ""
 c_file = 'qapi-visit.c'
 h_file = 'qapi-visit.h'
-
-do_c = False
-do_h = False
 do_builtins = False
 
+(input_file, output_dir, do_c, do_h, prefix, opts) = \
+    parse_command_line("b", ["builtins"])
+
 for o, a in opts:
-    if o in ("-p", "--prefix"):
-        prefix = a
-    elif o in ("-i", "--input-file"):
-        input_file = a
-    elif o in ("-o", "--output-dir"):
-        output_dir = a + "/"
-    elif o in ("-c", "--source"):
-        do_c = True
-    elif o in ("-h", "--header"):
-        do_h = True
-    elif o in ("-b", "--builtins"):
+    if o in ("-b", "--builtins"):
         do_builtins = True
 
-if not do_c and not do_h:
-    do_c = True
-    do_h = True
-
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
 
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c963249..1d8a3e5 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -13,6 +13,7 @@
 
 import re
 from ordereddict import OrderedDict
+import getopt
 import os
 import sys
 import string
@@ -896,3 +897,42 @@ def guardend(name):
 
 ''',
                  name=guardname(name))
+
+def parse_command_line(extra_options = "", extra_long_options = []):
+
+    try:
+        opts, args = getopt.gnu_getopt(sys.argv[1:],
+                                       "chp:i:o:" + extra_options,
+                                       ["source", "header", "prefix=",
+                                        "input-file=", "output-dir="]
+                                       + extra_long_options)
+    except getopt.GetoptError, err:
+        print str(err)
+        sys.exit(1)
+
+    output_dir = ""
+    prefix = ""
+    do_c = False
+    do_h = False
+    extra_opts = []
+
+    for oa in opts:
+        o, a = oa
+        if o in ("-p", "--prefix"):
+            prefix = a
+        elif o in ("-i", "--input-file"):
+            input_file = a
+        elif o in ("-o", "--output-dir"):
+            output_dir = a + "/"
+        elif o in ("-c", "--source"):
+            do_c = True
+        elif o in ("-h", "--header"):
+            do_h = True
+        else:
+            extra_opts.append(oa)
+
+    if not do_c and not do_h:
+        do_c = True
+        do_h = True
+
+    return (input_file, output_dir, do_c, do_h, prefix, extra_opts)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 10/19] qapi: Factor parse_command_line() out of the generators

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 34 +++-------------------------------
>  scripts/qapi-event.py    | 32 +-------------------------------
>  scripts/qapi-types.py    | 36 ++++--------------------------------
>  scripts/qapi-visit.py    | 35 ++++-------------------------------
>  scripts/qapi.py          | 40 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+), 125 deletions(-)

Nice savings in size.
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: 604 bytes --]

[Qemu-devel] [PATCH RFC 11/19] qapi: Fix generators to report command line errors decently

[Markus Armbruster]

Report to stderr, prefix with the program name.  Also reject
extra arguments.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1d8a3e5..c952c26 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -907,7 +907,7 @@ def parse_command_line(extra_options = "", extra_long_options = []):
                                         "input-file=", "output-dir="]
                                        + extra_long_options)
     except getopt.GetoptError, err:
-        print str(err)
+        print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
         sys.exit(1)
 
     output_dir = ""
@@ -935,4 +935,8 @@ def parse_command_line(extra_options = "", extra_long_options = []):
         do_c = True
         do_h = True
 
+    if len(args) != 0:
+        print >>sys.stderr, "%s: too many arguments"% sys.argv[0]
+        sys.exit(1)
+
     return (input_file, output_dir, do_c, do_h, prefix, extra_opts)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 11/19] qapi: Fix generators to report command line errors decently

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Report to stderr, prefix with the program name.  Also reject
> extra arguments.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

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: 604 bytes --]

[Qemu-devel] [PATCH RFC 12/19] qapi: Turn generators' mandatory option -i into an argument

[Markus Armbruster]

Mandatory option is silly, and the error handling is missing: the
programs crash when -i isn't supplied.  Make it an argument, and check
it properly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 Makefile        | 14 +++++++-------
 scripts/qapi.py | 10 ++++------
 tests/Makefile  |  8 ++++----
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/Makefile b/Makefile
index 0a5bc09..1e26f4d 100644
--- a/Makefile
+++ b/Makefile
@@ -243,17 +243,17 @@ qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
 qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
 $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
-		$(gen-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
+		$(gen-out-type) -o qga/qapi-generated -p "qga-" $<, \
 		"  GEN   $@")
 qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h :\
 $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py \
-		$(gen-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
+		$(gen-out-type) -o qga/qapi-generated -p "qga-" $<, \
 		"  GEN   $@")
 qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-marshal.c :\
 $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
-		$(gen-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
+		$(gen-out-type) -o qga/qapi-generated -p "qga-" $<, \
 		"  GEN   $@")
 
 qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
@@ -263,22 +263,22 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
 qapi-types.c qapi-types.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
-		$(gen-out-type) -o "." -b -i $<, \
+		$(gen-out-type) -o "." -b $<, \
 		"  GEN   $@")
 qapi-visit.c qapi-visit.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py \
-		$(gen-out-type) -o "." -b -i $<, \
+		$(gen-out-type) -o "." -b $<, \
 		"  GEN   $@")
 qapi-event.c qapi-event.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \
-		$(gen-out-type) -o "." -i $<, \
+		$(gen-out-type) -o "." $<, \
 		"  GEN   $@")
 qmp-commands.h qmp-marshal.c :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
-		$(gen-out-type) -o "." -m -i $<, \
+		$(gen-out-type) -o "." -m $<, \
 		"  GEN   $@")
 
 QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c952c26..189c9cd 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -902,10 +902,9 @@ def parse_command_line(extra_options = "", extra_long_options = []):
 
     try:
         opts, args = getopt.gnu_getopt(sys.argv[1:],
-                                       "chp:i:o:" + extra_options,
+                                       "chp:o:" + extra_options,
                                        ["source", "header", "prefix=",
-                                        "input-file=", "output-dir="]
-                                       + extra_long_options)
+                                        "output-dir="] + extra_long_options)
     except getopt.GetoptError, err:
         print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
         sys.exit(1)
@@ -920,8 +919,6 @@ def parse_command_line(extra_options = "", extra_long_options = []):
         o, a = oa
         if o in ("-p", "--prefix"):
             prefix = a
-        elif o in ("-i", "--input-file"):
-            input_file = a
         elif o in ("-o", "--output-dir"):
             output_dir = a + "/"
         elif o in ("-c", "--source"):
@@ -935,8 +932,9 @@ def parse_command_line(extra_options = "", extra_long_options = []):
         do_c = True
         do_h = True
 
-    if len(args) != 0:
+    if len(args) != 1:
         print >>sys.stderr, "%s: too many arguments"% sys.argv[0]
         sys.exit(1)
+    input_file = args[0]
 
     return (input_file, output_dir, do_c, do_h, prefix, extra_opts)
diff --git a/tests/Makefile b/tests/Makefile
index 64113a9..597919c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -297,22 +297,22 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
-		$(gen-out-type) -o tests -p "test-" -i $<, \
+		$(gen-out-type) -o tests -p "test-" $<, \
 		"  GEN   $@")
 tests/test-qapi-visit.c tests/test-qapi-visit.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py \
-		$(gen-out-type) -o tests -p "test-" -i $<, \
+		$(gen-out-type) -o tests -p "test-" $<, \
 		"  GEN   $@")
 tests/test-qmp-commands.h tests/test-qmp-marshal.c :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
-		$(gen-out-type) -o tests -p "test-" -i $<, \
+		$(gen-out-type) -o tests -p "test-" $<, \
 		"  GEN   $@")
 tests/test-qapi-event.c tests/test-qapi-event.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \
-		$(gen-out-type) -o tests -p "test-" -i $<, \
+		$(gen-out-type) -o tests -p "test-" $<, \
 		"  GEN   $@")
 
 tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 12/19] qapi: Turn generators' mandatory option -i into an argument

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Mandatory option is silly, and the error handling is missing: the
> programs crash when -i isn't supplied.  Make it an argument, and check
> it properly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  Makefile        | 14 +++++++-------
>  scripts/qapi.py | 10 ++++------
>  tests/Makefile  |  8 ++++----
>  3 files changed, 15 insertions(+), 17 deletions(-)

Needs corresponding changes to docs/qapi-code-gen.txt (several examples
use --input-file=...).


> @@ -935,8 +932,9 @@ def parse_command_line(extra_options = "", extra_long_options = []):
>          do_c = True
>          do_h = True
>  
> -    if len(args) != 0:
> +    if len(args) != 1:
>          print >>sys.stderr, "%s: too many arguments"% sys.argv[0]
>          sys.exit(1)

Won't this report 'too many arguments' even for a missing argument?

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 12/19] qapi: Turn generators' mandatory option -i into an argument

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/02/2015 11:28 AM, Markus Armbruster wrote:
>> Mandatory option is silly, and the error handling is missing: the
>> programs crash when -i isn't supplied.  Make it an argument, and check
>> it properly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  Makefile        | 14 +++++++-------
>>  scripts/qapi.py | 10 ++++------
>>  tests/Makefile  |  8 ++++----
>>  3 files changed, 15 insertions(+), 17 deletions(-)
>
> Needs corresponding changes to docs/qapi-code-gen.txt (several examples
> use --input-file=...).

Indeed.

>> @@ -935,8 +932,9 @@ def parse_command_line(extra_options = "",
>> extra_long_options = []):
>>          do_c = True
>>          do_h = True
>>  
>> -    if len(args) != 0:
>> +    if len(args) != 1:
>>          print >>sys.stderr, "%s: too many arguments"% sys.argv[0]
>>          sys.exit(1)
>
> Won't this report 'too many arguments' even for a missing argument?

Will fix.  Thanks!

[Qemu-devel] [PATCH RFC 13/19] qapi: Factor open_output(), close_output() out of generators

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 101 +++++++++++++++++------------------------------
 scripts/qapi-event.py    |  85 ++++++++++++---------------------------
 scripts/qapi-types.py    |  81 ++++++++++++-------------------------
 scripts/qapi-visit.py    | 101 ++++++++++++++++-------------------------------
 scripts/qapi.py          |  50 +++++++++++++++++++++++
 5 files changed, 172 insertions(+), 246 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 872bb01..10fa570 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -15,8 +15,6 @@
 from ordereddict import OrderedDict
 from qapi import *
 import re
-import os
-import errno
 
 def type_visitor(name):
     if type(name) == list:
@@ -321,51 +319,18 @@ qapi_init(qmp_init_marshal);
                 registry=registry.rstrip())
     return ret
 
-def gen_command_decl_prologue(header, guard, prefix=""):
+def gen_command_decl_prologue(prefix=""):
     ret = mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QAPI function prototypes
- *
- * Copyright IBM, Corp. 2011
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#ifndef %(guard)s
-#define %(guard)s
-
 #include "%(prefix)sqapi-types.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
 
 ''',
-                 header=basename(header), guard=guardname(header), prefix=prefix)
+                 prefix=prefix)
     return ret
 
 def gen_command_def_prologue(prefix="", proxy=False):
     ret = mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QMP->QAPI command dispatch
- *
- * Copyright IBM, Corp. 2011
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
 #include "qemu-common.h"
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
@@ -384,8 +349,6 @@ def gen_command_def_prologue(prefix="", proxy=False):
         ret += '#include "%sqmp-commands.h"' % prefix
     return ret + "\n\n"
 
-c_file = 'qmp-marshal.c'
-h_file = 'qmp-commands.h'
 middle_mode = False
 
 (input_file, output_dir, do_c, do_h, prefix, opts) = \
@@ -395,29 +358,44 @@ for o, a in opts:
     if o in ("-m", "--middle"):
         middle_mode = True
 
-c_file = output_dir + prefix + c_file
-h_file = output_dir + prefix + h_file
-
-def maybe_open(really, name, opt):
-    if really:
-        return open(name, opt)
-    else:
-        import StringIO
-        return StringIO.StringIO()
-
-try:
-    os.makedirs(output_dir)
-except os.error, e:
-    if e.errno != errno.EEXIST:
-        raise
-
 exprs = parse_schema(input_file)
 commands = filter(lambda expr: expr.has_key('command'), exprs)
 commands = filter(lambda expr: not expr.has_key('gen'), commands)
 
-fdecl = maybe_open(do_h, h_file, 'w')
-fdef = maybe_open(do_c, c_file, 'w')
-ret = gen_command_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
+c_comment = '''
+/*
+ * schema-defined QMP->QAPI command dispatch
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
+h_comment = '''
+/*
+ * schema-defined QAPI function prototypes
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
+
+(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
+                            'qmp-marshal.c', 'qmp-commands.h',
+                            c_comment, h_comment)
+
+ret = gen_command_decl_prologue(prefix=prefix)
 fdecl.write(ret)
 ret = gen_command_def_prologue(prefix=prefix)
 fdef.write(ret)
@@ -441,13 +419,8 @@ for cmd in commands:
     ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
     fdef.write(ret)
 
-fdecl.write("\n#endif\n");
-
 if not middle_mode:
     ret = gen_registry(commands)
     fdef.write(ret)
 
-fdef.flush()
-fdef.close()
-fdecl.flush()
-fdecl.close()
+close_output(fdef, fdecl)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index c7bbd54..3fd7e20 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -11,8 +11,6 @@
 
 from ordereddict import OrderedDict
 from qapi import *
-import os
-import errno
 
 def _generate_event_api_name(event_name, params):
     api_name = "void qapi_event_send_%s(" % c_fun(event_name).lower();
@@ -214,36 +212,9 @@ const char *%(event_enum_name)s_lookup[] = {
 ''')
     return ret
 
-
-# Start the real job
-
-c_file = 'qapi-event.c'
-h_file = 'qapi-event.h'
-
 (input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
 
-c_file = output_dir + prefix + c_file
-h_file = output_dir + prefix + h_file
-
-try:
-    os.makedirs(output_dir)
-except os.error, e:
-    if e.errno != errno.EEXIST:
-        raise
-
-def maybe_open(really, name, opt):
-    if really:
-        return open(name, opt)
-    else:
-        import StringIO
-        return StringIO.StringIO()
-
-fdef = maybe_open(do_c, c_file, 'w')
-fdecl = maybe_open(do_h, h_file, 'w')
-
-fdef.write(mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
+c_comment = '''
 /*
  * schema-defined QAPI event functions
  *
@@ -256,41 +227,43 @@ fdef.write(mcgen('''
  * See the COPYING.LIB file in the top-level directory.
  *
  */
+'''
+h_comment = '''
+/*
+ * schema-defined QAPI event functions
+ *
+ * Copyright (c) 2014 Wenchao Xia
+ *
+ * Authors:
+ *  Wenchao Xia  <wenchaoqemu@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
 
+(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
+                            'qapi-event.c', 'qapi-event.h',
+                            c_comment, h_comment)
+
+fdef.write(mcgen('''
 #include "qemu-common.h"
-#include "%(header)s"
+#include "%(prefix)sqapi-event.h"
 #include "%(prefix)sqapi-visit.h"
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/qmp-event.h"
 
 ''',
-                 prefix=prefix, header=basename(h_file)))
+                 prefix=prefix))
 
 fdecl.write(mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QAPI event functions
- *
- * Copyright (c) 2014 Wenchao Xia
- *
- * Authors:
- *  Wenchao Xia  <wenchaoqemu@gmail.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#ifndef %(guard)s
-#define %(guard)s
-
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
-                  prefix=prefix, guard=guardname(h_file)))
+                  prefix=prefix))
 
 exprs = parse_schema(input_file)
 
@@ -323,12 +296,4 @@ fdecl.write(ret)
 ret = generate_event_enum_lookup(event_enum_name, event_enum_strings)
 fdef.write(ret)
 
-fdecl.write('''
-#endif
-''')
-
-fdecl.flush()
-fdecl.close()
-
-fdef.flush()
-fdef.close()
+close_output(fdef, fdecl)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 3b7ceef..31e8d22 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -11,8 +11,6 @@
 
 from ordereddict import OrderedDict
 from qapi import *
-import os
-import errno
 
 def generate_fwd_struct(name, members, builtin_type=False):
     if builtin_type:
@@ -271,8 +269,6 @@ void qapi_free_%(type)s(%(c_type)s obj)
                 c_type=c_type(name),type=name)
     return ret
 
-c_file = 'qapi-types.c'
-h_file = 'qapi-types.h'
 do_builtins = False
 
 (input_file, output_dir, do_c, do_h, prefix, opts) = \
@@ -282,28 +278,7 @@ for o, a in opts:
     if o in ("-b", "--builtins"):
         do_builtins = True
 
-c_file = output_dir + prefix + c_file
-h_file = output_dir + prefix + h_file
-
-try:
-    os.makedirs(output_dir)
-except os.error, e:
-    if e.errno != errno.EEXIST:
-        raise
-
-def maybe_open(really, name, opt):
-    if really:
-        return open(name, opt)
-    else:
-        import StringIO
-        return StringIO.StringIO()
-
-fdef = maybe_open(do_c, c_file, 'w')
-fdecl = maybe_open(do_h, h_file, 'w')
-
-fdef.write(mcgen('''
-/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
+c_comment = '''
 /*
  * deallocation functions for schema-defined QAPI types
  *
@@ -317,37 +292,39 @@ fdef.write(mcgen('''
  * See the COPYING.LIB file in the top-level directory.
  *
  */
+'''
+h_comment = '''
+/*
+ * schema-defined QAPI types
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
 
+(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
+                            'qapi-types.c', 'qapi-types.h',
+                            c_comment, h_comment)
+
+fdef.write(mcgen('''
 #include "qapi/dealloc-visitor.h"
 #include "%(prefix)sqapi-types.h"
 #include "%(prefix)sqapi-visit.h"
 
-''',             prefix=prefix))
+''',
+                 prefix=prefix))
 
 fdecl.write(mcgen('''
-/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QAPI types
- *
- * Copyright IBM, Corp. 2011
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#ifndef %(guard)s
-#define %(guard)s
-
 #include <stdbool.h>
 #include <stdint.h>
 
-''',
-                  guard=guardname(h_file)))
+'''))
 
 exprs = parse_schema(input_file)
 exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
@@ -425,12 +402,4 @@ for expr in exprs:
         continue
     fdecl.write(ret)
 
-fdecl.write('''
-#endif
-''')
-
-fdecl.flush()
-fdecl.close()
-
-fdef.flush()
-fdef.close()
+close_output(fdef, fdecl)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ef4a2fd..a7a7568 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,8 +15,6 @@
 from ordereddict import OrderedDict
 from qapi import *
 import re
-import os
-import errno
 
 implicit_structs = []
 
@@ -401,8 +399,6 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **er
 ''',
                 name=name)
 
-c_file = 'qapi-visit.c'
-h_file = 'qapi-visit.h'
 do_builtins = False
 
 (input_file, output_dir, do_c, do_h, prefix, opts) = \
@@ -412,70 +408,51 @@ for o, a in opts:
     if o in ("-b", "--builtins"):
         do_builtins = True
 
-c_file = output_dir + prefix + c_file
-h_file = output_dir + prefix + h_file
+c_comment = '''
+/*
+ * schema-defined QAPI visitor functions
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
+h_comment = '''
+/*
+ * schema-defined QAPI visitor functions
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
 
-try:
-    os.makedirs(output_dir)
-except os.error, e:
-    if e.errno != errno.EEXIST:
-        raise
-
-def maybe_open(really, name, opt):
-    if really:
-        return open(name, opt)
-    else:
-        import StringIO
-        return StringIO.StringIO()
-
-fdef = maybe_open(do_c, c_file, 'w')
-fdecl = maybe_open(do_h, h_file, 'w')
+(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
+                            'qapi-visit.c', 'qapi-visit.h',
+                            c_comment, h_comment)
 
 fdef.write(mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QAPI visitor functions
- *
- * Copyright IBM, Corp. 2011
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
 #include "qemu-common.h"
-#include "%(header)s"
+#include "%(prefix)sqapi-visit.h"
 ''',
-                 header=basename(h_file)))
+                 prefix = prefix))
 
 fdecl.write(mcgen('''
-/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-/*
- * schema-defined QAPI visitor functions
- *
- * Copyright IBM, Corp. 2011
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#ifndef %(guard)s
-#define %(guard)s
-
 #include "qapi/visitor.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
-                  prefix=prefix, guard=guardname(h_file)))
+                  prefix=prefix))
 
 exprs = parse_schema(input_file)
 
@@ -532,12 +509,4 @@ for expr in exprs:
         ret += generate_enum_declaration(expr['enum'], expr['data'])
         fdecl.write(ret)
 
-fdecl.write('''
-#endif
-''')
-
-fdecl.flush()
-fdecl.close()
-
-fdef.flush()
-fdef.close()
+close_output(fdef, fdecl)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 189c9cd..480d638 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -13,6 +13,7 @@
 
 import re
 from ordereddict import OrderedDict
+import errno
 import getopt
 import os
 import sys
@@ -938,3 +939,52 @@ def parse_command_line(extra_options = "", extra_long_options = []):
     input_file = args[0]
 
     return (input_file, output_dir, do_c, do_h, prefix, extra_opts)
+
+def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
+                c_comment, h_comment):
+    c_file = output_dir + prefix + c_file
+    h_file = output_dir + prefix + h_file
+
+    try:
+        os.makedirs(output_dir)
+    except os.error, e:
+        if e.errno != errno.EEXIST:
+            raise
+
+    def maybe_open(really, name, opt):
+        if really:
+            return open(name, opt)
+        else:
+            import StringIO
+            return StringIO.StringIO()
+
+    fdef = maybe_open(do_c, c_file, 'w')
+    fdecl = maybe_open(do_h, h_file, 'w')
+
+    fdef.write(mcgen('''
+/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+%(comment)s
+''',
+                     comment = c_comment))
+
+    fdecl.write(mcgen('''
+/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+%(comment)s
+#ifndef %(guard)s
+#define %(guard)s
+
+''',
+                      comment = h_comment, guard = guardname(h_file)))
+
+    return (fdef, fdecl)
+
+def close_output(fdef, fdecl):
+    fdecl.write('''
+#endif
+''')
+
+    fdecl.flush()
+    fdecl.close()
+
+    fdef.flush()
+    fdef.close()
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 13/19] qapi: Factor open_output(), close_output() out of generators

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 101 +++++++++++++++++------------------------------
>  scripts/qapi-event.py    |  85 ++++++++++++---------------------------
>  scripts/qapi-types.py    |  81 ++++++++++++-------------------------
>  scripts/qapi-visit.py    | 101 ++++++++++++++++-------------------------------
>  scripts/qapi.py          |  50 +++++++++++++++++++++++
>  5 files changed, 172 insertions(+), 246 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 14/19] qapi: Drop pointless flush() before close()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 480d638..e645623 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -982,9 +982,5 @@ def close_output(fdef, fdecl):
     fdecl.write('''
 #endif
 ''')
-
-    fdecl.flush()
     fdecl.close()
-
-    fdef.flush()
     fdef.close()
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 14/19] qapi: Drop pointless flush() before close()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH RFC 15/19] qapi: Inline gen_command_decl_prologue(), gen_command_def_prologue()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 58 ++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 10fa570..bf41a78 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -319,36 +319,6 @@ qapi_init(qmp_init_marshal);
                 registry=registry.rstrip())
     return ret
 
-def gen_command_decl_prologue(prefix=""):
-    ret = mcgen('''
-#include "%(prefix)sqapi-types.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/error.h"
-
-''',
-                 prefix=prefix)
-    return ret
-
-def gen_command_def_prologue(prefix="", proxy=False):
-    ret = mcgen('''
-#include "qemu-common.h"
-#include "qemu/module.h"
-#include "qapi/qmp/qerror.h"
-#include "qapi/qmp/types.h"
-#include "qapi/qmp/dispatch.h"
-#include "qapi/visitor.h"
-#include "qapi/qmp-output-visitor.h"
-#include "qapi/qmp-input-visitor.h"
-#include "qapi/dealloc-visitor.h"
-#include "%(prefix)sqapi-types.h"
-#include "%(prefix)sqapi-visit.h"
-
-''',
-                prefix=prefix)
-    if not proxy:
-        ret += '#include "%sqmp-commands.h"' % prefix
-    return ret + "\n\n"
-
 middle_mode = False
 
 (input_file, output_dir, do_c, do_h, prefix, opts) = \
@@ -395,10 +365,30 @@ h_comment = '''
                             'qmp-marshal.c', 'qmp-commands.h',
                             c_comment, h_comment)
 
-ret = gen_command_decl_prologue(prefix=prefix)
-fdecl.write(ret)
-ret = gen_command_def_prologue(prefix=prefix)
-fdef.write(ret)
+fdef.write(mcgen('''
+#include "qemu-common.h"
+#include "qemu/module.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/types.h"
+#include "qapi/qmp/dispatch.h"
+#include "qapi/visitor.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/dealloc-visitor.h"
+#include "%(prefix)sqapi-types.h"
+#include "%(prefix)sqapi-visit.h"
+#include "%(prefix)sqmp-commands.h"
+
+''',
+                prefix=prefix))
+
+fdecl.write(mcgen('''
+#include "%(prefix)sqapi-types.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+
+''',
+                 prefix=prefix))
 
 for cmd in commands:
     arglist = []
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 15/19] qapi: Inline gen_command_decl_prologue(), gen_command_def_prologue()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 58 ++++++++++++++++++++----------------------------
>  1 file changed, 24 insertions(+), 34 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 16/19] qobject: Clean up around qtype_code

[Markus Armbruster]

QTYPE_NONE is a sentinel value.  No QObject has this type code.
Document it properly.

Fix dump_qobject() to abort() on QTYPE_NONE, just like for any other
invalid type code.

Fix to_json() to abort() on all invalid type codes, not just
QTYPE_MAX.

Clean up Property member qtype's type: it's a qtype_code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/qapi.c               | 3 ---
 include/hw/qdev-core.h     | 2 +-
 include/qapi/qmp/qobject.h | 2 +-
 qobject/qjson.c            | 3 +--
 4 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 8a19aed..3802187 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -519,9 +519,6 @@ static void dump_qobject(fprintf_function func_fprintf, void *f,
             QDECREF(value);
             break;
         }
-        case QTYPE_NONE:
-            break;
-        case QTYPE_MAX:
         default:
             abort();
     }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..9a0ee30 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -226,7 +226,7 @@ struct Property {
     PropertyInfo *info;
     int          offset;
     uint8_t      bitnr;
-    uint8_t      qtype;
+    qtype_code   qtype;
     int64_t      defval;
     int          arrayoffset;
     PropertyInfo *arrayinfo;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index d0bbc7c..0991296 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -36,7 +36,7 @@
 #include <assert.h>
 
 typedef enum {
-    QTYPE_NONE,
+    QTYPE_NONE,    /* sentinel value, no QObject has this type code */
     QTYPE_QINT,
     QTYPE_QSTRING,
     QTYPE_QDICT,
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 12c576d..f2857c1 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -260,9 +260,8 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
     case QTYPE_QERROR:
         /* XXX: should QError be emitted? */
-    case QTYPE_NONE:
         break;
-    case QTYPE_MAX:
+    default:
         abort();
     }
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 16/19] qobject: Clean up around qtype_code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

On 04/02/2015 11:29 AM, Markus Armbruster wrote:
> QTYPE_NONE is a sentinel value.  No QObject has this type code.
> Document it properly.
> 
> Fix dump_qobject() to abort() on QTYPE_NONE, just like for any other
> invalid type code.
> 
> Fix to_json() to abort() on all invalid type codes, not just
> QTYPE_MAX.
> 
> Clean up Property member qtype's type: it's a qtype_code.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/qapi.c               | 3 ---
>  include/hw/qdev-core.h     | 2 +-
>  include/qapi/qmp/qobject.h | 2 +-
>  qobject/qjson.c            | 3 +--
>  4 files changed, 3 insertions(+), 7 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: 604 bytes --]

[Qemu-devel] [PATCH RFC 17/19] qobject: Add a special null QObject

[Markus Armbruster]

I'm going to fix the JSON parser to recognize null.  The obvious
representation of JSON null as (QObject *)NULL doesn't work, because
the parser already uses it as an error value.  Perhaps we should
change it to free NULL for null, but that's more than I can do right
now.  Create a special null QObject instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qobject.h |  9 +++++++++
 qobject/Makefile.objs      |  2 +-
 qobject/qjson.c            |  3 +++
 qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 qobject/qnull.c

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 0991296..b6d18c2 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -37,6 +37,7 @@
 
 typedef enum {
     QTYPE_NONE,    /* sentinel value, no QObject has this type code */
+    QTYPE_QNULL,
     QTYPE_QINT,
     QTYPE_QSTRING,
     QTYPE_QDICT,
@@ -110,4 +111,12 @@ static inline qtype_code qobject_type(const QObject *obj)
     return obj->type->code;
 }
 
+extern QObject qnull_;
+
+static inline QObject *qnull(void)
+{
+    qobject_incref(&qnull_);
+    return &qnull_;
+}
+
 #endif /* QOBJECT_H */
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index c9ff59c..f7595f5 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,3 +1,3 @@
-util-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
+util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
 util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 util-obj-y += qerror.o
diff --git a/qobject/qjson.c b/qobject/qjson.c
index f2857c1..846733d 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -127,6 +127,9 @@ static void to_json_list_iter(QObject *obj, void *opaque)
 static void to_json(const QObject *obj, QString *str, int pretty, int indent)
 {
     switch (qobject_type(obj)) {
+    case QTYPE_QNULL:
+        qstring_append(str, "null");
+        break;
     case QTYPE_QINT: {
         QInt *val = qobject_to_qint(obj);
         char buffer[1024];
diff --git a/qobject/qnull.c b/qobject/qnull.c
new file mode 100644
index 0000000..58118c2
--- /dev/null
+++ b/qobject/qnull.c
@@ -0,0 +1,29 @@
+/*
+ * QNull
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later.  See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu-common.h"
+#include "qapi/qmp/qobject.h"
+
+static void qnull_destroy_obj(QObject *obj)
+{
+    assert(0);
+}
+
+static const QType qnull_type = {
+    .code = QTYPE_QINT,
+    .destroy = qnull_destroy_obj,
+};
+
+QObject qnull_ = {
+    .type = &qnull_type,
+    .refcnt = 1
+};
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 17/19] qobject: Add a special null QObject

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

On 04/02/2015 11:29 AM, Markus Armbruster wrote:
> I'm going to fix the JSON parser to recognize null.  The obvious
> representation of JSON null as (QObject *)NULL doesn't work, because
> the parser already uses it as an error value.  Perhaps we should
> change it to free NULL for null, but that's more than I can do right
> now.  Create a special null QObject instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qmp/qobject.h |  9 +++++++++
>  qobject/Makefile.objs      |  2 +-
>  qobject/qjson.c            |  3 +++
>  qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100644 qobject/qnull.c
> 

> +static const QType qnull_type = {
> +    .code = QTYPE_QINT,

s/QINT/QNULL/

> +    .destroy = qnull_destroy_obj,
> +};
> +
> +QObject qnull_ = {
> +    .type = &qnull_type,
> +    .refcnt = 1
> +};

Worth a trailing comma in the initializer?

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 17/19] qobject: Add a special null QObject

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/02/2015 11:29 AM, Markus Armbruster wrote:
>> I'm going to fix the JSON parser to recognize null.  The obvious
>> representation of JSON null as (QObject *)NULL doesn't work, because
>> the parser already uses it as an error value.  Perhaps we should
>> change it to free NULL for null, but that's more than I can do right
>> now.  Create a special null QObject instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qmp/qobject.h |  9 +++++++++
>>  qobject/Makefile.objs      |  2 +-
>>  qobject/qjson.c            |  3 +++
>>  qobject/qnull.c            | 29 +++++++++++++++++++++++++++++
>>  4 files changed, 42 insertions(+), 1 deletion(-)
>>  create mode 100644 qobject/qnull.c
>> 
>
>> +static const QType qnull_type = {
>> +    .code = QTYPE_QINT,
>
> s/QINT/QNULL/

Oww.

>> +    .destroy = qnull_destroy_obj,
>> +};
>> +
>> +QObject qnull_ = {
>> +    .type = &qnull_type,
>> +    .refcnt = 1
>> +};
>
> Worth a trailing comma in the initializer?

Unlikely to require more initializers, but I don't mind.

Conflicts with your "qapi: Accept 'null' in QMP".  Current plan: you
pick the best of both into a new series.

[Qemu-devel] [PATCH RFC 18/19] json-parser: Fix to recognize null

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/json-parser.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 4288267..717cb8f 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -561,6 +561,8 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
         ret = QOBJECT(qbool_from_int(true));
     } else if (token_is_keyword(token, "false")) {
         ret = QOBJECT(qbool_from_int(false));
+    } else if (token_is_keyword(token, "null")) {
+        ret = qnull();
     } else {
         parse_error(ctxt, token, "invalid keyword `%s'", token_get_value(token));
         goto out;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 18/19] json-parser: Fix to recognize null

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On 04/02/2015 11:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qobject/json-parser.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 4288267..717cb8f 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -561,6 +561,8 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
>          ret = QOBJECT(qbool_from_int(true));
>      } else if (token_is_keyword(token, "false")) {
>          ret = QOBJECT(qbool_from_int(false));
> +    } else if (token_is_keyword(token, "null")) {
> +        ret = qnull();
>      } else {
>          parse_error(ctxt, token, "invalid keyword `%s'", token_get_value(token));
>          goto out;
> 

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Caution, rough edges.

qapi/introspect.json defines the introspection schema.  It should do
for uses other than QMP.
FIXME it's almost entirely devoid of comments.

The introspection schema does not reflect all the rules and
restrictions that apply to QAPI schemata.  A valid QAPI schema has an
introspection value conforming to the introspection schema, but the
converse is not true.

Introspection lowers away a number of schema details:

* The builtin types are declared with their JSON type.

  TODO should we map all the integer types to just int?

* Implicit type definitions are made explicit, and given
  auto-generated names.  These names start with ':' so they don't
  clash with the user's names.

  Example: a simple union implicitly defines an enumeration type for
  its discriminator.

* All type references are by name.

* Base types are flattened.

* The top type (named '**') can hold any value.

  It can currently occur only in commands, but the introspection
  schema doesn't reflect that.

* The struct and union types are generalized into an object type.

* Commands take a single argument and return a single result.

  Dictionary argument/result or list result is an implicit type
  definition.

  Missing argument/result is an implicit definition of the empty
  object.

  The argument is always of object type, but the introspection schema
  doesn't reflect that.

  The 'gen': false directive is omitted as implementation detail.

* Events carry a single data value.

  Implicit type definition as above.

  The value is of object type, but the introspection schema doesn't
  reflect that.

* Types not used by commands or events are omitted.

  Indirect use counts as use.

* Optional members have a default, which can only be null right now

  Instead of a mandatory "optional" flag, we have an optionial
  default.  No default means mandatory, default null means optional
  without default value.  Non-null is available for optional with
  default.

TODO much of the above should go into docs.

New generator scripts/qapi-introspect.py computes an introspection
value for its input, and generates a C variable holding it.
Pythonistas may find it ugly and/or clumsy.

FIXME it can generate awfully long lines

The schema generation proper is pretty trivial.  Much of the
qapi-introspect.py's code actually does something else: convert the
output of parse_schema() into a more usable data structure, lowering
away uninteresting stuff.  This should really be done in qapi.py, so
the other generators can use it, too.

For testing, I feed it tests/qapi-schema/qapi-schema-test.json, but no
more.

New QMP command query-schema parses its return value from that
variable.  Its reply is less than 75KiBytes right now.

A compile time test that the value can be parsed would be nice.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 .gitignore                 |   1 +
 Makefile                   |   9 +-
 Makefile.objs              |   1 +
 monitor.c                  |   8 +
 qapi-schema.json           |   3 +
 qapi/introspect.json       |  72 ++++++++
 qmp-commands.hx            |  16 ++
 scripts/qapi-introspect.py | 430 +++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore           |   1 +
 tests/Makefile             |   8 +-
 10 files changed, 547 insertions(+), 2 deletions(-)
 create mode 100644 qapi/introspect.json
 create mode 100644 scripts/qapi-introspect.py

diff --git a/.gitignore b/.gitignore
index e32a584..91569ba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -32,6 +32,7 @@
 /qapi-visit.[ch]
 /qapi-event.[ch]
 /qmp-commands.h
+/qmp-introspect.[ch]
 /qmp-marshal.c
 /qemu-doc.html
 /qemu-tech.html
diff --git a/Makefile b/Makefile
index 1e26f4d..cda999e 100644
--- a/Makefile
+++ b/Makefile
@@ -47,6 +47,8 @@ endif
 GENERATED_HEADERS = config-host.h qemu-options.def
 GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
 GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c
+GENERATED_HEADERS += qmp-introspect.h
+GENERATED_SOURCES += qmp-introspect.c
 
 GENERATED_HEADERS += trace/generated-events.h
 GENERATED_SOURCES += trace/generated-events.c
@@ -258,7 +260,7 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 
 qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
                $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
-               $(SRC_PATH)/qapi/event.json
+               $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json
 
 qapi-types.c qapi-types.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
@@ -280,6 +282,11 @@ $(qapi-modules) $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
 		$(gen-out-type) -o "." -m $<, \
 		"  GEN   $@")
+qmp-introspect.h qmp-introspect.c :\
+$(qapi-modules) $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py \
+		$(gen-out-type) -o "." $<, \
+		"  GEN   $@")
 
 QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
 $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
diff --git a/Makefile.objs b/Makefile.objs
index 28999d3..414229f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -80,6 +80,7 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y)
 # qapi
 
 common-obj-y += qmp-marshal.o
+common-obj-y += qmp-introspect.o
 common-obj-y += qmp.o hmp.o
 endif
 
diff --git a/monitor.c b/monitor.c
index 68873ec..fcb9242 100644
--- a/monitor.c
+++ b/monitor.c
@@ -73,6 +73,7 @@
 #include "block/qapi.h"
 #include "qapi/qmp-event.h"
 #include "qapi-event.h"
+#include "qmp-introspect.h"
 #include "sysemu/block-backend.h"
 
 /* for hmp_info_irq/pic */
@@ -997,6 +998,13 @@ EventInfoList *qmp_query_events(Error **errp)
     return ev_list;
 }
 
+static int qmp_query_schema(Monitor *mon, const QDict *qdict,
+                            QObject **ret_data)
+{
+    *ret_data = qobject_from_json(qmp_schema_json);
+    return 0;
+}
+
 /* set the current CPU defined by the user */
 int monitor_set_cpu(int cpu_index)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index 6b280b7..0efeb80 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -14,6 +14,9 @@
 # Tracing commands
 { 'include': 'qapi/trace.json' }
 
+# QAPI introspection
+{ 'include': 'qapi/introspect.json' }
+
 ##
 # LostTickPolicy:
 #
diff --git a/qapi/introspect.json b/qapi/introspect.json
new file mode 100644
index 0000000..87de856
--- /dev/null
+++ b/qapi/introspect.json
@@ -0,0 +1,72 @@
+# -*- Mode: Python -*-
+#
+# QAPI introspection
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# Authors:
+#  Markus Armbruster <armbru@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+{ 'enum': 'SchemaMetaType',
+  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
+            'command', 'event' ] }
+
+{ 'type': 'SchemaInfoBase',
+  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
+
+{ 'enum': 'JSONPrimitiveType',
+  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
+
+{ 'type': 'SchemaInfoBuiltin',
+  'data': { 'json-type': 'JSONPrimitiveType' } }
+
+{ 'type': 'SchemaInfoEnum',
+  'data': { 'values': ['str'] } }
+
+{ 'type': 'SchemaInfoArray',
+  'data': { 'element-type': 'str' } }
+
+{ 'alternate': 'Value',
+  'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool': 'bool' } }
+
+{ 'type': 'SchemaInfoObjectMember',
+  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
+# @default's type must match @type
+# can only default simple types, not objects or arrays
+
+{ 'type': 'SchemaInfoObjectVariant',
+  'data': { 'case': 'str',
+            'members': [ 'SchemaInfoObjectMember' ] } }
+
+{ 'type': 'SchemaInfoObject',
+  'data': { 'members': [ 'SchemaInfoObjectMember' ],
+            '*discriminator': 'str',
+            '*variants': [ 'SchemaInfoObjectVariant' ] } }
+
+{ 'type': 'SchemaInfoAlternate',
+  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }
+
+{ 'type': 'SchemaInfoCommand',
+  'data': { 'args': 'str', 'returns': 'str' } }
+
+{ 'type': 'SchemaInfoEvent',
+  'data': { 'data': 'str' } }
+
+{ 'union': 'SchemaInfo',
+  'base': 'SchemaInfoBase',
+  'discriminator': 'meta-type',
+  'data': {
+      'builtin': 'SchemaInfoBuiltin',
+      'enum': 'SchemaInfoEnum',
+      'array': 'SchemaInfoArray',
+      'object': 'SchemaInfoObject',
+      'alternate': 'SchemaInfoAlternate',
+      'command': 'SchemaInfoCommand',
+      'event': 'SchemaInfoEvent' } }
+
+{ 'command': 'query-schema',
+  'returns': [ 'SchemaInfo' ],
+  'gen': false }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7f68760..27ab209 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2039,6 +2039,22 @@ EQMP
     },
 
 SQMP
+query-schema
+------------
+
+Return the QMP schema.
+
+FIXME explain
+
+EQMP
+
+    {
+        .name       = "query-schema",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_query_schema,
+    },
+
+SQMP
 query-chardev
 -------------
 
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
new file mode 100644
index 0000000..c09276b
--- /dev/null
+++ b/scripts/qapi-introspect.py
@@ -0,0 +1,430 @@
+#
+# QAPI introspection generator
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# Authors:
+#  Markus Armbruster <armbru@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+from ordereddict import OrderedDict
+from qapi import *
+import getopt
+import errno
+
+class QAPISchemaEntity:
+    def __init__(self, name, info):
+        assert isinstance(name, str)
+        self.name = name
+        self.info = info
+    def check(self):
+        pass
+    def collect(self, seen):
+        seen.add(self.name)
+
+entity_dict = {}
+
+def def_entity(ent):
+    assert ent.name not in entity_dict
+    entity_dict[ent.name] = ent
+
+def gen_json_helper(name, mtype, extra):
+    if extra:
+        extra = ", " + extra
+    else:
+        extra = ""
+    return "{ 'name': '%s', 'meta-type': '%s'%s }" % (name, mtype, extra)
+
+class QAPISchemaType(QAPISchemaEntity):
+    pass
+
+def get_type(name):
+    if isinstance(entity_dict.get(name), QAPISchemaType):
+        return entity_dict[name]
+    return None
+
+def check_schema():
+    for ent in entity_dict.values():
+        ent.check()
+
+class QAPISchemaBuiltinType(QAPISchemaType):
+    def __init__(self, name, json_type):
+        QAPISchemaType.__init__(self, name, None)
+        assert json_type in ('string', 'number', 'int', 'boolean', 'null', 'value')
+        self.json_type = json_type
+    def gen_json(self):
+        return gen_json_helper(self.name, 'builtin',
+                               "'json-type': '%s'" % self.json_type)
+
+class QAPISchemaEnumType(QAPISchemaType):
+    def __init__(self, name, info, values):
+        QAPISchemaType.__init__(self, name, info)
+        for v in values:
+            assert isinstance(v, str)
+        self.values = values
+    def gen_json(self):
+        return gen_json_helper(self.name, 'enum',
+                               "'values': [ %s ]" % ", ".join(map(lambda v: "'%s'" % v, self.values)))
+
+def make_implicit_enum_type(name, role, values):
+    name = ':enum-%s-%s' % (name, role)
+    def_entity(QAPISchemaEnumType(name, None, values))
+    return name
+
+class QAPISchemaArrayType(QAPISchemaType):
+    def __init__(self, name, info, element_type):
+        QAPISchemaType.__init__(self, name, info)
+        assert isinstance(element_type, str)
+        self.element_type = element_type
+    def check(self):
+        assert get_type(self.element_type)
+    def collect(self, seen):
+        seen.add(self.name)
+        get_type(self.element_type).collect(seen)
+    def gen_json(self):
+        return gen_json_helper(self.name, 'array',
+                               "'element-type': '%s'" % self.element_type)
+
+def make_array_type(element_type):
+    name = ':[%s]' % element_type
+    if not get_type(name):
+        def_entity(QAPISchemaArrayType(name, None, element_type))
+    return name
+
+class QAPISchemaObjectType(QAPISchemaType):
+    def __init__(self, name, info, base, local_members, variants):
+        QAPISchemaType.__init__(self, name, info)
+        assert base == None or isinstance(base, str)
+        assert isinstance(local_members, QAPISchemaObjectTypeMembers)
+        if variants != None:
+            assert isinstance(variants, QAPISchemaObjectTypeVariants)
+        self.base = base
+        self.local_members = local_members
+        self.variants = variants
+        self.members = None
+    def check(self):
+        if self.members:
+            return                      # already checked
+        assert self.members == None     # not running in cycles
+        self.members = False            # mark as being checked
+        if self.base:
+            base = get_type(self.base)
+            base.check()
+            members = list(base.members.members)
+        else:
+            base = None
+            members = []
+        seen = {}
+        for m in members:
+            seen[m.name] = m
+        self.local_members.check(members, seen)
+        if self.variants:
+            self.variants.check(seen)
+        self.members = QAPISchemaObjectTypeMembers(members)
+    def collect(self, seen):
+        if self.name in seen:
+            return
+        seen.add(self.name)
+        self.members.collect(seen)
+        if self.variants:
+            self.variants.collect(seen)
+    def gen_json(self):
+        extra = "'members': %s" % self.members.gen_json()
+        if self.variants:
+            extra += ", %s" % self.variants.gen_json()
+        return gen_json_helper(self.name, 'object', extra)
+
+def make_implicit_object_type(name, role, members):
+    if members.members == []:
+        name = ':empty'
+        if get_type(name):
+            return name
+    else:
+        name = ':obj-%s-%s' % (name, role)
+    def_entity(QAPISchemaObjectType(name, None, None, members, None))
+    return name
+
+class QAPISchemaObjectTypeMember:
+    def __init__(self, name, typ, optional):
+        assert isinstance(name, str)
+        assert isinstance(typ, str)
+        assert isinstance(optional, bool)
+        self.name = name
+        self.type = typ
+        self.optional = optional
+    def check(self, seen):
+        assert self.name not in seen
+        assert get_type(self.type)
+    def collect(self, seen):
+        get_type(self.type).collect(seen)
+    def gen_json(self):
+        default = ''
+        if self.optional:
+            default = ", 'default': null"
+        return "{ 'name': '%s', 'type': '%s'%s }" % (self.name, self.type, default)
+
+class QAPISchemaObjectTypeMembers:
+    def __init__(self, members):
+        for m in members:
+            assert isinstance(m, QAPISchemaObjectTypeMember)
+        self.members = members
+    def check(self, all_members, seen):
+        for m in self.members:
+            m.check(seen)
+            all_members.append(m)
+            seen[m.name] = m
+    def collect(self, seen):
+        for m in self.members:
+            m.collect(seen)
+    def gen_json(self):
+        return "[ " + ", ".join(map(lambda m: m.gen_json(), self.members)) + " ]"
+
+class QAPISchemaObjectTypeVariant:
+    def __init__(self, case, members):
+        assert isinstance(case, str)
+        if not isinstance(members, str):
+            assert isinstance(members, QAPISchemaObjectTypeMembers)
+        self.case = case
+        self.members = members
+    def check(self, disc_type, seen):
+        assert self.case in disc_type.values
+        if isinstance(self.members, str):
+            get_type(self.members).check()
+            self.members = get_type(self.members).members
+        self.members.check([], seen)
+    def collect(self, seen):
+        self.members.collect(seen)
+    def gen_json(self):
+        return "{ 'case': '%s', 'members': %s }" % (self.case, self.members.gen_json())
+
+class QAPISchemaObjectTypeVariants:
+    def __init__(self, discriminator, variants):
+        assert isinstance(discriminator, str)
+        for v in variants:
+            assert isinstance(v, QAPISchemaObjectTypeVariant)
+        self.discriminator = discriminator
+        self.variants = variants
+    def check(self, seen):
+        disc_type = get_type(seen[self.discriminator].type)
+        assert isinstance(disc_type, QAPISchemaEnumType)
+        for v in self.variants:
+            vseen = dict(seen)
+            v.check(disc_type, vseen)
+    def collect(self, seen):
+        for v in self.variants:
+            v.collect(seen)
+    def gen_json(self):
+        return "'discriminator': '%s', 'variants': [ %s ]" % (self.discriminator, ", ".join(map(lambda v: v.gen_json(), self.variants)))
+
+class QAPISchemaAlternateType(QAPISchemaType):
+    def __init__(self, name, info, members):
+        QAPISchemaType.__init__(self, name, info)
+        assert isinstance(members, QAPISchemaObjectTypeMembers)
+        self.members = members
+    def check(self):
+        self.members.check([], {})
+    def collect(self, seen):
+        if self.name in seen:
+            return
+        seen.add(self.name)
+        self.members.collect(seen)
+    def gen_json(self):
+        return gen_json_helper(self.name, 'alternate',
+                               "'members': %s" % self.members.gen_json())
+
+class QAPISchemaCommand(QAPISchemaEntity):
+    def __init__(self, name, info, args, rets):
+        QAPISchemaEntity.__init__(self, name, info)
+        if not isinstance(args, str):
+            assert isinstance(args, QAPISchemaObjectTypeMembers)
+        if not isinstance(rets, str):
+            assert isinstance(rets, QAPISchemaObjectTypeMembers)
+        self.args = args
+        self.rets = rets
+    def check(self):
+        if not isinstance(self.args, str):
+            self.args = make_implicit_object_type(self.name, 'args', self.args)
+            entity_dict[self.args].check()
+        if not isinstance(self.rets, str):
+            self.rets = make_implicit_object_type(self.name, 'rets', self.rets)
+            entity_dict[self.rets].check()
+    def collect(self, seen):
+        seen.add(self.name)
+        get_type(self.args).collect(seen)
+        get_type(self.rets).collect(seen)
+    def gen_json(self):
+        return gen_json_helper(self.name, 'command',
+                               "'args': '%s', 'returns': '%s'" % (self.args, self.rets))
+
+class QAPISchemaEvent(QAPISchemaEntity):
+    def __init__(self, name, info, data):
+        QAPISchemaEntity.__init__(self, name, info)
+        if not isinstance(data, str):
+            assert isinstance(data, QAPISchemaObjectTypeMembers)
+        self.data = data
+    def check(self):
+        if not isinstance(self.data, str):
+            self.data = make_implicit_object_type(self.name, 'data', self.data)
+            entity_dict[self.data].check()
+    def collect(self, seen):
+        seen.add(self.name)
+        get_type(self.data).collect(seen)
+    def gen_json(self):
+        return gen_json_helper(self.name, 'event', "'data': '%s'" % self.data)
+
+def def_builtin_types():
+    def_entity(QAPISchemaBuiltinType('str', 'string'))
+    def_entity(QAPISchemaBuiltinType('number', 'number'))
+    for i in ('int', 'int8', 'int16', 'int32', 'int64',
+              'uint8', 'uint16', 'uint32', 'uint64', 'size'):
+        def_entity(QAPISchemaBuiltinType(i, 'int'))
+    def_entity(QAPISchemaBuiltinType('bool', 'boolean'))
+    def_entity(QAPISchemaBuiltinType('**', 'value'))
+
+def def_enum_type(expr):
+    def_entity(QAPISchemaEnumType(expr['enum'], None, expr['data']))
+
+def make_member(name, typ):
+    optional = False
+    if name.startswith('*'):
+        name = name[1:]
+        optional = True
+    if isinstance(typ, list):
+        typ = make_array_type(typ[0])
+    return QAPISchemaObjectTypeMember(name, typ, optional)
+
+def make_members(data):
+    return QAPISchemaObjectTypeMembers(map(lambda mname: make_member(mname, data[mname]), data))
+
+def make_flat_variant(case, typ):
+    return QAPISchemaObjectTypeVariant(case, typ)
+
+def make_simple_variant(case, typ):
+    return QAPISchemaObjectTypeVariant(case,
+                                       make_members(OrderedDict({ 'data': typ })))
+
+def def_struct_type(expr):
+    def_entity(QAPISchemaObjectType(expr['type'], None, expr.get('base'),
+                                    make_members(expr['data']), None))
+
+def def_union_type(expr):
+    name = expr['union']
+    data = expr['data']
+    discriminator = expr.get('discriminator')
+    if discriminator:
+        base = expr['base']
+        members = OrderedDict()
+        variants = map(lambda dval: make_flat_variant(dval, data[dval]),
+                       data)
+    else:
+        base = None
+        enum = make_implicit_enum_type(name, 'kind', data.keys())
+        members = OrderedDict({ 'type': enum })
+        discriminator = 'type'
+        variants = map(lambda dval: make_simple_variant(dval, data[dval]),
+                       data)
+    def_entity(QAPISchemaObjectType(name, None, base,
+                                    make_members(members),
+                                    QAPISchemaObjectTypeVariants(discriminator,
+                                                                 variants)))
+
+def def_alternate_type(expr):
+    def_entity(QAPISchemaAlternateType(expr['alternate'], None,
+                                       make_members(expr['data'])))
+
+def def_command(expr):
+    args = expr.get('data', {})
+    rets = expr.get('returns', {})
+    if not isinstance(args, str):
+        args = make_members(args)
+    if isinstance(rets, list):
+        assert len(rets) == 1
+        rets = make_array_type(rets[0])
+    elif not isinstance(rets, str):
+        rets = make_members(rets)
+    def_entity(QAPISchemaCommand(expr['command'], None, args, rets))
+
+def def_event(expr):
+    data = expr.get('data', {})
+    if not isinstance(data, str):
+        data = make_members(data)
+    def_entity(QAPISchemaEvent(expr['event'], None, data))
+
+def def_schema(exprs):
+    for expr in exprs:
+        if 'enum' in expr:
+            def_enum_type(expr)
+        elif 'type' in expr:
+            def_struct_type(expr)
+        elif 'union' in expr:
+            def_union_type(expr)
+        elif 'alternate' in expr:
+            def_alternate_type(expr)
+        elif 'command' in expr:
+            def_command(expr)
+        elif 'event' in expr:
+            def_event(expr)
+        else:
+            assert False
+
+def gen_qmp_schema_json():
+    seen = set()
+    for ent in entity_dict.values():
+        if not isinstance(ent, QAPISchemaType):
+            ent.collect(seen)
+    return filter(None, map(lambda name: entity_dict[name].gen_json(),
+                            sorted(seen)))
+
+(input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
+
+c_comment = '''
+/*
+ * QAPI/QMP schema introspection
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
+h_comment = '''
+/*
+ * QAPI/QMP schema introspection
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+'''
+
+(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
+                            'qmp-introspect.c', 'qmp-introspect.h',
+                            c_comment, h_comment)
+
+fdef.write(mcgen('''
+#include "%(prefix)sqmp-introspect.h"
+
+''',
+                 prefix=prefix))
+
+fdecl.write(mcgen('''
+extern char qmp_schema_json[];
+'''))
+
+exprs = parse_schema(input_file)
+
+def_builtin_types()
+def_schema(exprs)
+check_schema()
+
+jsons = gen_qmp_schema_json()
+fdef.write('char qmp_schema_json[] = "["\n    "'
+           + ',"\n    "'.join(jsons)
+           + ']";\n')
+
+close_output(fdef, fdecl)
diff --git a/tests/.gitignore b/tests/.gitignore
index 0dcb618..a583380 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -18,6 +18,7 @@ test-opts-visitor
 test-qapi-event.[ch]
 test-qapi-types.[ch]
 test-qapi-visit.[ch]
+test-qapi-introspect.[ch]
 test-qdev-global-props
 test-qemu-opts
 test-qmp-commands
diff --git a/tests/Makefile b/tests/Makefile
index 597919c..a3a171f 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -241,7 +241,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
 	include-repetition.json event-nest-struct.json event-case.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
-		     tests/test-qmp-commands.h tests/test-qapi-event.h
+		     tests/test-qmp-commands.h tests/test-qapi-event.h \
+                     tests/test-qmp-introspect.h
 
 test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/check-qlist.o tests/check-qfloat.o tests/check-qjson.o \
@@ -314,6 +315,11 @@ $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-eve
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \
 		$(gen-out-type) -o tests -p "test-" $<, \
 		"  GEN   $@")
+tests/test-qmp-introspect.c tests/test-qmp-introspect.h :\
+$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py \
+		$(gen-out-type) -o tests -p "test-" $<, \
+		"  GEN   $@")
 
 tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
 tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 14286 bytes --]

On 04/02/2015 11:29 AM, Markus Armbruster wrote:
> Caution, rough edges.
> 
> qapi/introspect.json defines the introspection schema.  It should do
> for uses other than QMP.

That is, QGA should also be able to reuse it for introspection.

> FIXME it's almost entirely devoid of comments.

Yeah, but it's only RFC quality, and if we alter design you don't have
to worry about lengthy comments to keep in sync :)  Of course, the final
version ought to have good comments.

> 
> The introspection schema does not reflect all the rules and
> restrictions that apply to QAPI schemata.  A valid QAPI schema has an
> introspection value conforming to the introspection schema, but the
> converse is not true.

I can live with that.  Introspection is output-only (we aren't creating
new runtime commands by using introspection descriptions as input, so it
doesn't matter if the introspection schema permits more than qapi will
ever generate).

> 
> Introspection lowers away a number of schema details:
> 
> * The builtin types are declared with their JSON type.
> 
>   TODO should we map all the integer types to just int?

Or even have introspection output two things: type ('int' for all JSON
integers, regardless of range) and range ([0,255] for 'uint8').  I could
live with that (ideally, knowing the range will help libvirt avoid
passing in too-large-a-value to a parameter it introspected; but right
now the idea is that most of libvirt's introspection will be "does
something exist" not "what range does it support", and that libvirt will
already have hard-coded knowledge of "if it exists, it is a uint8"
without having to ask introspection for the type libvirt will supply).

> 
> * Implicit type definitions are made explicit, and given
>   auto-generated names.  These names start with ':' so they don't
>   clash with the user's names.

Hopefully that works.  Although having to parse for ':' is an argument
that we are packing multiple pieces of information in one JSON
name/value, which sometimes means we should have instead supplied two
different name/values for the two different pieces of information. Oh
well, I'll see more when I get into the schema part.

Maybe we could also tweak the generator to put implicit names in the
leading '_' namespace (we already outlaw use of leading '_' in all but
downstream names, and require downstream names to use double '__', so we
could easily identify '_generated' and '___generated' as internal vs.
'regular' and '__downstream' as user-supplied; it would free up current
places where the qapi user cannot supply certain names. But not for this
series).

> 
>   Example: a simple union implicitly defines an enumeration type for
>   its discriminator.
> 
> * All type references are by name.

Fine if the type is named; but what happens if the type is anonymous
inline (as in many commands?)  Also, in my nested struct cleanups, I
found several places where it would be nice to patch the generator to
support more anonymous inline types, such as in:

{ 'enum': 'MyEnum', 'data': [ 'one', 'two' ] }
{ 'union': 'Foo', 'base': { 'switch': 'MyEnum', 'name': 'str' },
  'discriminator': 'switch', data': {
    'one': { 'value': 'int' },
    'two': { 'default': 'int' } } }

It would even work for our long-hand of optional arguments:
{ 'command': 'foo', 'data': {
  'bar': { 'type': { 'name': 'str', 'value': 'int' },
           'optional': true } } }

I guess we can always go with anonymous inline types resulting in an
implicit named type creation, and refer to that name, if cramming an
anonymous type into the schema is too hard.

> 
> * Base types are flattened.

That's fine - we are introspecting what can be sent on the wire, and
don't care what types were used in the qapi to arrive at that point.

> 
> * The top type (named '**') can hold any value.
> 
>   It can currently occur only in commands, but the introspection
>   schema doesn't reflect that.
> 
> * The struct and union types are generalized into an object type.

I think you mentioned ideas on that in reviewing my v5 thread; they
sounded reasonable at the time, so we'll see when I actually review the
schema.

> 
> * Commands take a single argument and return a single result.
> 
>   Dictionary argument/result or list result is an implicit type
>   definition.
> 
>   Missing argument/result is an implicit definition of the empty
>   object.
> 
>   The argument is always of object type, but the introspection schema
>   doesn't reflect that.
> 
>   The 'gen': false directive is omitted as implementation detail.

Yep, that's fine. QAPI drives internal details that don't affect the
wire format, and it's fine that introspection doesn't have to expose them.

> 
> * Events carry a single data value.
> 
>   Implicit type definition as above.
> 
>   The value is of object type, but the introspection schema doesn't
>   reflect that.
> 
> * Types not used by commands or events are omitted.
> 
>   Indirect use counts as use.
> 
> * Optional members have a default, which can only be null right now

Even for integral members?

> 
>   Instead of a mandatory "optional" flag, we have an optionial

s/optionial/optional/

>   default.  No default means mandatory, default null means optional
>   without default value.  Non-null is available for optional with
>   default.
> 
> TODO much of the above should go into docs.
> 
> New generator scripts/qapi-introspect.py computes an introspection
> value for its input, and generates a C variable holding it.
> Pythonistas may find it ugly and/or clumsy.

I'm not one, so my review will (happily?) paper over the clumsy usage.

> 
> FIXME it can generate awfully long lines

Not the first of our generators with that problem.  But even tougher if
you are generating C strings for highly-complex types.

> 
> The schema generation proper is pretty trivial.  Much of the
> qapi-introspect.py's code actually does something else: convert the
> output of parse_schema() into a more usable data structure, lowering
> away uninteresting stuff.  This should really be done in qapi.py, so
> the other generators can use it, too.

And we may want to start by breaking that out into separate commits.

> 
> For testing, I feed it tests/qapi-schema/qapi-schema-test.json, but no
> more.

That's actually a good test - it tries to cover as much of our generator
as possible, without too many repetitions of types of objects.

> 
> New QMP command query-schema parses its return value from that
> variable.  Its reply is less than 75KiBytes right now.

A bit heavy; implies that we might want it to support optional
filtering. But that can be added on top.

> 
> A compile time test that the value can be parsed would be nice.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  .gitignore                 |   1 +
>  Makefile                   |   9 +-
>  Makefile.objs              |   1 +
>  monitor.c                  |   8 +
>  qapi-schema.json           |   3 +
>  qapi/introspect.json       |  72 ++++++++
>  qmp-commands.hx            |  16 ++
>  scripts/qapi-introspect.py | 430 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore           |   1 +
>  tests/Makefile             |   8 +-

No docs/qapi-code-gen.txt changes? (Oh right, you said so, for the RFC...)

>  10 files changed, 547 insertions(+), 2 deletions(-)
>  create mode 100644 qapi/introspect.json
>  create mode 100644 scripts/qapi-introspect.py

Should it be 755?  Oh, none of the other qapi*.py are.

> +++ b/monitor.c
> @@ -73,6 +73,7 @@
>  #include "block/qapi.h"
>  #include "qapi/qmp-event.h"
>  #include "qapi-event.h"
> +#include "qmp-introspect.h"
>  #include "sysemu/block-backend.h"
>  
>  /* for hmp_info_irq/pic */
> @@ -997,6 +998,13 @@ EventInfoList *qmp_query_events(Error **errp)
>      return ev_list;
>  }
>  
> +static int qmp_query_schema(Monitor *mon, const QDict *qdict,
> +                            QObject **ret_data)
> +{
> +    *ret_data = qobject_from_json(qmp_schema_json);
> +    return 0;

Deceptively simple - it means everything is known at compile-time (no
run-time filtering for conditionally implemented commands; might become
important later on for listing only whitelisted QGA commands, for example?)

> +}
> +
>  /* set the current CPU defined by the user */
>  int monitor_set_cpu(int cpu_index)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 6b280b7..0efeb80 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -14,6 +14,9 @@
>  # Tracing commands
>  { 'include': 'qapi/trace.json' }
>  
> +# QAPI introspection
> +{ 'include': 'qapi/introspect.json' }
> +
>  ##
>  # LostTickPolicy:
>  #
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> new file mode 100644
> index 0000000..87de856
> --- /dev/null
> +++ b/qapi/introspect.json
> @@ -0,0 +1,72 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI introspection
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# Authors:
> +#  Markus Armbruster <armbru@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +{ 'enum': 'SchemaMetaType',
> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
> +            'command', 'event' ] }

Yep, definitely depends on my work going in first.

'builtin' is not an English word; do we want to go with the longer
"built-in", or is it not worth it?

> +
> +{ 'type': 'SchemaInfoBase',
> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
> +
> +{ 'enum': 'JSONPrimitiveType',
> +  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
> +
> +{ 'type': 'SchemaInfoBuiltin',
> +  'data': { 'json-type': 'JSONPrimitiveType' } }
> +
> +{ 'type': 'SchemaInfoEnum',
> +  'data': { 'values': ['str'] } }

At one point, one thread suggested that we might want to allow QAPI to
support enums with fixed values, as in:

'data': [ {'one': 1}, {'three': 3} ]

I guess returning an array of 'str' for now is safe; because we could
always increase introspection to return an alternate between 'str' and a
formal struct down the road if the user needs to know about enums with
values that are guaranteed (vs. the usual enum that is name-association
only with no guaranteed value).

> +
> +{ 'type': 'SchemaInfoArray',
> +  'data': { 'element-type': 'str' } }

We currently don't allow array of anonymous types, but looks like your
implicit type naming would cover that.

> +
> +{ 'alternate': 'Value',
> +  'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool': 'bool' } }

Nothing for objects?...

> +
> +{ 'type': 'SchemaInfoObjectMember',
> +  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
> +# @default's type must match @type
> +# can only default simple types, not objects or arrays

but then again, that's what you said, in a rare comment.

> +
> +{ 'type': 'SchemaInfoObjectVariant',
> +  'data': { 'case': 'str',
> +            'members': [ 'SchemaInfoObjectMember' ] } }
> +
> +{ 'type': 'SchemaInfoObject',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
> +            '*discriminator': 'str',
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }

I guess since the discriminator is always a JSON string, that the 'str'
here is the type name ('str' or an enum name)?  Or is it the
discriminator member name ('type' on simple unions, whatever
'discriminator' was on flat unions)?  We probably want BOTH pieces of
information, always, particularly if I finish my work on extending
simple unions to have an enum-type discriminator.  On the other hand,
the 'variants' array lists all of the branches, which means I can
reconstruct the enum values (or even the open-coded 'str' values)
without caring whether the discriminator was typed.

So after all that, I guess you are using it as the discriminator member
name and NOT its type.

> +
> +{ 'type': 'SchemaInfoAlternate',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }
> +
> +{ 'type': 'SchemaInfoCommand',
> +  'data': { 'args': 'str', 'returns': 'str' } }

We could always use an alternate to return either a 'str' or a
'SchemaInfoObject' to represent anonymous inline types without having to
go through an implicit generated name.  If that makes any more sense.

> +
> +{ 'type': 'SchemaInfoEvent',
> +  'data': { 'data': 'str' } }

Likewise.

> +
> +{ 'union': 'SchemaInfo',
> +  'base': 'SchemaInfoBase',
> +  'discriminator': 'meta-type',
> +  'data': {
> +      'builtin': 'SchemaInfoBuiltin',
> +      'enum': 'SchemaInfoEnum',
> +      'array': 'SchemaInfoArray',
> +      'object': 'SchemaInfoObject',
> +      'alternate': 'SchemaInfoAlternate',
> +      'command': 'SchemaInfoCommand',
> +      'event': 'SchemaInfoEvent' } }
> +
> +{ 'command': 'query-schema',
> +  'returns': [ 'SchemaInfo' ],
> +  'gen': false }

Overall, looks pretty nice.  But if we reuse this file for both QMP and
QGA, then maybe the 'command' should be in the parent file that includes
this (so that this file is just everything through 'SchemaInfo' to
declare the types, but leaves QGA free to name its command different
than QMP, if desired)

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7f68760..27ab209 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2039,6 +2039,22 @@ EQMP
>      },
>  
>  SQMP
> +query-schema
> +------------
> +
> +Return the QMP schema.
> +
> +FIXME explain
> +
> +EQMP
> +
> +    {
> +        .name       = "query-schema",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_query_schema,
> +    },
> +
> +SQMP
>  query-chardev
>  -------------
>  
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> new file mode 100644
> index 0000000..c09276b
> --- /dev/null
> +++ b/scripts/qapi-introspect.py

I stopped reviewing here for today.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Alberto Garcia]

On Sat 11 Apr 2015 01:06:58 AM CEST, Eric Blake <eblake@redhat.com> wrote:

>> +{ 'type': 'SchemaInfoEnum',
>> +  'data': { 'values': ['str'] } }
>
> At one point, one thread suggested that we might want to allow QAPI to
> support enums with fixed values, as in:
>
> 'data': [ {'one': 1}, {'three': 3} ]

Out of curiosity, what's the use case for that?

Berto

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake]

On 04/15/2015 03:16 AM, Alberto Garcia wrote:
> On Sat 11 Apr 2015 01:06:58 AM CEST, Eric Blake <eblake@redhat.com> wrote:
> 
>>> +{ 'type': 'SchemaInfoEnum',
>>> +  'data': { 'values': ['str'] } }
>>
>> At one point, one thread suggested that we might want to allow QAPI to
>> support enums with fixed values, as in:
>>
>> 'data': [ {'one': 1}, {'three': 3} ]
> 
> Out of curiosity, what's the use case for that?

If we add that extension, it will allow the creation of C enums with
specific values, rather than defaulting to consecutive initialization
from 0.

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

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Kevin Wolf]

Am 15.04.2015 um 14:56 hat Eric Blake geschrieben:
> On 04/15/2015 03:16 AM, Alberto Garcia wrote:
> > On Sat 11 Apr 2015 01:06:58 AM CEST, Eric Blake <eblake@redhat.com> wrote:
> > 
> >>> +{ 'type': 'SchemaInfoEnum',
> >>> +  'data': { 'values': ['str'] } }
> >>
> >> At one point, one thread suggested that we might want to allow QAPI to
> >> support enums with fixed values, as in:
> >>
> >> 'data': [ {'one': 1}, {'three': 3} ]
> > 
> > Out of curiosity, what's the use case for that?
> 
> If we add that extension, it will allow the creation of C enums with
> specific values, rather than defaulting to consecutive initialization
> from 0.

But would we want to expose that on the external API? So far we always
said that the mapping to integer constants is an implementation detail
and can safely be changed.

Kevin

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On 04/23/2015 06:55 AM, Kevin Wolf wrote:
> Am 15.04.2015 um 14:56 hat Eric Blake geschrieben:
>> On 04/15/2015 03:16 AM, Alberto Garcia wrote:
>>> On Sat 11 Apr 2015 01:06:58 AM CEST, Eric Blake <eblake@redhat.com> wrote:
>>>
>>>>> +{ 'type': 'SchemaInfoEnum',
>>>>> +  'data': { 'values': ['str'] } }
>>>>
>>>> At one point, one thread suggested that we might want to allow QAPI to
>>>> support enums with fixed values, as in:
>>>>
>>>> 'data': [ {'one': 1}, {'three': 3} ]
>>>
>>> Out of curiosity, what's the use case for that?
>>
>> If we add that extension, it will allow the creation of C enums with
>> specific values, rather than defaulting to consecutive initialization
>> from 0.
> 
> But would we want to expose that on the external API? So far we always
> said that the mapping to integer constants is an implementation detail
> and can safely be changed.

Oh, good point.  We've already stated that there are some details of
qapi (such as 'gen':false) that are not worth exposing through
introspection.  So even if we ever add specific C values to enums in
qapi, I agree that we don't need to expose it during introspection, so
we wouldn't need to change SchemaInfoEnum.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/02/2015 11:29 AM, Markus Armbruster wrote:
>> Caution, rough edges.
>> 
>> qapi/introspect.json defines the introspection schema.  It should do
>> for uses other than QMP.
>
> That is, QGA should also be able to reuse it for introspection.
>
>> FIXME it's almost entirely devoid of comments.
>
> Yeah, but it's only RFC quality, and if we alter design you don't have
> to worry about lengthy comments to keep in sync :)  Of course, the final
> version ought to have good comments.
>
>> 
>> The introspection schema does not reflect all the rules and
>> restrictions that apply to QAPI schemata.  A valid QAPI schema has an
>> introspection value conforming to the introspection schema, but the
>> converse is not true.
>
> I can live with that.  Introspection is output-only (we aren't creating
> new runtime commands by using introspection descriptions as input, so it
> doesn't matter if the introspection schema permits more than qapi will
> ever generate).
>
>> 
>> Introspection lowers away a number of schema details:
>> 
>> * The builtin types are declared with their JSON type.
>> 
>>   TODO should we map all the integer types to just int?
>
> Or even have introspection output two things: type ('int' for all JSON
> integers, regardless of range) and range ([0,255] for 'uint8').  I could
> live with that (ideally, knowing the range will help libvirt avoid
> passing in too-large-a-value to a parameter it introspected; but right
> now the idea is that most of libvirt's introspection will be "does
> something exist" not "what range does it support", and that libvirt will
> already have hard-coded knowledge of "if it exists, it is a uint8"
> without having to ask introspection for the type libvirt will supply).

I'm reluctant to add range information without a clear use.

Tight range information based on declared types requires range types,
and a lot of discipline using them.

>> * Implicit type definitions are made explicit, and given
>>   auto-generated names.  These names start with ':' so they don't
>>   clash with the user's names.
>
> Hopefully that works.  Although having to parse for ':' is an argument
> that we are packing multiple pieces of information in one JSON
> name/value, which sometimes means we should have instead supplied two
> different name/values for the two different pieces of information. Oh
> well, I'll see more when I get into the schema part.

Design assumption: users don't care whether something was explicitly
named by a human.

Corollary: users are not expected to check for ':'.

> Maybe we could also tweak the generator to put implicit names in the
> leading '_' namespace (we already outlaw use of leading '_' in all but
> downstream names, and require downstream names to use double '__', so we
> could easily identify '_generated' and '___generated' as internal vs.
> 'regular' and '__downstream' as user-supplied; it would free up current
> places where the qapi user cannot supply certain names. But not for this
> series).

Workable if we enforce "no explicit name can start with a single '_'".
I picked ':' for the RFC to simplify my correctness argument.

>>   Example: a simple union implicitly defines an enumeration type for
>>   its discriminator.
>> 
>> * All type references are by name.
>
> Fine if the type is named; but what happens if the type is anonymous
> inline (as in many commands?)

The system generates a type with a made-up name.  Bye, bye, special
case :)

>                                Also, in my nested struct cleanups, I
> found several places where it would be nice to patch the generator to
> support more anonymous inline types, such as in:
>
> { 'enum': 'MyEnum', 'data': [ 'one', 'two' ] }
> { 'union': 'Foo', 'base': { 'switch': 'MyEnum', 'name': 'str' },
>   'discriminator': 'switch', data': {
>     'one': { 'value': 'int' },
>     'two': { 'default': 'int' } } }
>
> It would even work for our long-hand of optional arguments:
> { 'command': 'foo', 'data': {
>   'bar': { 'type': { 'name': 'str', 'value': 'int' },
>            'optional': true } } }
>
> I guess we can always go with anonymous inline types resulting in an
> implicit named type creation, and refer to that name, if cramming an
> anonymous type into the schema is too hard.

Supporting inline types in the schema is fine with me, as long as it
doesn't get in the way of extensibility.

Example of inline types getting in the way: nested structs preclude
clean extension of member declarations for additional properties.
That's why you're removing them.

In introspection context, I view inline vs. explicit type as irrelevant
detail.  Introspection examines the actual structure of things, not how
the schema author chose to write it up.

>> * Base types are flattened.
>
> That's fine - we are introspecting what can be sent on the wire, and
> don't care what types were used in the qapi to arrive at that point.

Exactly.  Same argument as for inline vs. explicit type.

>> * The top type (named '**') can hold any value.
>> 
>>   It can currently occur only in commands, but the introspection
>>   schema doesn't reflect that.
>> 
>> * The struct and union types are generalized into an object type.
>
> I think you mentioned ideas on that in reviewing my v5 thread; they
> sounded reasonable at the time, so we'll see when I actually review the
> schema.
>
>> 
>> * Commands take a single argument and return a single result.
>> 
>>   Dictionary argument/result or list result is an implicit type
>>   definition.
>> 
>>   Missing argument/result is an implicit definition of the empty
>>   object.
>> 
>>   The argument is always of object type, but the introspection schema
>>   doesn't reflect that.
>> 
>>   The 'gen': false directive is omitted as implementation detail.
>
> Yep, that's fine. QAPI drives internal details that don't affect the
> wire format, and it's fine that introspection doesn't have to expose them.
>
>> 
>> * Events carry a single data value.
>> 
>>   Implicit type definition as above.
>> 
>>   The value is of object type, but the introspection schema doesn't
>>   reflect that.
>> 
>> * Types not used by commands or events are omitted.
>> 
>>   Indirect use counts as use.
>> 
>> * Optional members have a default, which can only be null right now
>
> Even for integral members?

Yes, because the schema doesn't support default values, yet.

The obvious introspection solution for the current state of things would
be a flag "optional", present and true when a member is optional.

My solution tries to provide for a future schema extension by default
values: instead of flag "optional", have "default", present and null
when a member is optional.  Value changes from null to the actual
default value when we add default values.

Thus, "'default': null" means "schema declares no default value".  The
meaning of omitting the member is unspecified in the schema then.

>>   Instead of a mandatory "optional" flag, we have an optionial
>
> s/optionial/optional/
>
>>   default.  No default means mandatory, default null means optional
>>   without default value.  Non-null is available for optional with
>>   default.
>> 
>> TODO much of the above should go into docs.
>> 
>> New generator scripts/qapi-introspect.py computes an introspection
>> value for its input, and generates a C variable holding it.
>> Pythonistas may find it ugly and/or clumsy.
>
> I'm not one, so my review will (happily?) paper over the clumsy usage.
>
>> 
>> FIXME it can generate awfully long lines
>
> Not the first of our generators with that problem.  But even tougher if
> you are generating C strings for highly-complex types.

Perhaps we can have a general long-line-breaker to take care of the
problem everywhere.  If long lines bother us.

>> The schema generation proper is pretty trivial.  Much of the
>> qapi-introspect.py's code actually does something else: convert the
>> output of parse_schema() into a more usable data structure, lowering
>> away uninteresting stuff.  This should really be done in qapi.py, so
>> the other generators can use it, too.
>
> And we may want to start by breaking that out into separate commits.

Yup.  I expect that to be at least as much work as creating this first
monolithic version.

>> For testing, I feed it tests/qapi-schema/qapi-schema-test.json, but no
>> more.
>
> That's actually a good test - it tries to cover as much of our generator
> as possible, without too many repetitions of types of objects.
>
>> 
>> New QMP command query-schema parses its return value from that
>> variable.  Its reply is less than 75KiBytes right now.
>
> A bit heavy; implies that we might want it to support optional
> filtering. But that can be added on top.

Clients should query-schema just once.

The value of query-schema is everything reachable from commands and
events.  An obvious filter would be everything reachable from commands
and events the client actually needs.  I doubt that'll reduce the size
all that much.  It'll increase complexity, though.  Got better filtering
ideas?

Here's another way to reduce I/O.  Observe that query-schema's value can
change only when you replace the QEMU binary.  Have a way to query the
hash of the value.  Clients can then cache query-schema's value, and
query-schema only when the hash doesn't match a cached value.

>> A compile time test that the value can be parsed would be nice.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  .gitignore                 |   1 +
>>  Makefile                   |   9 +-
>>  Makefile.objs              |   1 +
>>  monitor.c                  |   8 +
>>  qapi-schema.json           |   3 +
>>  qapi/introspect.json       |  72 ++++++++
>>  qmp-commands.hx            |  16 ++
>>  scripts/qapi-introspect.py | 430
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/.gitignore           |   1 +
>>  tests/Makefile             |   8 +-
>
> No docs/qapi-code-gen.txt changes? (Oh right, you said so, for the RFC...)
>
>>  10 files changed, 547 insertions(+), 2 deletions(-)
>>  create mode 100644 qapi/introspect.json
>>  create mode 100644 scripts/qapi-introspect.py
>
> Should it be 755?  Oh, none of the other qapi*.py are.

They arguably should be, except for qapi.py.

>> +++ b/monitor.c
>> @@ -73,6 +73,7 @@
>>  #include "block/qapi.h"
>>  #include "qapi/qmp-event.h"
>>  #include "qapi-event.h"
>> +#include "qmp-introspect.h"
>>  #include "sysemu/block-backend.h"
>>  
>>  /* for hmp_info_irq/pic */
>> @@ -997,6 +998,13 @@ EventInfoList *qmp_query_events(Error **errp)
>>      return ev_list;
>>  }
>>  
>> +static int qmp_query_schema(Monitor *mon, const QDict *qdict,
>> +                            QObject **ret_data)
>> +{
>> +    *ret_data = qobject_from_json(qmp_schema_json);
>> +    return 0;
>
> Deceptively simple - it means everything is known at compile-time (no
> run-time filtering for conditionally implemented commands; might become
> important later on for listing only whitelisted QGA commands, for example?)

Stupidest solution that could possibly work, i.e. the one you should
always try first :)

If we need to mess with it, we probably want to parse the sucker with
visitors into the generated C data structures.  Could also be useful to
verify the value actually conforms to the introspection schema.

>> +}
>> +
>>  /* set the current CPU defined by the user */
>>  int monitor_set_cpu(int cpu_index)
>>  {
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 6b280b7..0efeb80 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -14,6 +14,9 @@
>>  # Tracing commands
>>  { 'include': 'qapi/trace.json' }
>>  
>> +# QAPI introspection
>> +{ 'include': 'qapi/introspect.json' }
>> +
>>  ##
>>  # LostTickPolicy:
>>  #
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> new file mode 100644
>> index 0000000..87de856
>> --- /dev/null
>> +++ b/qapi/introspect.json
>> @@ -0,0 +1,72 @@
>> +# -*- Mode: Python -*-
>> +#
>> +# QAPI introspection
>> +#
>> +# Copyright (C) 2015 Red Hat, Inc.
>> +#
>> +# Authors:
>> +#  Markus Armbruster <armbru@redhat.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +# See the COPYING file in the top-level directory.
>> +
>> +{ 'enum': 'SchemaMetaType',
>> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
>> +            'command', 'event' ] }
>
> Yep, definitely depends on my work going in first.
>
> 'builtin' is not an English word; do we want to go with the longer
> "built-in", or is it not worth it?

Matter of taste.  Whatever is more consistent with usage elsewhere gets
my vote.

>> +
>> +{ 'type': 'SchemaInfoBase',
>> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
>> +
>> +{ 'enum': 'JSONPrimitiveType',
>> +  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
>> +
>> +{ 'type': 'SchemaInfoBuiltin',
>> +  'data': { 'json-type': 'JSONPrimitiveType' } }
>> +
>> +{ 'type': 'SchemaInfoEnum',
>> +  'data': { 'values': ['str'] } }
>
> At one point, one thread suggested that we might want to allow QAPI to
> support enums with fixed values, as in:
>
> 'data': [ {'one': 1}, {'three': 3} ]
>
> I guess returning an array of 'str' for now is safe; because we could
> always increase introspection to return an alternate between 'str' and a
> formal struct down the road if the user needs to know about enums with
> values that are guaranteed (vs. the usual enum that is name-association
> only with no guaranteed value).

The numeric value is an implementation detail, as Kevin pointed out.

>> +
>> +{ 'type': 'SchemaInfoArray',
>> +  'data': { 'element-type': 'str' } }
>
> We currently don't allow array of anonymous types, but looks like your
> implicit type naming would cover that.

Yup.  Treating anonymous types that way avoids special restrictions.

>> +
>> +{ 'alternate': 'Value',
>> + 'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool':
>> bool' } }
>
> Nothing for objects?...

What type could I use there?  Closest we have is '**', but that doesn't
work here.

Unless we decide not to support object defaults, we better figure out
how we want to do them here.  We don't have to actually implement them
right away, of course.

>> +
>> +{ 'type': 'SchemaInfoObjectMember',
>> +  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
>> +# @default's type must match @type
>> +# can only default simple types, not objects or arrays
>
> but then again, that's what you said, in a rare comment.
>
>> +
>> +{ 'type': 'SchemaInfoObjectVariant',
>> +  'data': { 'case': 'str',
>> +            'members': [ 'SchemaInfoObjectMember' ] } }
>> +
>> +{ 'type': 'SchemaInfoObject',
>> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
>> +            '*discriminator': 'str',
>> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }
>
> I guess since the discriminator is always a JSON string, that the 'str'
> here is the type name ('str' or an enum name)?  Or is it the
> discriminator member name ('type' on simple unions, whatever
> 'discriminator' was on flat unions)?  We probably want BOTH pieces of
> information, always, particularly if I finish my work on extending
> simple unions to have an enum-type discriminator.  On the other hand,
> the 'variants' array lists all of the branches, which means I can
> reconstruct the enum values (or even the open-coded 'str' values)
> without caring whether the discriminator was typed.
>
> So after all that, I guess you are using it as the discriminator member
> name and NOT its type.

Correct.

>> +
>> +{ 'type': 'SchemaInfoAlternate',
>> +  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }
>> +
>> +{ 'type': 'SchemaInfoCommand',
>> +  'data': { 'args': 'str', 'returns': 'str' } }
>
> We could always use an alternate to return either a 'str' or a
> 'SchemaInfoObject' to represent anonymous inline types without having to
> go through an implicit generated name.  If that makes any more sense.

Yes, but I rather like the simplicity of having only named types.

>> +
>> +{ 'type': 'SchemaInfoEvent',
>> +  'data': { 'data': 'str' } }
>
> Likewise.
>
>> +
>> +{ 'union': 'SchemaInfo',
>> +  'base': 'SchemaInfoBase',
>> +  'discriminator': 'meta-type',
>> +  'data': {
>> +      'builtin': 'SchemaInfoBuiltin',
>> +      'enum': 'SchemaInfoEnum',
>> +      'array': 'SchemaInfoArray',
>> +      'object': 'SchemaInfoObject',
>> +      'alternate': 'SchemaInfoAlternate',
>> +      'command': 'SchemaInfoCommand',
>> +      'event': 'SchemaInfoEvent' } }
>> +
>> +{ 'command': 'query-schema',
>> +  'returns': [ 'SchemaInfo' ],
>> +  'gen': false }
>
> Overall, looks pretty nice.  But if we reuse this file for both QMP and
> QGA, then maybe the 'command' should be in the parent file that includes
> this (so that this file is just everything through 'SchemaInfo' to
> declare the types, but leaves QGA free to name its command different
> than QMP, if desired)

Working assumption: QGA is syntactically just like QMP.  Michael, can
you think of a reason why QGA could require introspection differences?

>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 7f68760..27ab209 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2039,6 +2039,22 @@ EQMP
>>      },
>>  
>>  SQMP
>> +query-schema
>> +------------
>> +
>> +Return the QMP schema.
>> +
>> +FIXME explain
>> +
>> +EQMP
>> +
>> +    {
>> +        .name       = "query-schema",
>> +        .args_type  = "",
>> +        .mhandler.cmd_new = qmp_query_schema,
>> +    },
>> +
>> +SQMP
>>  query-chardev
>>  -------------
>>  
>> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
>> new file mode 100644
>> index 0000000..c09276b
>> --- /dev/null
>> +++ b/scripts/qapi-introspect.py
>
> I stopped reviewing here for today.

Thanks!

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Kevin Wolf]

One question in addition to what Eric already asked:

Am 02.04.2015 um 19:29 hat Markus Armbruster geschrieben:
> +{ 'enum': 'SchemaMetaType',
> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
> +            'command', 'event' ] }
> +
> +{ 'type': 'SchemaInfoBase',
> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
> +
> +{ 'enum': 'JSONPrimitiveType',
> +  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
> +
> +{ 'type': 'SchemaInfoBuiltin',
> +  'data': { 'json-type': 'JSONPrimitiveType' } }
> +
> +{ 'type': 'SchemaInfoEnum',
> +  'data': { 'values': ['str'] } }
> +
> +{ 'type': 'SchemaInfoArray',
> +  'data': { 'element-type': 'str' } }
> +
> +{ 'alternate': 'Value',
> +  'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool': 'bool' } }
> +
> +{ 'type': 'SchemaInfoObjectMember',
> +  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
> +# @default's type must match @type
> +# can only default simple types, not objects or arrays
> +
> +{ 'type': 'SchemaInfoObjectVariant',
> +  'data': { 'case': 'str',
> +            'members': [ 'SchemaInfoObjectMember' ] } }
> +
> +{ 'type': 'SchemaInfoObject',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
> +            '*discriminator': 'str',
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }
> +
> +{ 'type': 'SchemaInfoAlternate',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }

I'm not sure if I understand correctly how this one works. My guess
would be this:

- The elements in members describe the different variants. That is, you
  choose of the members, whereas in SchemaInfoObject all of the members
  exist. Perhaps they should be using different names then?

- The type (= discriminator) of a variant is indirectly specified by the
  'type' of the SchemaInfoObjectMember: This is a QAPI type, but the
  JSON type can be resolved by checking 'meta-type' of the QAPI type
  (and SchemaInfoBuiltin if necessary)

- The 'name' in SchemaInfoObjectMember could be the key name of the
  variant in the alternate definition. However, this name isn't used
  anywhere in the QMP protocol, so maybe it is better kept internal.
  'name' isn't optional, though.

- The 'default' of SchemaInfoObjectMember must be omitted.

Do I understand the intention reasonably right? If so, maybe it would be
better to use a separate type for alternate variants.

Kevin

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> One question in addition to what Eric already asked:
>
> Am 02.04.2015 um 19:29 hat Markus Armbruster geschrieben:
>> +{ 'enum': 'SchemaMetaType',
>> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
>> +            'command', 'event' ] }
>> +
>> +{ 'type': 'SchemaInfoBase',
>> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
>> +
>> +{ 'enum': 'JSONPrimitiveType',
>> +  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
>> +
>> +{ 'type': 'SchemaInfoBuiltin',
>> +  'data': { 'json-type': 'JSONPrimitiveType' } }
>> +
>> +{ 'type': 'SchemaInfoEnum',
>> +  'data': { 'values': ['str'] } }
>> +
>> +{ 'type': 'SchemaInfoArray',
>> +  'data': { 'element-type': 'str' } }
>> +
>> +{ 'alternate': 'Value',
>> + 'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool':
>> bool' } }
>> +
>> +{ 'type': 'SchemaInfoObjectMember',
>> +  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
>> +# @default's type must match @type
>> +# can only default simple types, not objects or arrays
>> +
>> +{ 'type': 'SchemaInfoObjectVariant',
>> +  'data': { 'case': 'str',
>> +            'members': [ 'SchemaInfoObjectMember' ] } }
>> +
>> +{ 'type': 'SchemaInfoObject',
>> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
>> +            '*discriminator': 'str',
>> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }
>> +
>> +{ 'type': 'SchemaInfoAlternate',
>> +  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }
>
> I'm not sure if I understand correctly how this one works. My guess
> would be this:
>
> - The elements in members describe the different variants. That is, you
>   choose of the members, whereas in SchemaInfoObject all of the members
>   exist.

Correct.

>          Perhaps they should be using different names then?

I simply reused SchemaInfoObjectMember.  Similar to how C's struct and
union share member declaration syntax.

> - The type (= discriminator) of a variant is indirectly specified by the
>   'type' of the SchemaInfoObjectMember: This is a QAPI type, but the
>   JSON type can be resolved by checking 'meta-type' of the QAPI type
>   (and SchemaInfoBuiltin if necessary)

Correct.

The introspection schema doesn't reflect the "the variant's JSON types
are all different" rule.

> - The 'name' in SchemaInfoObjectMember could be the key name of the
>   variant in the alternate definition. However, this name isn't used
>   anywhere in the QMP protocol, so maybe it is better kept internal.
>   'name' isn't optional, though.

This is actually a more convincing argument for having a separate
SchemaInfoAlternateMember.

> - The 'default' of SchemaInfoObjectMember must be omitted.

Likewise.

> Do I understand the intention reasonably right? If so, maybe it would be
> better to use a separate type for alternate variants.

I'll certainly consider that for the next iteration.

Thanks!

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On 04/02/2015 11:29 AM, Markus Armbruster wrote:

> 
> * Implicit type definitions are made explicit, and given
>   auto-generated names.  These names start with ':' so they don't
>   clash with the user's names.
> 
>   Example: a simple union implicitly defines an enumeration type for
>   its discriminator.

Given that I just tripped over a bug in my series where I failed to
consider that '*name' and 'name' should be considered the same, I'm
wondering if we should first update the parse engine to flatten
shorthand into a canonical form (that is, get to a point where we have a
list of all names and their C counterparts), rather than having to tweak
lots of places in the backends to repeatedly make the same translations
over and over again (stripping off leading '*', converting qapi
'default' into C 'q_default', converting qapi 'a-b' into C 'a_b', etc.).

I bet some of the backend generator gets simpler if the front end reuses
your work to get into a canonical form on initial parse.


> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,430 @@

> +
> +def make_implicit_enum_type(name, role, values):
> +    name = ':enum-%s-%s' % (name, role)

Evidence of my python newbie-ness:

Here, you are using string formatting (as in 'pattern' % arguments), in
other patches, I've seen you use concatenation (would look like ':enum-'
+ name + '-' + role).  Is there any rhyme or reason why one form should
be considered over the other?

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/02/2015 11:29 AM, Markus Armbruster wrote:
>
>> 
>> * Implicit type definitions are made explicit, and given
>>   auto-generated names.  These names start with ':' so they don't
>>   clash with the user's names.
>> 
>>   Example: a simple union implicitly defines an enumeration type for
>>   its discriminator.
>
> Given that I just tripped over a bug in my series where I failed to
> consider that '*name' and 'name' should be considered the same, I'm
> wondering if we should first update the parse engine to flatten
> shorthand into a canonical form (that is, get to a point where we have a
> list of all names and their C counterparts), rather than having to tweak
> lots of places in the backends to repeatedly make the same translations
> over and over again (stripping off leading '*', converting qapi
> 'default' into C 'q_default', converting qapi 'a-b' into C 'a_b', etc.).
>
> I bet some of the backend generator gets simpler if the front end reuses
> your work to get into a canonical form on initial parse.

Agree:

>> The schema generation proper is pretty trivial.  Much of the
>> qapi-introspect.py's code actually does something else: convert the
>> output of parse_schema() into a more usable data structure, lowering
>> away uninteresting stuff.  This should really be done in qapi.py, so
>> the other generators can use it, too.

However, we can't simply rebase your series onto mine, because mine is
already based on yours.  I really, really want to base on your tests,
simplifications and bug fixes.

Rearranging everything now doesn't sound appealing to me.

Let's get our combined work in, except for this PATCH 19, which isn't
ready, yet.

>> +++ b/scripts/qapi-introspect.py
>> @@ -0,0 +1,430 @@
>
>> +
>> +def make_implicit_enum_type(name, role, values):
>> +    name = ':enum-%s-%s' % (name, role)
>
> Evidence of my python newbie-ness:
>
> Here, you are using string formatting (as in 'pattern' % arguments), in
> other patches, I've seen you use concatenation (would look like ':enum-'
> + name + '-' + role).  Is there any rhyme or reason why one form should
> be considered over the other?

I'm no Pythonista, either.  I use whatever feels more readable.

Re: [Qemu-devel] [PATCH RFC 00/19] qapi: QMP introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Dring up your tea, here comes the introspection series.
> 
> * PATCH 01-15: As usual when I touch the qapi code generators, I need
>   double-digit patches just to get the mess cleaned up enough to admit
>   change :)
> 
> * PATCH 16-18: Fix the JSON parser to recognize null.  With an axe.

I did a similar thing, but only in one patch (I'm posting it now that
you've posted yours, even though I wrote it as part of my larger pending
v6 posting that I'm still working on)


-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 00/19] qapi: QMP introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On 04/02/2015 11:28 AM, Markus Armbruster wrote:
> Dring up your tea, here comes the introspection series.
> 
> * PATCH 01-15: As usual when I touch the qapi code generators, I need
>   double-digit patches just to get the mess cleaned up enough to admit
>   change :)
> 
> * PATCH 16-18: Fix the JSON parser to recognize null.  With an axe.
> 
> * PATCH 19: Introspection.  This one's completely unpolished and only
>   lightly tested.  There's no documentation apart from the commit
>   message.  I hope this is good enough to let us discuss the general
>   approach and the introspection schema.
> 
> Prior work: Amos's "[PATCH v4 0/5] QMP full introspection" from Jan 14
> https://lists.nongnu.org/archive/html/qemu-devel/2014-01/msg03013.html
> 
> Please note I'll be away from qemu-devel for three weeks.
> 
> Markus Armbruster (19):
>   tests: Add missing dependencies on $(qapi-py)
>   qapi: Fix C identifiers generated for names containing '.'

Looks like this series was based on my v5 nested struct removal (because
in v6, I removed the 'import string' that I had added in v5, and your
patch 2 complained about unknown name 'string' as a result).  Fairly
obvious resolution for anyone else testing the series.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]