* [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1)
@ 2018-06-27 16:35 Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions Marc-André Lureau
` (14 more replies)
0 siblings, 15 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Hi,
In order to clean-up some hacks in qapi (having to unregister commands
at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef condition"
(see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).
However, we decided to drop that patch from the series and solve the
problem later. The main issues were:
- the syntax was awkward to the JSON schema and documentation
- the evaluation of the condition was done in the qapi scripts, with
very limited capability
- each target/config would need different generated files.
Instead, it could defer the #if evaluation to the C-preprocessor.
With this series, top-level qapi JSON entity can take 'if' keys:
{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
'if': 'defined(TEST_IF_STRUCT)' }
A benefit from having conditional schema is that introspection will
reflect more accurately the capability of the server. Another benefit
is that it may help to remove some dead code when disabling a
functionality.
Starting from patch "qapi: add conditions to VNC type/commands/events
on the schema", the series demonstrates adding conditions.
There are a lot more things one can make conditional in the QAPI
schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
however I am still evaluating the implication of such changes both
externally and internally, for those interested, I can share my wip
branch.
Comments welcome,
v6:
- subset of v5, which only adds top-level conditions (03-14,31 and
applicable parts of 35-37)
- removed QAPI_REGENERATE test leftover
- comment ifcontext() usage
- simplify .start_if()/.end_if() with a single ._start_if member
- removed useless 'saved' variable
- renamed QAPIGenCSnippet -> QAPIGenCCode
- fix and add a test when a conditional command returns a common type
- use Markus version of "qapi-types: add #if conditions to types & visitors"
- pylint fixes
- commit message fixes/changes
- rebased
v5:
- adapt to the new 'modular' / single generator design, making
splitting schema a bit more complicated than before, when it was
only at build-sys level. See "RFC: qapi: learn to split schema by
'top-unit'" for the new apporach at splitting common and target
parts.
- replace the #if-wrapper decorator with a @contextmanager (if necessary,
this could be replaced by explicit begin/end calls instead now)
- better split of target events, introducing seperate limiter/emitter
- add a 'RFC: make RTC_CHANGE per-target' to show how conditional
could be used and exercice the 'per-unit' event limiter
- +painful rebase
- add some r-b tags
v4:
- added "qlit: use QType instead of int" patch
- use a "default:" case in qobject_from_qlit()
- added a test for invalid 'if': ['']
- listify ifcond in constructors
- added "qapi: leave the ifcond attribute undefined until check()"
patch (could be squashed)
- use a negative lookahead in "qapi: mcgen() shouldn't indent # lines"
- fix extra empty line in "qapi-introspect: add preprocessor
conditions to generated QLit"
- added desugar/normalization passes, such as NAME -> { 'name': NAME }
- splitted "qapi: change enum visitor to take QAPISchemaMember"
- prettify test-qapi.py members output
- added missing #if wrapping for enum members in generated introspection
- clarified some error messages
- added "qapi: rename allow_dict to allow_implicit" patch for clarity
- added a seperate patch for "qapi: add a dictionnary form" for enum
names
- added a seperate patch for "qapi: add a dictionnary form" for struct
members
- squashed the patches adding #if to generated code (some types
generate both enum and struct members for example, so a step-by-step
is unnecessarily complicated to deal with)
- squashed some tests with related patch
- added "qapi: add an error in case a discriminator is conditionnal"
- add back VNC-specific crypto 'des-rfb' when VNC is disabled
- change the way unit filtering is done (so no -u means no filtering)
- made target.json the top-level documentation schema
- removed some blank lines in generated code output
- improve documentation, add various tests
- commit message improvements
- misc pycodestyle/pep8 fixes
- add r-b tags
v3:
- rebased (qlit is now merged upstream)
- solve the per-target #ifdef problem by using a target.json
and new qapi generated target files
- update some commit messages based on Markus review
- more schema error reporting
- move the ifcond argument closer to info/doc
- use mcgen() in gen_if()/gen_endif()
- simplify "modify to_qlit() to take an optional suffix"
- fix generated qlit indentation
- fix temporary build break by merging #if types & visitors patch
- fix some redundant condtionals generation
- change enum visitor to take QAPISchemaMember
- reject unknown dictionnary keys in { .., 'if': ..}
- split qapi test visitor print() with trailing ',' trick
v2: after Markus review
- "qboject: add literal qobject type", splitted & many updates
- "qapi: generate a literal qobject for introspection", fixed leak
- "visitor: pass size of strings array to enum visitor", rewrote as a
series of patches to change char*[] to struct { int n, char *[] }.
- split the "if" patches in parsing, tests, generation
- added a docs/devel/qapi-code-gen.txt patch
- added a patch to remove enum value assignment, to avoid "holes"
with #if values
- update #if VNC patch, compile out "info vnc", compile out des-rfb
- dropped "qobject: replace dump_qobject() by qobject_to_string()" patch
- dropped qapi.mk patch
- other smaller changes
Marc-André Lureau (14):
qapi: add 'if' to top-level expressions
tests
qapi: pass 'if' condition into QAPISchemaEntity objects
qapi: leave the ifcond attribute undefined until check()
qapi: add 'ifcond' to visitor methods
qapi: mcgen() shouldn't indent # lines
qapi: add #if/#endif helpers
qapi-introspect: modify to_qlit() to append ',' on level > 0
qapi-introspect: add preprocessor conditions to generated QLit
qapi/commands: add #if conditions to commands
qapi/events: add #if conditions to events
qapi: add 'If:' section to generated documentation
qapi: add conditions to VNC type/commands/events on the schema
qapi: add conditions to SPICE type/commands/events on the schema
Markus Armbruster (1):
qapi-types: add #if conditions to types & visitors
qapi/char.json | 8 +-
qapi/ui.json | 75 +++---
scripts/qapi/commands.py | 23 +-
scripts/qapi/common.py | 283 ++++++++++++++++++-----
scripts/qapi/doc.py | 32 +--
scripts/qapi/events.py | 8 +-
scripts/qapi/introspect.py | 47 ++--
scripts/qapi/types.py | 58 +++--
scripts/qapi/visit.py | 41 ++--
ui/vnc.h | 2 +
hmp.c | 9 +-
monitor.c | 3 -
qmp.c | 46 +---
tests/test-qmp-cmds.c | 13 ++
docs/devel/qapi-code-gen.txt | 22 ++
hmp-commands-info.hx | 2 +
tests/Makefile.include | 4 +
tests/qapi-schema/bad-if-empty-list.err | 1 +
tests/qapi-schema/bad-if-empty-list.exit | 1 +
tests/qapi-schema/bad-if-empty-list.json | 3 +
tests/qapi-schema/bad-if-empty-list.out | 0
tests/qapi-schema/bad-if-empty.err | 1 +
tests/qapi-schema/bad-if-empty.exit | 1 +
tests/qapi-schema/bad-if-empty.json | 3 +
tests/qapi-schema/bad-if-empty.out | 0
tests/qapi-schema/bad-if-list.err | 1 +
tests/qapi-schema/bad-if-list.exit | 1 +
tests/qapi-schema/bad-if-list.json | 3 +
tests/qapi-schema/bad-if-list.out | 0
tests/qapi-schema/bad-if.err | 1 +
tests/qapi-schema/bad-if.exit | 1 +
tests/qapi-schema/bad-if.json | 3 +
tests/qapi-schema/bad-if.out | 0
tests/qapi-schema/doc-good.json | 2 +-
tests/qapi-schema/doc-good.out | 1 +
tests/qapi-schema/doc-good.texi | 2 +
tests/qapi-schema/qapi-schema-test.json | 26 +++
tests/qapi-schema/qapi-schema-test.out | 35 +++
tests/qapi-schema/test-qapi.py | 19 +-
39 files changed, 554 insertions(+), 227 deletions(-)
mode change 100644 => 100755 scripts/qapi/doc.py
create mode 100644 tests/qapi-schema/bad-if-empty-list.err
create mode 100644 tests/qapi-schema/bad-if-empty-list.exit
create mode 100644 tests/qapi-schema/bad-if-empty-list.json
create mode 100644 tests/qapi-schema/bad-if-empty-list.out
create mode 100644 tests/qapi-schema/bad-if-empty.err
create mode 100644 tests/qapi-schema/bad-if-empty.exit
create mode 100644 tests/qapi-schema/bad-if-empty.json
create mode 100644 tests/qapi-schema/bad-if-empty.out
create mode 100644 tests/qapi-schema/bad-if-list.err
create mode 100644 tests/qapi-schema/bad-if-list.exit
create mode 100644 tests/qapi-schema/bad-if-list.json
create mode 100644 tests/qapi-schema/bad-if-list.out
create mode 100644 tests/qapi-schema/bad-if.err
create mode 100644 tests/qapi-schema/bad-if.exit
create mode 100644 tests/qapi-schema/bad-if.json
create mode 100644 tests/qapi-schema/bad-if.out
--
2.18.0.rc1
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-28 14:57 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 02/15] tests Marc-André Lureau
` (13 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Accept 'if' key in top-level elements, accepted as string or list of
string type. The following patches will modify the test visitor to
check the value is correctly saved, and generate #if/#endif code (as a
single #if/endif line or a series for a list).
Example of 'if' key:
{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
'if': 'defined(TEST_IF_STRUCT)' }
The generated code is for now *unconditional*. Later patches generate
the conditionals.
A following patch for qapi-code-gen.txt will provide more complete
documentation for 'if' usage.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/common.py | 35 ++++++++++++++++++++----
tests/test-qmp-cmds.c | 6 ++++
docs/devel/qapi-code-gen.txt | 22 +++++++++++++++
tests/Makefile.include | 4 +++
tests/qapi-schema/bad-if-empty-list.err | 1 +
tests/qapi-schema/bad-if-empty-list.exit | 1 +
tests/qapi-schema/bad-if-empty-list.json | 3 ++
tests/qapi-schema/bad-if-empty-list.out | 0
tests/qapi-schema/bad-if-empty.err | 1 +
tests/qapi-schema/bad-if-empty.exit | 1 +
tests/qapi-schema/bad-if-empty.json | 3 ++
tests/qapi-schema/bad-if-empty.out | 0
tests/qapi-schema/bad-if-list.err | 1 +
tests/qapi-schema/bad-if-list.exit | 1 +
tests/qapi-schema/bad-if-list.json | 3 ++
tests/qapi-schema/bad-if-list.out | 0
tests/qapi-schema/bad-if.err | 1 +
tests/qapi-schema/bad-if.exit | 1 +
tests/qapi-schema/bad-if.json | 3 ++
tests/qapi-schema/bad-if.out | 0
tests/qapi-schema/qapi-schema-test.json | 20 ++++++++++++++
tests/qapi-schema/qapi-schema-test.out | 22 +++++++++++++++
22 files changed, 123 insertions(+), 6 deletions(-)
create mode 100644 tests/qapi-schema/bad-if-empty-list.err
create mode 100644 tests/qapi-schema/bad-if-empty-list.exit
create mode 100644 tests/qapi-schema/bad-if-empty-list.json
create mode 100644 tests/qapi-schema/bad-if-empty-list.out
create mode 100644 tests/qapi-schema/bad-if-empty.err
create mode 100644 tests/qapi-schema/bad-if-empty.exit
create mode 100644 tests/qapi-schema/bad-if-empty.json
create mode 100644 tests/qapi-schema/bad-if-empty.out
create mode 100644 tests/qapi-schema/bad-if-list.err
create mode 100644 tests/qapi-schema/bad-if-list.exit
create mode 100644 tests/qapi-schema/bad-if-list.json
create mode 100644 tests/qapi-schema/bad-if-list.out
create mode 100644 tests/qapi-schema/bad-if.err
create mode 100644 tests/qapi-schema/bad-if.exit
create mode 100644 tests/qapi-schema/bad-if.json
create mode 100644 tests/qapi-schema/bad-if.out
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 8b6708dbf1..991045a478 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -638,6 +638,27 @@ def add_name(name, info, meta, implicit=False):
all_names[name] = meta
+def check_if(expr, info):
+
+ def check_if_str(ifcond, info):
+ if not isinstance(ifcond, str):
+ raise QAPISemError(
+ info, "'if' condition must be a string or a list of strings")
+ if ifcond == '':
+ raise QAPISemError(info, "'if' condition '' makes no sense")
+
+ ifcond = expr.get('if')
+ if ifcond is None:
+ return
+ if isinstance(ifcond, list):
+ if ifcond == []:
+ raise QAPISemError(info, "'if' condition [] is useless")
+ for elt in ifcond:
+ check_if_str(elt, info)
+ else:
+ check_if_str(ifcond, info)
+
+
def check_type(info, source, value, allow_array=False,
allow_dict=False, allow_optional=False,
allow_metas=[]):
@@ -871,6 +892,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
raise QAPISemError(info,
"'%s' of %s '%s' should only use true value"
% (key, meta, name))
+ if key == 'if':
+ check_if(expr, info)
for key in required:
if key not in expr:
raise QAPISemError(info, "Key '%s' is missing from %s '%s'"
@@ -899,28 +922,28 @@ def check_exprs(exprs):
if 'enum' in expr:
meta = 'enum'
- check_keys(expr_elem, 'enum', ['data'], ['prefix'])
+ check_keys(expr_elem, 'enum', ['data'], ['if', 'prefix'])
enum_types[expr[meta]] = expr
elif 'union' in expr:
meta = 'union'
check_keys(expr_elem, 'union', ['data'],
- ['base', 'discriminator'])
+ ['base', 'discriminator', 'if'])
union_types[expr[meta]] = expr
elif 'alternate' in expr:
meta = 'alternate'
- check_keys(expr_elem, 'alternate', ['data'])
+ check_keys(expr_elem, 'alternate', ['data'], ['if'])
elif 'struct' in expr:
meta = 'struct'
- check_keys(expr_elem, 'struct', ['data'], ['base'])
+ check_keys(expr_elem, 'struct', ['data'], ['base', 'if'])
struct_types[expr[meta]] = expr
elif 'command' in expr:
meta = 'command'
check_keys(expr_elem, 'command', [],
['data', 'returns', 'gen', 'success-response',
- 'boxed', 'allow-oob', 'allow-preconfig'])
+ 'boxed', 'allow-oob', 'allow-preconfig', 'if'])
elif 'event' in expr:
meta = 'event'
- check_keys(expr_elem, 'event', [], ['data', 'boxed'])
+ check_keys(expr_elem, 'event', [], ['data', 'boxed', 'if'])
else:
raise QAPISemError(expr_elem['info'],
"Expression is missing metatype")
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 491b0c4a44..eaf30b7a0f 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -12,6 +12,12 @@
static QmpCommandList qmp_commands;
+/* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
+void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
+{
+}
+/* #endif */
+
void qmp_user_def_cmd(Error **errp)
{
}
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 88a70e4d45..7af60b48f3 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -739,6 +739,28 @@ Example: Red Hat, Inc. controls redhat.com, and may therefore add a
downstream command __com.redhat_drive-mirror.
+=== Configuring the schema ===
+
+Top-level QAPI expressions. The value must be a string or a list of
+string. The corresponding generated code will then guard the inclusion
+of that member in the larger struct or function with #if IFCOND
+(or several #if lines for a list), where IFCOND is the value of the
+'if' key.
+
+'struct', 'enum', 'union', 'alternate', 'command' and 'event'
+top-level QAPI expressions can take an 'if' keyword like:
+
+{ 'struct': 'IfStruct', 'data': { 'foo': 'int' },
+ 'if': 'defined(IFCOND)' }
+
+Please note that you are responsible to ensure that the C code will
+compile with an arbitrary combination of conditions, since the
+generators are unable to check it at this point.
+
+The presence of 'if' keys in the schema is reflected through to the
+introspection output depending on the build configuration.
+
+
== Client JSON Protocol introspection ==
Clients of a Client JSON Protocol commonly need to figure out what
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e8bb2d8f66..9faefd7cd2 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -442,6 +442,10 @@ qapi-schema += args-unknown.json
qapi-schema += bad-base.json
qapi-schema += bad-data.json
qapi-schema += bad-ident.json
+qapi-schema += bad-if.json
+qapi-schema += bad-if-empty.json
+qapi-schema += bad-if-empty-list.json
+qapi-schema += bad-if-list.json
qapi-schema += bad-type-bool.json
qapi-schema += bad-type-dict.json
qapi-schema += bad-type-int.json
diff --git a/tests/qapi-schema/bad-if-empty-list.err b/tests/qapi-schema/bad-if-empty-list.err
new file mode 100644
index 0000000000..75fe6497bc
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-if-empty-list.json:2: 'if' condition [] is useless
diff --git a/tests/qapi-schema/bad-if-empty-list.exit b/tests/qapi-schema/bad-if-empty-list.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty-list.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/bad-if-empty-list.json b/tests/qapi-schema/bad-if-empty-list.json
new file mode 100644
index 0000000000..94f2eb8670
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty-list.json
@@ -0,0 +1,3 @@
+# check empty 'if' list
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+ 'if': [] }
diff --git a/tests/qapi-schema/bad-if-empty-list.out b/tests/qapi-schema/bad-if-empty-list.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/bad-if-empty.err b/tests/qapi-schema/bad-if-empty.err
new file mode 100644
index 0000000000..358bdc3e51
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-if-empty.json:2: 'if' condition '' makes no sense
diff --git a/tests/qapi-schema/bad-if-empty.exit b/tests/qapi-schema/bad-if-empty.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/bad-if-empty.json b/tests/qapi-schema/bad-if-empty.json
new file mode 100644
index 0000000000..fe1dd4eca6
--- /dev/null
+++ b/tests/qapi-schema/bad-if-empty.json
@@ -0,0 +1,3 @@
+# check empty 'if'
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+ 'if': '' }
diff --git a/tests/qapi-schema/bad-if-empty.out b/tests/qapi-schema/bad-if-empty.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/bad-if-list.err b/tests/qapi-schema/bad-if-list.err
new file mode 100644
index 0000000000..0af6316f78
--- /dev/null
+++ b/tests/qapi-schema/bad-if-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-if-list.json:2: 'if' condition '' makes no sense
diff --git a/tests/qapi-schema/bad-if-list.exit b/tests/qapi-schema/bad-if-list.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/bad-if-list.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/bad-if-list.json b/tests/qapi-schema/bad-if-list.json
new file mode 100644
index 0000000000..49ced9b9ca
--- /dev/null
+++ b/tests/qapi-schema/bad-if-list.json
@@ -0,0 +1,3 @@
+# check invalid 'if' content
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+ 'if': ['foo', ''] }
diff --git a/tests/qapi-schema/bad-if-list.out b/tests/qapi-schema/bad-if-list.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
new file mode 100644
index 0000000000..c2e3f5f44c
--- /dev/null
+++ b/tests/qapi-schema/bad-if.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-if.json:2: 'if' condition must be a string or a list of strings
diff --git a/tests/qapi-schema/bad-if.exit b/tests/qapi-schema/bad-if.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/bad-if.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/bad-if.json b/tests/qapi-schema/bad-if.json
new file mode 100644
index 0000000000..3edd1a0bf2
--- /dev/null
+++ b/tests/qapi-schema/bad-if.json
@@ -0,0 +1,3 @@
+# check invalid 'if' type
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+ 'if': { 'value': 'defined(TEST_IF_STRUCT)' } }
diff --git a/tests/qapi-schema/bad-if.out b/tests/qapi-schema/bad-if.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 7b59817f04..ec0fea804f 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -193,3 +193,23 @@
'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' },
'returns': '__org.qemu_x-Union1' }
+
+# test 'if' condition handling
+
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+ 'if': 'defined(TEST_IF_STRUCT)' }
+
+{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],
+ 'if': 'defined(TEST_IF_ENUM)' }
+
+{ 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
+ 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
+
+{ 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' },
+ 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
+
+{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
+ 'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
+
+{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
+ 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 0dbcdafa3c..34b6d546f3 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -233,3 +233,25 @@ object q_obj___org.qemu_x-command-arg
member d: __org.qemu_x-Alt optional=False
command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
gen=True success_response=True boxed=False oob=False preconfig=False
+object TestIfStruct
+ member foo: int optional=False
+enum TestIfEnum ['foo', 'bar']
+object q_obj_TestStruct-wrapper
+ member data: TestStruct optional=False
+enum TestIfUnionKind ['foo']
+object TestIfUnion
+ member type: TestIfUnionKind optional=False
+ tag type
+ case foo: q_obj_TestStruct-wrapper
+alternate TestIfAlternate
+ tag type
+ case foo: int
+ case bar: TestStruct
+object q_obj_TestIfCmd-arg
+ member foo: TestIfStruct optional=False
+command TestIfCmd q_obj_TestIfCmd-arg -> None
+ gen=True success_response=True boxed=False oob=False preconfig=False
+object q_obj_TestIfEvent-arg
+ member foo: TestIfStruct optional=False
+event TestIfEvent q_obj_TestIfEvent-arg
+ boxed=False
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 02/15] tests
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-27 16:39 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 03/15] qapi: pass 'if' condition into QAPISchemaEntity objects Marc-André Lureau
` (12 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
---
tests/test-qmp-cmds.c | 8 +++++++-
tests/qapi-schema/qapi-schema-test.json | 6 ++++++
tests/qapi-schema/qapi-schema-test.out | 6 +++++-
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index eaf30b7a0f..840530b84c 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -13,11 +13,17 @@
static QmpCommandList qmp_commands;
/* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
-void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
+UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
{
+ return NULL;
}
/* #endif */
+UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
+{
+ return NULL;
+}
+
void qmp_user_def_cmd(Error **errp)
{
}
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ec0fea804f..16209b57b3 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -56,6 +56,9 @@
'data': { 'string0': 'str',
'dict1': 'UserDefTwoDict' } }
+{ 'struct': 'UserDefThree',
+ 'data': { 'string0': 'str' } }
+
# dummy struct to force generation of array types not otherwise mentioned
{ 'struct': 'ForceArrays',
'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
@@ -209,7 +212,10 @@
'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
+ 'returns': 'UserDefThree',
'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
+{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
+
{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 34b6d546f3..ed25e5b60c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -36,6 +36,8 @@ object UserDefTwoDict
object UserDefTwo
member string0: str optional=False
member dict1: UserDefTwoDict optional=False
+object UserDefThree
+ member string0: str optional=False
object ForceArrays
member unused1: UserDefOneList optional=False
member unused2: UserDefTwoList optional=False
@@ -249,7 +251,9 @@ alternate TestIfAlternate
case bar: TestStruct
object q_obj_TestIfCmd-arg
member foo: TestIfStruct optional=False
-command TestIfCmd q_obj_TestIfCmd-arg -> None
+command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
+ gen=True success_response=True boxed=False oob=False preconfig=False
+command TestCmdReturnDefThree None -> UserDefThree
gen=True success_response=True boxed=False oob=False preconfig=False
object q_obj_TestIfEvent-arg
member foo: TestIfStruct optional=False
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 03/15] qapi: pass 'if' condition into QAPISchemaEntity objects
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 02/15] tests Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check() Marc-André Lureau
` (11 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Built-in objects remain unconditional. Explicitly defined objects use
the condition specified in the schema. Implicitly defined objects
inherit their condition from their users. For most of them, there is
exactly one user, so the condition to use is obvious. The exception
is wrapped types generated for simple union variants, which can be
shared by any number of simple unions. The tight condition would be
the disjunction of the conditions of these simple unions. For now,
use the wrapped type's condition instead. Much simpler and good
enough for now.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/common.py | 97 ++++++++++++++++++++++++++++--------------
1 file changed, 65 insertions(+), 32 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 991045a478..4f4014b387 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1001,8 +1001,16 @@ def check_exprs(exprs):
# Schema compiler frontend
#
+def listify_cond(ifcond):
+ if not ifcond:
+ return []
+ if not isinstance(ifcond, list):
+ return [ifcond]
+ return ifcond
+
+
class QAPISchemaEntity(object):
- def __init__(self, name, info, doc):
+ def __init__(self, name, info, doc, ifcond=None):
assert name is None or isinstance(name, str)
self.name = name
self.module = None
@@ -1013,6 +1021,7 @@ class QAPISchemaEntity(object):
# such place).
self.info = info
self.doc = doc
+ self.ifcond = listify_cond(ifcond)
def c_name(self):
return c_name(self.name)
@@ -1145,8 +1154,8 @@ class QAPISchemaBuiltinType(QAPISchemaType):
class QAPISchemaEnumType(QAPISchemaType):
- def __init__(self, name, info, doc, values, prefix):
- QAPISchemaType.__init__(self, name, info, doc)
+ def __init__(self, name, info, doc, ifcond, values, prefix):
+ QAPISchemaType.__init__(self, name, info, doc, ifcond)
for v in values:
assert isinstance(v, QAPISchemaMember)
v.set_owner(name)
@@ -1181,7 +1190,7 @@ class QAPISchemaEnumType(QAPISchemaType):
class QAPISchemaArrayType(QAPISchemaType):
def __init__(self, name, info, element_type):
- QAPISchemaType.__init__(self, name, info, None)
+ QAPISchemaType.__init__(self, name, info, None, None)
assert isinstance(element_type, str)
self._element_type_name = element_type
self.element_type = None
@@ -1189,6 +1198,7 @@ class QAPISchemaArrayType(QAPISchemaType):
def check(self, schema):
self.element_type = schema.lookup_type(self._element_type_name)
assert self.element_type
+ self.ifcond = self.element_type.ifcond
def is_implicit(self):
return True
@@ -1210,11 +1220,12 @@ class QAPISchemaArrayType(QAPISchemaType):
class QAPISchemaObjectType(QAPISchemaType):
- def __init__(self, name, info, doc, base, local_members, variants):
+ def __init__(self, name, info, doc, ifcond,
+ base, local_members, variants):
# struct has local_members, optional base, and no variants
# flat union has base, variants, and no local_members
# simple union has local_members, variants, and no base
- QAPISchemaType.__init__(self, name, info, doc)
+ QAPISchemaType.__init__(self, name, info, doc, ifcond)
assert base is None or isinstance(base, str)
for m in local_members:
assert isinstance(m, QAPISchemaObjectTypeMember)
@@ -1410,8 +1421,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
class QAPISchemaAlternateType(QAPISchemaType):
- def __init__(self, name, info, doc, variants):
- QAPISchemaType.__init__(self, name, info, doc)
+ def __init__(self, name, info, doc, ifcond, variants):
+ QAPISchemaType.__init__(self, name, info, doc, ifcond)
assert isinstance(variants, QAPISchemaObjectTypeVariants)
assert variants.tag_member
variants.set_owner(name)
@@ -1447,9 +1458,9 @@ class QAPISchemaAlternateType(QAPISchemaType):
class QAPISchemaCommand(QAPISchemaEntity):
- def __init__(self, name, info, doc, arg_type, ret_type,
+ def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
gen, success_response, boxed, allow_oob, allow_preconfig):
- QAPISchemaEntity.__init__(self, name, info, doc)
+ QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
assert not arg_type or isinstance(arg_type, str)
assert not ret_type or isinstance(ret_type, str)
self._arg_type_name = arg_type
@@ -1490,8 +1501,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
class QAPISchemaEvent(QAPISchemaEntity):
- def __init__(self, name, info, doc, arg_type, boxed):
- QAPISchemaEntity.__init__(self, name, info, doc)
+ def __init__(self, name, info, doc, ifcond, arg_type, boxed):
+ QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
assert not arg_type or isinstance(arg_type, str)
self._arg_type_name = arg_type
self.arg_type = None
@@ -1590,22 +1601,22 @@ class QAPISchema(object):
('null', 'null', 'QNull' + pointer_suffix)]:
self._def_builtin_type(*t)
self.the_empty_object_type = QAPISchemaObjectType(
- 'q_empty', None, None, None, [], None)
+ 'q_empty', None, None, None, None, [], None)
self._def_entity(self.the_empty_object_type)
qtype_values = self._make_enum_members(['none', 'qnull', 'qnum',
'qstring', 'qdict', 'qlist',
'qbool'])
- self._def_entity(QAPISchemaEnumType('QType', None, None,
+ self._def_entity(QAPISchemaEnumType('QType', None, None, None,
qtype_values, 'QTYPE'))
def _make_enum_members(self, values):
return [QAPISchemaMember(v) for v in values]
- def _make_implicit_enum_type(self, name, info, values):
+ def _make_implicit_enum_type(self, name, info, ifcond, values):
# See also QAPISchemaObjectTypeMember._pretty_owner()
name = name + 'Kind' # Use namespace reserved by add_name()
self._def_entity(QAPISchemaEnumType(
- name, info, None, self._make_enum_members(values), None))
+ name, info, None, ifcond, self._make_enum_members(values), None))
return name
def _make_array_type(self, element_type, info):
@@ -1614,22 +1625,37 @@ class QAPISchema(object):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
- def _make_implicit_object_type(self, name, info, doc, role, members):
+ def _make_implicit_object_type(self, name, info, doc, ifcond,
+ role, members):
if not members:
return None
# See also QAPISchemaObjectTypeMember._pretty_owner()
name = 'q_obj_%s-%s' % (name, role)
- if not self.lookup_entity(name, QAPISchemaObjectType):
- self._def_entity(QAPISchemaObjectType(name, info, doc, None,
- members, None))
+ typ = self.lookup_entity(name, QAPISchemaObjectType)
+ if typ:
+ # The implicit object type has multiple users. This can
+ # happen only for simple unions' implicit wrapper types.
+ # Its ifcond should be the disjunction of its user's
+ # ifconds. Not implemented. Instead, we always pass the
+ # wrapped type's ifcond, which is trivially the same for all
+ # users. It's also necessary for the wrapper to compile.
+ # But it's not tight: the disjunction need not imply it. We
+ # may end up compiling useless wrapper types.
+ # TODO kill simple unions or implement the disjunction
+ assert ifcond == typ.ifcond
+ else:
+ self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
+ None, members, None))
return name
def _def_enum_type(self, expr, info, doc):
name = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
+ ifcond = expr.get('if')
self._def_entity(QAPISchemaEnumType(
- name, info, doc, self._make_enum_members(data), prefix))
+ name, info, doc, ifcond,
+ self._make_enum_members(data), prefix))
def _make_member(self, name, typ, info):
optional = False
@@ -1649,7 +1675,8 @@ class QAPISchema(object):
name = expr['struct']
base = expr.get('base')
data = expr['data']
- self._def_entity(QAPISchemaObjectType(name, info, doc, base,
+ ifcond = expr.get('if')
+ self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
self._make_members(data, info),
None))
@@ -1661,18 +1688,21 @@ class QAPISchema(object):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
typ = self._make_implicit_object_type(
- typ, info, None, 'wrapper', [self._make_member('data', typ, info)])
+ typ, info, None, self.lookup_type(typ).ifcond,
+ 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
def _def_union_type(self, expr, info, doc):
name = expr['union']
data = expr['data']
base = expr.get('base')
+ ifcond = expr.get('if')
tag_name = expr.get('discriminator')
tag_member = None
if isinstance(base, dict):
- base = (self._make_implicit_object_type(
- name, info, doc, 'base', self._make_members(base, info)))
+ base = self._make_implicit_object_type(
+ name, info, doc, ifcond,
+ 'base', self._make_members(base, info))
if tag_name:
variants = [self._make_variant(key, value)
for (key, value) in data.items()]
@@ -1680,12 +1710,12 @@ class QAPISchema(object):
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.items()]
- typ = self._make_implicit_enum_type(name, info,
+ typ = self._make_implicit_enum_type(name, info, ifcond,
[v.name for v in variants])
tag_member = QAPISchemaObjectTypeMember('type', typ, False)
members = [tag_member]
self._def_entity(
- QAPISchemaObjectType(name, info, doc, base, members,
+ QAPISchemaObjectType(name, info, doc, ifcond, base, members,
QAPISchemaObjectTypeVariants(tag_name,
tag_member,
variants)))
@@ -1693,11 +1723,12 @@ class QAPISchema(object):
def _def_alternate_type(self, expr, info, doc):
name = expr['alternate']
data = expr['data']
+ ifcond = expr.get('if')
variants = [self._make_variant(key, value)
for (key, value) in data.items()]
tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
self._def_entity(
- QAPISchemaAlternateType(name, info, doc,
+ QAPISchemaAlternateType(name, info, doc, ifcond,
QAPISchemaObjectTypeVariants(None,
tag_member,
variants)))
@@ -1711,13 +1742,14 @@ class QAPISchema(object):
boxed = expr.get('boxed', False)
allow_oob = expr.get('allow-oob', False)
allow_preconfig = expr.get('allow-preconfig', False)
+ ifcond = expr.get('if')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, info, doc, 'arg', self._make_members(data, info))
+ name, info, doc, ifcond, 'arg', self._make_members(data, info))
if isinstance(rets, list):
assert len(rets) == 1
rets = self._make_array_type(rets[0], info)
- self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
+ self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
gen, success_response,
boxed, allow_oob, allow_preconfig))
@@ -1725,10 +1757,11 @@ class QAPISchema(object):
name = expr['event']
data = expr.get('data')
boxed = expr.get('boxed', False)
+ ifcond = expr.get('if')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, info, doc, 'arg', self._make_members(data, info))
- self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed))
+ name, info, doc, ifcond, 'arg', self._make_members(data, info))
+ self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, data, boxed))
def _def_exprs(self, exprs):
for expr_elem in exprs:
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check()
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (2 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 03/15] qapi: pass 'if' condition into QAPISchemaEntity objects Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-28 17:45 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods Marc-André Lureau
` (10 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
We commonly initialize attributes to None in .init(), then set their
real value in .check(). Accessing the attribute before .check()
yields None. If we're lucky, the code that accesses the attribute
prematurely chokes on None.
It won't for .ifcond, because None is a legitimate value.
Leave the ifcond attribute undefined until check().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/common.py | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 4f4014b387..46e33e23e4 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1021,13 +1021,19 @@ class QAPISchemaEntity(object):
# such place).
self.info = info
self.doc = doc
- self.ifcond = listify_cond(ifcond)
+ self._ifcond = ifcond # self.ifcond is set only after .check()
def c_name(self):
return c_name(self.name)
def check(self, schema):
- pass
+ if isinstance(self._ifcond, QAPISchemaType):
+ # inherit the condition from a type
+ typ = self._ifcond
+ typ.check(schema)
+ self.ifcond = typ.ifcond
+ else:
+ self.ifcond = listify_cond(self._ifcond)
def is_implicit(self):
return not self.info
@@ -1164,6 +1170,7 @@ class QAPISchemaEnumType(QAPISchemaType):
self.prefix = prefix
def check(self, schema):
+ QAPISchemaType.check(self, schema)
seen = {}
for v in self.values:
v.check_clash(self.info, seen)
@@ -1196,8 +1203,10 @@ class QAPISchemaArrayType(QAPISchemaType):
self.element_type = None
def check(self, schema):
+ QAPISchemaType.check(self, schema)
self.element_type = schema.lookup_type(self._element_type_name)
assert self.element_type
+ self.element_type.check(schema)
self.ifcond = self.element_type.ifcond
def is_implicit(self):
@@ -1240,6 +1249,7 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members = None
def check(self, schema):
+ QAPISchemaType.check(self, schema)
if self.members is False: # check for cycles
raise QAPISemError(self.info,
"Object %s contains itself" % self.name)
@@ -1430,6 +1440,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
+ QAPISchemaType.check(self, schema)
self.variants.tag_member.check(schema)
# Not calling self.variants.check_clash(), because there's nothing
# to clash with
@@ -1474,6 +1485,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
self.allow_preconfig = allow_preconfig
def check(self, schema):
+ QAPISchemaEntity.check(self, schema)
if self._arg_type_name:
self.arg_type = schema.lookup_type(self._arg_type_name)
assert (isinstance(self.arg_type, QAPISchemaObjectType) or
@@ -1509,6 +1521,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
self.boxed = boxed
def check(self, schema):
+ QAPISchemaEntity.check(self, schema)
if self._arg_type_name:
self.arg_type = schema.lookup_type(self._arg_type_name)
assert (isinstance(self.arg_type, QAPISchemaObjectType) or
@@ -1642,7 +1655,7 @@ class QAPISchema(object):
# But it's not tight: the disjunction need not imply it. We
# may end up compiling useless wrapper types.
# TODO kill simple unions or implement the disjunction
- assert ifcond == typ.ifcond
+ assert ifcond == typ._ifcond # pylint: disable=protected-access
else:
self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
None, members, None))
@@ -1688,7 +1701,7 @@ class QAPISchema(object):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
typ = self._make_implicit_object_type(
- typ, info, None, self.lookup_type(typ).ifcond,
+ typ, info, None, self.lookup_type(typ),
'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (3 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check() Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-28 17:48 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 06/15] qapi: mcgen() shouldn't indent # lines Marc-André Lureau
` (9 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Modify the test visitor to check correct passing of values.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/commands.py | 2 +-
scripts/qapi/common.py | 31 ++++++++++++++------------
scripts/qapi/doc.py | 10 ++++-----
scripts/qapi/events.py | 2 +-
scripts/qapi/introspect.py | 12 +++++-----
scripts/qapi/types.py | 8 +++----
scripts/qapi/visit.py | 8 +++----
tests/qapi-schema/qapi-schema-test.out | 9 ++++++++
tests/qapi-schema/test-qapi.py | 19 +++++++++++-----
9 files changed, 61 insertions(+), 40 deletions(-)
mode change 100644 => 100755 scripts/qapi/doc.py
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3b0867c14f..dcc03c7859 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -277,7 +277,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
c_prefix=c_name(self._prefix, protect=False)))
genc.add(gen_registry(self._regy, self._prefix))
- def visit_command(self, name, info, arg_type, ret_type, gen,
+ def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
if not gen:
return
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 46e33e23e4..feae646e09 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1062,26 +1062,26 @@ class QAPISchemaVisitor(object):
def visit_builtin_type(self, name, info, json_type):
pass
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
pass
- def visit_array_type(self, name, info, element_type):
+ def visit_array_type(self, name, info, ifcond, element_type):
pass
- def visit_object_type(self, name, info, base, members, variants):
+ def visit_object_type(self, name, info, ifcond, base, members, variants):
pass
- def visit_object_type_flat(self, name, info, members, variants):
+ def visit_object_type_flat(self, name, info, ifcond, members, variants):
pass
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
pass
- def visit_command(self, name, info, arg_type, ret_type, gen,
+ def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
pass
- def visit_event(self, name, info, arg_type, boxed):
+ def visit_event(self, name, info, ifcond, arg_type, boxed):
pass
@@ -1191,7 +1191,7 @@ class QAPISchemaEnumType(QAPISchemaType):
return 'string'
def visit(self, visitor):
- visitor.visit_enum_type(self.name, self.info,
+ visitor.visit_enum_type(self.name, self.info, self.ifcond,
self.member_names(), self.prefix)
@@ -1225,7 +1225,8 @@ class QAPISchemaArrayType(QAPISchemaType):
return 'array of ' + elt_doc_type
def visit(self, visitor):
- visitor.visit_array_type(self.name, self.info, self.element_type)
+ visitor.visit_array_type(self.name, self.info, self.ifcond,
+ self.element_type)
class QAPISchemaObjectType(QAPISchemaType):
@@ -1307,9 +1308,9 @@ class QAPISchemaObjectType(QAPISchemaType):
return 'object'
def visit(self, visitor):
- visitor.visit_object_type(self.name, self.info,
+ visitor.visit_object_type(self.name, self.info, self.ifcond,
self.base, self.local_members, self.variants)
- visitor.visit_object_type_flat(self.name, self.info,
+ visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
self.members, self.variants)
@@ -1462,7 +1463,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
return 'value'
def visit(self, visitor):
- visitor.visit_alternate_type(self.name, self.info, self.variants)
+ visitor.visit_alternate_type(self.name, self.info, self.ifcond,
+ self.variants)
def is_empty(self):
return False
@@ -1505,7 +1507,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
assert isinstance(self.ret_type, QAPISchemaType)
def visit(self, visitor):
- visitor.visit_command(self.name, self.info,
+ visitor.visit_command(self.name, self.info, self.ifcond,
self.arg_type, self.ret_type,
self.gen, self.success_response,
self.boxed, self.allow_oob,
@@ -1538,7 +1540,8 @@ class QAPISchemaEvent(QAPISchemaEntity):
raise QAPISemError(self.info, "Use of 'boxed' requires 'data'")
def visit(self, visitor):
- visitor.visit_event(self.name, self.info, self.arg_type, self.boxed)
+ visitor.visit_event(self.name, self.info, self.ifcond,
+ self.arg_type, self.boxed)
class QAPISchema(object):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
old mode 100644
new mode 100755
index b5630844f9..4db6674dc3
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -204,14 +204,14 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
def write(self, output_dir):
self._gen.write(output_dir, self._prefix + 'qapi-doc.texi')
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
doc = self.cur_doc
self._gen.add(TYPE_FMT(type='Enum',
name=doc.symbol,
body=texi_entity(doc, 'Values',
member_func=texi_enum_value)))
- def visit_object_type(self, name, info, base, members, variants):
+ def visit_object_type(self, name, info, ifcond, base, members, variants):
doc = self.cur_doc
if base and base.is_implicit():
base = None
@@ -220,13 +220,13 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
body=texi_entity(doc, 'Members',
base, variants)))
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
doc = self.cur_doc
self._gen.add(TYPE_FMT(type='Alternate',
name=doc.symbol,
body=texi_entity(doc, 'Members')))
- def visit_command(self, name, info, arg_type, ret_type, gen,
+ def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
doc = self.cur_doc
if boxed:
@@ -240,7 +240,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
name=doc.symbol,
body=body))
- def visit_event(self, name, info, arg_type, boxed):
+ def visit_event(self, name, info, ifcond, arg_type, boxed):
doc = self.cur_doc
self._gen.add(MSG_FMT(type='Event',
name=doc.symbol,
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 5657524688..0a1afac134 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -184,7 +184,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
genh.add(gen_enum(self._enum_name, self._event_names))
genc.add(gen_enum_lookup(self._enum_name, self._event_names))
- def visit_event(self, name, info, arg_type, boxed):
+ def visit_event(self, name, info, ifcond, arg_type, boxed):
self._genh.add(gen_event_send_decl(name, arg_type, boxed))
self._genc.add(gen_event_send(name, arg_type, boxed, self._enum_name))
self._event_names.append(name)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 6ad198ae5b..245cfdfb65 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -149,26 +149,26 @@ const QLitObject %(c_name)s = %(c_string)s;
def visit_builtin_type(self, name, info, json_type):
self._gen_qlit(name, 'builtin', {'json-type': json_type})
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
self._gen_qlit(name, 'enum', {'values': values})
- def visit_array_type(self, name, info, element_type):
+ def visit_array_type(self, name, info, ifcond, element_type):
element = self._use_type(element_type)
self._gen_qlit('[' + element + ']', 'array', {'element-type': element})
- def visit_object_type_flat(self, name, info, members, variants):
+ def visit_object_type_flat(self, name, info, ifcond, members, variants):
obj = {'members': [self._gen_member(m) for m in members]}
if variants:
obj.update(self._gen_variants(variants.tag_member.name,
variants.variants))
self._gen_qlit(name, 'object', obj)
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
self._gen_qlit(name, 'alternate',
{'members': [{'type': self._use_type(m.type)}
for m in variants.variants]})
- def visit_command(self, name, info, arg_type, ret_type, gen,
+ def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
@@ -178,7 +178,7 @@ const QLitObject %(c_name)s = %(c_string)s;
'allow-oob': allow_oob,
'allow-preconfig': allow_preconfig})
- def visit_event(self, name, info, arg_type, boxed):
+ def visit_event(self, name, info, ifcond, arg_type, boxed):
arg_type = arg_type or self._schema.the_empty_object_type
self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)})
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index a599352e59..659075f884 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -208,16 +208,16 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
self._genh.add(gen_type_cleanup_decl(name))
self._genc.add(gen_type_cleanup(name))
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
self._genh.preamble_add(gen_enum(name, values, prefix))
self._genc.add(gen_enum_lookup(name, values, prefix))
- def visit_array_type(self, name, info, element_type):
+ def visit_array_type(self, name, info, ifcond, element_type):
self._genh.preamble_add(gen_fwd_object_or_array(name))
self._genh.add(gen_array(name, element_type))
self._gen_type_cleanup(name)
- def visit_object_type(self, name, info, base, members, variants):
+ def visit_object_type(self, name, info, ifcond, base, members, variants):
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
@@ -231,7 +231,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
# implicit types won't be directly allocated/freed
self._gen_type_cleanup(name)
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
self._genh.preamble_add(gen_fwd_object_or_array(name))
self._genh.add(gen_object(name, None,
[variants.tag_member], variants))
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index bdcafb64ee..34fe1ef5eb 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -310,15 +310,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
''',
types=types))
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
self._genh.add(gen_visit_decl(name, scalar=True))
self._genc.add(gen_visit_enum(name))
- def visit_array_type(self, name, info, element_type):
+ def visit_array_type(self, name, info, ifcond, element_type):
self._genh.add(gen_visit_decl(name))
self._genc.add(gen_visit_list(name, element_type))
- def visit_object_type(self, name, info, base, members, variants):
+ def visit_object_type(self, name, info, ifcond, base, members, variants):
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
@@ -331,7 +331,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
self._genh.add(gen_visit_decl(name))
self._genc.add(gen_visit_object(name, base, members, variants))
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
self._genh.add(gen_visit_decl(name))
self._genc.add(gen_visit_alternate(name, variants))
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index ed25e5b60c..0da92455da 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -237,25 +237,34 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio
gen=True success_response=True boxed=False oob=False preconfig=False
object TestIfStruct
member foo: int optional=False
+ if ['defined(TEST_IF_STRUCT)']
enum TestIfEnum ['foo', 'bar']
+ if ['defined(TEST_IF_ENUM)']
object q_obj_TestStruct-wrapper
member data: TestStruct optional=False
enum TestIfUnionKind ['foo']
+ if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
object TestIfUnion
member type: TestIfUnionKind optional=False
tag type
case foo: q_obj_TestStruct-wrapper
+ if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
alternate TestIfAlternate
tag type
case foo: int
case bar: TestStruct
+ if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
object q_obj_TestIfCmd-arg
member foo: TestIfStruct optional=False
+ if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
gen=True success_response=True boxed=False oob=False preconfig=False
+ if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
command TestCmdReturnDefThree None -> UserDefThree
gen=True success_response=True boxed=False oob=False preconfig=False
object q_obj_TestIfEvent-arg
member foo: TestIfStruct optional=False
+ if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
event TestIfEvent q_obj_TestIfEvent-arg
boxed=False
+ if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 4512a41504..43aaf5a20b 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -23,12 +23,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
def visit_include(self, name, info):
print('include %s' % name)
- def visit_enum_type(self, name, info, values, prefix):
+ def visit_enum_type(self, name, info, ifcond, values, prefix):
print('enum %s %s' % (name, values))
if prefix:
print(' prefix %s' % prefix)
+ self._print_if(ifcond)
- def visit_object_type(self, name, info, base, members, variants):
+ def visit_object_type(self, name, info, ifcond, base, members, variants):
print('object %s' % name)
if base:
print(' base %s' % base.name)
@@ -36,21 +37,25 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
print(' member %s: %s optional=%s' % \
(m.name, m.type.name, m.optional))
self._print_variants(variants)
+ self._print_if(ifcond)
- def visit_alternate_type(self, name, info, variants):
+ def visit_alternate_type(self, name, info, ifcond, variants):
print('alternate %s' % name)
self._print_variants(variants)
+ self._print_if(ifcond)
- def visit_command(self, name, info, arg_type, ret_type, gen,
+ def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
print('command %s %s -> %s' % \
(name, arg_type and arg_type.name, ret_type and ret_type.name))
print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s' % \
(gen, success_response, boxed, allow_oob, allow_preconfig))
+ self._print_if(ifcond)
- def visit_event(self, name, info, arg_type, boxed):
+ def visit_event(self, name, info, ifcond, arg_type, boxed):
print('event %s %s' % (name, arg_type and arg_type.name))
print(' boxed=%s' % boxed)
+ self._print_if(ifcond)
@staticmethod
def _print_variants(variants):
@@ -59,6 +64,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
for v in variants.variants:
print(' case %s: %s' % (v.name, v.type.name))
+ @staticmethod
+ def _print_if(ifcond, indent=4):
+ if ifcond:
+ print('%sif %s' % (' ' * indent, ifcond))
try:
schema = QAPISchema(sys.argv[1])
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 06/15] qapi: mcgen() shouldn't indent # lines
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (4 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers Marc-André Lureau
` (8 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Skip preprocessor lines when adding indentation, since that would
likely result in invalid code.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/common.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index feae646e09..1b56065a80 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1941,8 +1941,8 @@ def cgen(code, **kwds):
if indent_level:
indent = genindent(indent_level)
# re.subn() lacks flags support before Python 2.7, use re.compile()
- raw = re.subn(re.compile(r'^.', re.MULTILINE),
- indent + r'\g<0>', raw)
+ raw = re.subn(re.compile(r'^(?!(#|$))', re.MULTILINE),
+ indent, raw)
raw = raw[0]
return re.sub(re.escape(eatspace) + r' *', '', raw)
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (5 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 06/15] qapi: mcgen() shouldn't indent # lines Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 11:53 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 08/15] qapi-introspect: modify to_qlit() to append ', ' on level > 0 Marc-André Lureau
` (7 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Add helpers to wrap generated code with #if/#endif lines.
Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
inherit from it, for full C files with copyright headers etc.
Add a 'with' statement context manager that will be used to wrap
generator visitor methods. The manager will check if code was
generated before adding #if/#endif lines on QAPIGenCSnippet
objects. Used in the following patches.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 97 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 1b56065a80..44eaf25850 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -12,6 +12,7 @@
# See the COPYING file in the top-level directory.
from __future__ import print_function
+from contextlib import contextmanager
import errno
import os
import re
@@ -1974,6 +1975,40 @@ def guardend(name):
name=guardname(name))
+def gen_if(ifcond):
+ ret = ''
+ for ifc in ifcond:
+ ret += mcgen('''
+#if %(cond)s
+''', cond=ifc)
+ return ret
+
+
+def gen_endif(ifcond):
+ ret = ''
+ for ifc in reversed(ifcond):
+ ret += mcgen('''
+#endif /* %(cond)s */
+''', cond=ifc)
+ return ret
+
+
+def wrap_ifcond(ifcond, before, after):
+ if ifcond is None or before == after:
+ return after
+
+ assert after.startswith(before)
+ out = before
+ added = after[len(before):]
+ if added[0] == '\n':
+ out += '\n'
+ added = added[1:]
+ out += gen_if(ifcond)
+ out += added
+ out += gen_endif(ifcond)
+ return out
+
+
def gen_enum_lookup(name, values, prefix=None):
ret = mcgen('''
@@ -2064,6 +2099,7 @@ class QAPIGen(object):
def __init__(self):
self._preamble = ''
self._body = ''
+ self._start_if = None
def preamble_add(self, text):
self._preamble += text
@@ -2071,6 +2107,25 @@ class QAPIGen(object):
def add(self, text):
self._body += text
+ def start_if(self, ifcond):
+ assert self._start_if is None
+
+ self._start_if = (ifcond, self._body, self._preamble)
+
+ def _wrap_ifcond(self):
+ pass
+
+ def end_if(self):
+ assert self._start_if
+
+ self._wrap_ifcond()
+ self._start_if = None
+
+ def get_content(self, fname=None):
+ assert self._start_if is None
+ return (self._top(fname) + self._preamble + self._body
+ + self._bottom(fname))
+
def _top(self, fname):
return ''
@@ -2091,8 +2146,7 @@ class QAPIGen(object):
f = open(fd, 'r+', encoding='utf-8')
else:
f = os.fdopen(fd, 'r+')
- text = (self._top(fname) + self._preamble + self._body
- + self._bottom(fname))
+ text = self.get_content(fname)
oldtext = f.read(len(text) + 1)
if text != oldtext:
f.seek(0)
@@ -2101,10 +2155,49 @@ class QAPIGen(object):
f.close()
-class QAPIGenC(QAPIGen):
+@contextmanager
+def ifcontext(ifcond, *args):
+ """A 'with' statement context manager to wrap with start_if()/end_if()
- def __init__(self, blurb, pydoc):
+ *args: variable length argument list of QAPIGen
+
+ Example::
+
+ with ifcontext(ifcond, self._genh, self._genc):
+ modify self._genh and self._genc ...
+
+ Is equivalent to calling::
+
+ self._genh.start_if(ifcond)
+ self._genc.start_if(ifcond)
+ modify self._genh and self._genc ...
+ self._genh.end_if()
+ self._genc.end_if()
+
+ """
+ for arg in args:
+ arg.start_if(ifcond)
+ yield
+ for arg in args:
+ arg.end_if()
+
+
+class QAPIGenCCode(QAPIGen):
+
+ def __init__(self):
QAPIGen.__init__(self)
+
+ def _wrap_ifcond(self):
+ self._body = wrap_ifcond(self._start_if[0],
+ self._start_if[1], self._body)
+ self._preamble = wrap_ifcond(self._start_if[0],
+ self._start_if[2], self._preamble)
+
+
+class QAPIGenC(QAPIGenCCode):
+
+ def __init__(self, blurb, pydoc):
+ QAPIGenCCode.__init__(self)
self._blurb = blurb
self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
re.MULTILINE))
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 08/15] qapi-introspect: modify to_qlit() to append ', ' on level > 0
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (6 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit Marc-André Lureau
` (6 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
The following patch is going to break list entries with #if/#endif, so
they should have the trailing ',' as suffix.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/introspect.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 245cfdfb65..bd7e1219be 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,7 +30,7 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
for elt in obj]
elts.append(indent(level + 1) + "{}")
ret += 'QLIT_QLIST(((QLitObject[]) {\n'
- ret += ',\n'.join(elts) + '\n'
+ ret += '\n'.join(elts) + '\n'
ret += indent(level) + '}))'
elif isinstance(obj, dict):
elts = []
@@ -45,6 +45,8 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
assert False # not implemented
+ if level > 0:
+ ret += ','
return ret
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (7 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 08/15] qapi-introspect: modify to_qlit() to append ', ' on level > 0 Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 12:09 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands Marc-André Lureau
` (5 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
The generator will take (obj, condition) tuples to wrap generated QLit
objects for 'obj' with #if/#endif conditions.
This commit adds 'ifcond' condition to top-level QLit objects.
See generated tests/test-qmp-introspect.c. Example diff after this patch:
--- before 2018-01-08 11:55:24.757083654 +0100
+++ tests/test-qmp-introspect.c 2018-01-08 13:08:44.477641629 +0100
@@ -51,6 +51,8 @@
{ "name", QLIT_QSTR("EVENT_F"), },
{}
})),
+#if defined(TEST_IF_CMD)
+#if defined(TEST_IF_STRUCT)
QLIT_QDICT(((QLitDictEntry[]) {
{ "arg-type", QLIT_QSTR("5"), },
{ "meta-type", QLIT_QSTR("command"), },
@@ -58,12 +60,16 @@
{ "ret-type", QLIT_QSTR("0"), },
{}
})),
+#endif /* defined(TEST_IF_STRUCT) */
+#endif /* defined(TEST_IF_CMD) */
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/introspect.py | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index bd7e1219be..71d4a779ce 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -18,6 +18,15 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
def indent(level):
return level * 4 * ' '
+ if isinstance(obj, tuple):
+ ifobj, ifcond = obj
+ ret = gen_if(ifcond)
+ ret += to_qlit(ifobj, level)
+ endif = gen_endif(ifcond)
+ if endif:
+ ret += '\n' + endif
+ return ret
+
ret = ''
if not suppress_first_indent:
ret += indent(level)
@@ -26,7 +35,7 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
elif isinstance(obj, str):
ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
elif isinstance(obj, list):
- elts = [to_qlit(elt, level + 1)
+ elts = [to_qlit(elt, level + 1).strip('\n')
for elt in obj]
elts.append(indent(level + 1) + "{}")
ret += 'QLIT_QLIST(((QLitObject[]) {\n'
@@ -128,12 +137,12 @@ const QLitObject %(c_name)s = %(c_string)s;
return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
- def _gen_qlit(self, name, mtype, obj):
+ def _gen_qlit(self, name, mtype, obj, ifcond):
if mtype not in ('command', 'event', 'builtin', 'array'):
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
- self._qlits.append(obj)
+ self._qlits.append((obj, ifcond))
def _gen_member(self, member):
ret = {'name': member.name, 'type': self._use_type(member.type)}
@@ -149,26 +158,27 @@ const QLitObject %(c_name)s = %(c_string)s;
return {'case': variant.name, 'type': self._use_type(variant.type)}
def visit_builtin_type(self, name, info, json_type):
- self._gen_qlit(name, 'builtin', {'json-type': json_type})
+ self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
def visit_enum_type(self, name, info, ifcond, values, prefix):
- self._gen_qlit(name, 'enum', {'values': values})
+ self._gen_qlit(name, 'enum', {'values': values}, ifcond)
def visit_array_type(self, name, info, ifcond, element_type):
element = self._use_type(element_type)
- self._gen_qlit('[' + element + ']', 'array', {'element-type': element})
+ self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
+ ifcond)
def visit_object_type_flat(self, name, info, ifcond, members, variants):
obj = {'members': [self._gen_member(m) for m in members]}
if variants:
obj.update(self._gen_variants(variants.tag_member.name,
variants.variants))
- self._gen_qlit(name, 'object', obj)
+ self._gen_qlit(name, 'object', obj, ifcond)
def visit_alternate_type(self, name, info, ifcond, variants):
self._gen_qlit(name, 'alternate',
{'members': [{'type': self._use_type(m.type)}
- for m in variants.variants]})
+ for m in variants.variants]}, ifcond)
def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
@@ -178,11 +188,12 @@ const QLitObject %(c_name)s = %(c_string)s;
{'arg-type': self._use_type(arg_type),
'ret-type': self._use_type(ret_type),
'allow-oob': allow_oob,
- 'allow-preconfig': allow_preconfig})
+ 'allow-preconfig': allow_preconfig}, ifcond)
def visit_event(self, name, info, ifcond, arg_type, boxed):
arg_type = arg_type or self._schema.the_empty_object_type
- self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)})
+ self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)},
+ ifcond)
def gen_introspect(schema, output_dir, prefix, opt_unmask):
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (8 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 14:43 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events Marc-André Lureau
` (4 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Wrap generated code with #if/#endif using an 'ifcontext' on
QAPIGenCSnippet objects.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/commands.py | 21 ++++++++++++---------
tests/test-qmp-cmds.c | 5 +++--
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index dcc03c7859..72749c7fc5 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -239,7 +239,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
QAPISchemaModularCVisitor.__init__(
self, prefix, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', __doc__)
- self._regy = ''
+ self._regy = QAPIGenCCode()
self._visited_ret_types = {}
def _begin_module(self, name):
@@ -275,20 +275,23 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
''',
c_prefix=c_name(self._prefix, protect=False)))
- genc.add(gen_registry(self._regy, self._prefix))
+ genc.add(gen_registry(self._regy.get_content(), self._prefix))
def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
if not gen:
return
- self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
- if ret_type and ret_type not in self._visited_ret_types[self._genc]:
+ if ret_type and \
+ ret_type not in self._visited_ret_types[self._genc]:
self._visited_ret_types[self._genc].add(ret_type)
- self._genc.add(gen_marshal_output(ret_type))
- self._genh.add(gen_marshal_decl(name))
- self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
- self._regy += gen_register_command(name, success_response, allow_oob,
- allow_preconfig)
+ with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
+ self._genc.add(gen_marshal_output(ret_type))
+ with ifcontext(ifcond, self._genh, self._genc, self._regy):
+ self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
+ self._genh.add(gen_marshal_decl(name))
+ self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+ self._regy.add(gen_register_command(name, success_response,
+ allow_oob, allow_preconfig))
def gen_commands(schema, output_dir, prefix):
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 840530b84c..bd27353908 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -12,12 +12,13 @@
static QmpCommandList qmp_commands;
-/* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
+#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
+void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
{
return NULL;
}
-/* #endif */
+#endif
UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
{
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (9 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 14:47 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors Marc-André Lureau
` (3 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Wrap generated code with #if/#endif using an 'ifcontext' on
QAPIGenCSnippet objects.
This makes a conditional event's qapi_event_send_FOO() compile-time
conditional, but its enum QAPIEvent member remains unconditional for
now. A follow up patch "qapi-event: add 'if' condition to implicit
event enum" will improve this.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/events.py | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 0a1afac134..764ef177ab 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -185,8 +185,10 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
genc.add(gen_enum_lookup(self._enum_name, self._event_names))
def visit_event(self, name, info, ifcond, arg_type, boxed):
- self._genh.add(gen_event_send_decl(name, arg_type, boxed))
- self._genc.add(gen_event_send(name, arg_type, boxed, self._enum_name))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.add(gen_event_send_decl(name, arg_type, boxed))
+ self._genc.add(gen_event_send(name, arg_type, boxed,
+ self._enum_name))
self._event_names.append(name)
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (10 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 14:50 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation Marc-André Lureau
` (2 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
From: Markus Armbruster <armbru@redhat.com>
Types & visitors are coupled and must be handled together to avoid
temporary build regression.
Wrap generated types/visitor code with #if/#endif using the context
helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/types.py | 48 ++++++++++++++++++++++++++-----------------
scripts/qapi/visit.py | 33 ++++++++++++++++-------------
2 files changed, 48 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 659075f884..fd7808103c 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -55,7 +55,7 @@ def gen_struct_members(members):
return ret
-def gen_object(name, base, members, variants):
+def gen_object(name, ifcond, base, members, variants):
if name in objects_seen:
return ''
objects_seen.add(name)
@@ -64,11 +64,14 @@ def gen_object(name, base, members, variants):
if variants:
for v in variants.variants:
if isinstance(v.type, QAPISchemaObjectType):
- ret += gen_object(v.type.name, v.type.base,
+ ret += gen_object(v.type.name, v.type.ifcond, v.type.base,
v.type.local_members, v.type.variants)
ret += mcgen('''
+''')
+ ret += gen_if(ifcond)
+ ret += mcgen('''
struct %(c_name)s {
''',
c_name=c_name(name))
@@ -101,6 +104,7 @@ struct %(c_name)s {
ret += mcgen('''
};
''')
+ ret += gen_endif(ifcond)
return ret
@@ -209,33 +213,39 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
self._genc.add(gen_type_cleanup(name))
def visit_enum_type(self, name, info, ifcond, values, prefix):
- self._genh.preamble_add(gen_enum(name, values, prefix))
- self._genc.add(gen_enum_lookup(name, values, prefix))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.preamble_add(gen_enum(name, values, prefix))
+ self._genc.add(gen_enum_lookup(name, values, prefix))
def visit_array_type(self, name, info, ifcond, element_type):
- self._genh.preamble_add(gen_fwd_object_or_array(name))
- self._genh.add(gen_array(name, element_type))
- self._gen_type_cleanup(name)
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.preamble_add(gen_fwd_object_or_array(name))
+ self._genh.add(gen_array(name, element_type))
+ self._gen_type_cleanup(name)
def visit_object_type(self, name, info, ifcond, base, members, variants):
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
- self._genh.preamble_add(gen_fwd_object_or_array(name))
- self._genh.add(gen_object(name, base, members, variants))
- if base and not base.is_implicit():
- self._genh.add(gen_upcast(name, base))
- # TODO Worth changing the visitor signature, so we could
- # directly use rather than repeat type.is_implicit()?
- if not name.startswith('q_'):
- # implicit types won't be directly allocated/freed
- self._gen_type_cleanup(name)
+ with ifcontext(ifcond, self._genh):
+ self._genh.preamble_add(gen_fwd_object_or_array(name))
+ self._genh.add(gen_object(name, ifcond, base, members, variants))
+ with ifcontext(ifcond, self._genh, self._genc):
+ if base and not base.is_implicit():
+ self._genh.add(gen_upcast(name, base))
+ # TODO Worth changing the visitor signature, so we could
+ # directly use rather than repeat type.is_implicit()?
+ if not name.startswith('q_'):
+ # implicit types won't be directly allocated/freed
+ self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, ifcond, variants):
- self._genh.preamble_add(gen_fwd_object_or_array(name))
- self._genh.add(gen_object(name, None,
+ with ifcontext(ifcond, self._genh):
+ self._genh.preamble_add(gen_fwd_object_or_array(name))
+ self._genh.add(gen_object(name, ifcond, None,
[variants.tag_member], variants))
- self._gen_type_cleanup(name)
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._gen_type_cleanup(name)
def gen_types(schema, output_dir, prefix, opt_builtins):
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 34fe1ef5eb..dd5034a66a 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -311,29 +311,34 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
types=types))
def visit_enum_type(self, name, info, ifcond, values, prefix):
- self._genh.add(gen_visit_decl(name, scalar=True))
- self._genc.add(gen_visit_enum(name))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.add(gen_visit_decl(name, scalar=True))
+ self._genc.add(gen_visit_enum(name))
def visit_array_type(self, name, info, ifcond, element_type):
- self._genh.add(gen_visit_decl(name))
- self._genc.add(gen_visit_list(name, element_type))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.add(gen_visit_decl(name))
+ self._genc.add(gen_visit_list(name, element_type))
def visit_object_type(self, name, info, ifcond, base, members, variants):
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
- self._genh.add(gen_visit_members_decl(name))
- self._genc.add(gen_visit_object_members(name, base, members, variants))
- # TODO Worth changing the visitor signature, so we could
- # directly use rather than repeat type.is_implicit()?
- if not name.startswith('q_'):
- # only explicit types need an allocating visit
- self._genh.add(gen_visit_decl(name))
- self._genc.add(gen_visit_object(name, base, members, variants))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.add(gen_visit_members_decl(name))
+ self._genc.add(gen_visit_object_members(name, base,
+ members, variants))
+ # TODO Worth changing the visitor signature, so we could
+ # directly use rather than repeat type.is_implicit()?
+ if not name.startswith('q_'):
+ # only explicit types need an allocating visit
+ self._genh.add(gen_visit_decl(name))
+ self._genc.add(gen_visit_object(name, base, members, variants))
def visit_alternate_type(self, name, info, ifcond, variants):
- self._genh.add(gen_visit_decl(name))
- self._genc.add(gen_visit_alternate(name, variants))
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genh.add(gen_visit_decl(name))
+ self._genc.add(gen_visit_alternate(name, variants))
def gen_visit(schema, output_dir, prefix, opt_builtins):
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (11 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 14:59 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE " Marc-André Lureau
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
The documentation is generated only once, and doesn't know C
pre-conditions. Add 'If:' sections for top-level entities.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/doc.py | 22 ++++++++++++----------
tests/qapi-schema/doc-good.json | 2 +-
tests/qapi-schema/doc-good.out | 1 +
tests/qapi-schema/doc-good.texi | 2 ++
4 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 4db6674dc3..987fd3c943 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -174,7 +174,7 @@ def texi_members(doc, what, base, variants, member_func):
return '\n@b{%s:}\n@table @asis\n%s@end table\n' % (what, items)
-def texi_sections(doc):
+def texi_sections(doc, ifcond):
"""Format additional sections following arguments"""
body = ''
for section in doc.sections:
@@ -185,14 +185,16 @@ def texi_sections(doc):
body += texi_example(section.text)
else:
body += texi_format(section.text)
+ if ifcond:
+ body += '\n\n@b{If:} @code{%s}' % ", ".join(ifcond)
return body
-def texi_entity(doc, what, base=None, variants=None,
+def texi_entity(doc, what, ifcond, base=None, variants=None,
member_func=texi_member):
return (texi_body(doc)
+ texi_members(doc, what, base, variants, member_func)
- + texi_sections(doc))
+ + texi_sections(doc, ifcond))
class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
@@ -208,7 +210,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
doc = self.cur_doc
self._gen.add(TYPE_FMT(type='Enum',
name=doc.symbol,
- body=texi_entity(doc, 'Values',
+ body=texi_entity(doc, 'Values', ifcond,
member_func=texi_enum_value)))
def visit_object_type(self, name, info, ifcond, base, members, variants):
@@ -217,14 +219,14 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
base = None
self._gen.add(TYPE_FMT(type='Object',
name=doc.symbol,
- body=texi_entity(doc, 'Members',
+ body=texi_entity(doc, 'Members', ifcond,
base, variants)))
def visit_alternate_type(self, name, info, ifcond, variants):
doc = self.cur_doc
self._gen.add(TYPE_FMT(type='Alternate',
name=doc.symbol,
- body=texi_entity(doc, 'Members')))
+ body=texi_entity(doc, 'Members', ifcond)))
def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
@@ -233,9 +235,9 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
body = texi_body(doc)
body += ('\n@b{Arguments:} the members of @code{%s}\n'
% arg_type.name)
- body += texi_sections(doc)
+ body += texi_sections(doc, ifcond)
else:
- body = texi_entity(doc, 'Arguments')
+ body = texi_entity(doc, 'Arguments', ifcond)
self._gen.add(MSG_FMT(type='Command',
name=doc.symbol,
body=body))
@@ -244,7 +246,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
doc = self.cur_doc
self._gen.add(MSG_FMT(type='Event',
name=doc.symbol,
- body=texi_entity(doc, 'Arguments')))
+ body=texi_entity(doc, 'Arguments', ifcond)))
def symbol(self, doc, entity):
if self._gen._body:
@@ -257,7 +259,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
assert not doc.args
if self._gen._body:
self._gen.add('\n')
- self._gen.add(texi_body(doc) + texi_sections(doc))
+ self._gen.add(texi_body(doc) + texi_sections(doc, None))
def gen_doc(schema, output_dir, prefix):
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 97ab4625ff..984cd8ed06 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -55,7 +55,7 @@
#
# @two is undocumented
##
-{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
+{ 'enum': 'Enum', 'data': [ 'one', 'two' ], 'if': 'defined(IFCOND)' }
##
# @Base:
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 9c8a4838e1..35f3f1164c 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -3,6 +3,7 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
prefix QTYPE
module doc-good.json
enum Enum ['one', 'two']
+ if ['defined(IFCOND)']
object Base
member base1: Enum optional=False
object Variant1
diff --git a/tests/qapi-schema/doc-good.texi b/tests/qapi-schema/doc-good.texi
index 0aed2300a5..e42eace474 100644
--- a/tests/qapi-schema/doc-good.texi
+++ b/tests/qapi-schema/doc-good.texi
@@ -89,6 +89,8 @@ Not documented
@end table
@code{two} is undocumented
+
+@b{If:} @code{defined(IFCOND)}
@end deftp
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (12 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 15:04 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE " Marc-André Lureau
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Add #if defined(CONFIG_VNC) in generated code, and adjust the
qmp/hmp code accordingly.
query-qmp-schema no longer reports the command/events etc as
available when disabled at compile.
Commands made conditional:
* query-vnc, query-vnc-servers, change-vnc-password
Before the patch, the commands for !CONFIG_VNC are stubs that fail
like this:
{"error": {"class": "GenericError",
"desc": "The feature 'vnc' is not enabled"}}
Afterwards, they fail like this:
{"error": {"class": "CommandNotFound",
"desc": "The command FOO has not been found"}}
I call that an improvement, because it lets clients distinguish
between command unavailable (class CommandNotFound) and command failed
(class GenericError).
Events made conditional:
* VNC_CONNECTED, VNC_INITIALIZED, VNC_DISCONNECTED
HMP change:
* info vnc
Will return "unknown command: 'info vnc'" when VNC is compiled
out (same as error for spice when --disable-spice)
Occurrences of VNC (case insensitive) in the schema that aren't
covered by this change:
* add_client
Command has other uses, including "socket bases character devices".
These are unconditional as far as I can tell.
* set_password, expire_password
In theory, these commands could be used for managing any service's
password. In practice, they're used for VNC and SPICE services.
They're documented for "remote display session" / "remote display
server".
The service is selected by argument @protocol. The code special-cases
protocol-specific argument checking, then calls a protocol-specific
function to do the work. If it fails, the command fails with "Could
not set password". It does when the service isn't compiled in (it's a
stub then).
We could make these commands conditional on the conjunction of all
services [currently: defined(CONFIG_VNC) || defined(CONFIG_SPICE)],
but I doubt it's worthwhile.
* change
Command has other uses, namely changing media.
This patch inlines a stub; no functional change.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
qapi/ui.json | 45 +++++++++++++++++++++++++++-----------------
ui/vnc.h | 2 ++
hmp.c | 9 ++++++++-
qmp.c | 30 ++++-------------------------
hmp-commands-info.hx | 2 ++
5 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/qapi/ui.json b/qapi/ui.json
index f48d2a0afb..ee90c1880b 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -377,7 +377,8 @@
'data': { 'host': 'str',
'service': 'str',
'family': 'NetworkAddressFamily',
- 'websocket': 'bool' } }
+ 'websocket': 'bool' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncServerInfo:
@@ -391,7 +392,8 @@
##
{ 'struct': 'VncServerInfo',
'base': 'VncBasicInfo',
- 'data': { '*auth': 'str' } }
+ 'data': { '*auth': 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncClientInfo:
@@ -408,7 +410,8 @@
##
{ 'struct': 'VncClientInfo',
'base': 'VncBasicInfo',
- 'data': { '*x509_dname': 'str', '*sasl_username': 'str' } }
+ 'data': { '*x509_dname': 'str', '*sasl_username': 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncInfo:
@@ -449,7 +452,8 @@
{ 'struct': 'VncInfo',
'data': {'enabled': 'bool', '*host': 'str',
'*family': 'NetworkAddressFamily',
- '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} }
+ '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']},
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncPrimaryAuth:
@@ -460,7 +464,8 @@
##
{ 'enum': 'VncPrimaryAuth',
'data': [ 'none', 'vnc', 'ra2', 'ra2ne', 'tight', 'ultra',
- 'tls', 'vencrypt', 'sasl' ] }
+ 'tls', 'vencrypt', 'sasl' ],
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncVencryptSubAuth:
@@ -474,8 +479,8 @@
'tls-none', 'x509-none',
'tls-vnc', 'x509-vnc',
'tls-plain', 'x509-plain',
- 'tls-sasl', 'x509-sasl' ] }
-
+ 'tls-sasl', 'x509-sasl' ],
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncServerInfo2:
@@ -492,8 +497,8 @@
{ 'struct': 'VncServerInfo2',
'base': 'VncBasicInfo',
'data': { 'auth' : 'VncPrimaryAuth',
- '*vencrypt' : 'VncVencryptSubAuth' } }
-
+ '*vencrypt' : 'VncVencryptSubAuth' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncInfo2:
@@ -525,7 +530,8 @@
'clients' : ['VncClientInfo'],
'auth' : 'VncPrimaryAuth',
'*vencrypt' : 'VncVencryptSubAuth',
- '*display' : 'str' } }
+ '*display' : 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @query-vnc:
@@ -556,8 +562,8 @@
# }
#
##
-{ 'command': 'query-vnc', 'returns': 'VncInfo' }
-
+{ 'command': 'query-vnc', 'returns': 'VncInfo',
+ 'if': 'defined(CONFIG_VNC)' }
##
# @query-vnc-servers:
#
@@ -567,7 +573,8 @@
#
# Since: 2.3
##
-{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'] }
+{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'],
+ 'if': 'defined(CONFIG_VNC)' }
##
# @change-vnc-password:
@@ -581,7 +588,8 @@
# Notes: An empty password in this command will set the password to the empty
# string. Existing clients are unaffected by executing this command.
##
-{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
+{ 'command': 'change-vnc-password', 'data': {'password': 'str'},
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VNC_CONNECTED:
@@ -610,7 +618,8 @@
##
{ 'event': 'VNC_CONNECTED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncBasicInfo' } }
+ 'client': 'VncBasicInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VNC_INITIALIZED:
@@ -637,7 +646,8 @@
##
{ 'event': 'VNC_INITIALIZED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncClientInfo' } }
+ 'client': 'VncClientInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VNC_DISCONNECTED:
@@ -663,7 +673,8 @@
##
{ 'event': 'VNC_DISCONNECTED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncClientInfo' } }
+ 'client': 'VncClientInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# = Input
diff --git a/ui/vnc.h b/ui/vnc.h
index 762632929b..a86e0610e8 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -297,7 +297,9 @@ struct VncState
bool encode_ws;
bool websocket;
+#ifdef CONFIG_VNC
VncClientInfo *info;
+#endif
/* Job thread bottom half has put data for a forced update
* into the output buffer. This offset points to the end of
diff --git a/hmp.c b/hmp.c
index f601099f90..3981fbe33e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -614,6 +614,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
qapi_free_BlockStatsList(stats_list);
}
+#ifdef CONFIG_VNC
/* Helper for hmp_info_vnc_clients, _servers */
static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
const char *name)
@@ -701,6 +702,7 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
qapi_free_VncInfo2List(info2l);
}
+#endif
#ifdef CONFIG_SPICE
void hmp_info_spice(Monitor *mon, const QDict *qdict)
@@ -1770,12 +1772,14 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+#ifdef CONFIG_VNC
static void hmp_change_read_arg(void *opaque, const char *password,
void *readline_opaque)
{
qmp_change_vnc_password(password, NULL);
monitor_read_command(opaque, 1);
}
+#endif
void hmp_change(Monitor *mon, const QDict *qdict)
{
@@ -1786,6 +1790,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
BlockdevChangeReadOnlyMode read_only_mode = 0;
Error *err = NULL;
+#ifdef CONFIG_VNC
if (strcmp(device, "vnc") == 0) {
if (read_only) {
monitor_printf(mon,
@@ -1800,7 +1805,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
}
}
qmp_change("vnc", target, !!arg, arg, &err);
- } else {
+ } else
+#endif
+ {
if (read_only) {
read_only_mode =
qapi_enum_parse(&BlockdevChangeReadOnlyMode_lookup,
diff --git a/qmp.c b/qmp.c
index 73e46d795f..bc5537b221 100644
--- a/qmp.c
+++ b/qmp.c
@@ -129,22 +129,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
}
}
-#ifndef CONFIG_VNC
-/* If VNC support is enabled, the "true" query-vnc command is
- defined in the VNC subsystem */
-VncInfo *qmp_query_vnc(Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
- return NULL;
-};
-
-VncInfo2List *qmp_query_vnc_servers(Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
- return NULL;
-};
-#endif
-
#ifndef CONFIG_SPICE
/*
* qmp_unregister_commands_hack() ensures that QMP command query-spice
@@ -412,23 +396,17 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
qmp_change_vnc_listen(target, errp);
}
}
-#else
-void qmp_change_vnc_password(const char *password, Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
-}
-static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
- Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
-}
#endif /* !CONFIG_VNC */
void qmp_change(const char *device, const char *target,
bool has_arg, const char *arg, Error **errp)
{
if (strcmp(device, "vnc") == 0) {
+#ifdef CONFIG_VNC
qmp_change_vnc(target, has_arg, arg, errp);
+#else
+ error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
+#endif
} else {
qmp_blockdev_change_medium(true, device, false, NULL, target,
has_arg, arg, false, 0, errp);
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 6db3457a78..9781cfad8d 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -425,6 +425,7 @@ STEXI
Show which guest mouse is receiving events.
ETEXI
+#if defined(CONFIG_VNC)
{
.name = "vnc",
.args_type = "",
@@ -432,6 +433,7 @@ ETEXI
.help = "show the vnc server status",
.cmd = hmp_info_vnc,
},
+#endif
STEXI
@item info vnc
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE type/commands/events on the schema
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
` (13 preceding siblings ...)
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema Marc-André Lureau
@ 2018-06-27 16:35 ` Marc-André Lureau
2018-07-03 15:05 ` Markus Armbruster
14 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, eblake, armbru,
Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
Add #if defined(CONFIG_SPICE) in generated code, and adjust the
qmp/hmp code accordingly.
query-qmp-schema no longer reports the command/events etc as
available when disabled at compile time.
Commands made conditional:
* query-spice
Before the patch, the command for !CONFIG_SPICE is unregistered. It
will fail with the same error.
Events made conditional:
* SPICE_CONNECTED, SPICE_INITIALIZED, SPICE_DISCONNECTED,
SPICE_MIGRATE_COMPLETED
Add TODO for conditional SPICE chardevs, delayed until the supports
for conditional members lands.
No HMP change, the code was already conditional.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
qapi/char.json | 8 ++++++--
qapi/ui.json | 30 ++++++++++++++++++++----------
monitor.c | 3 ---
qmp.c | 16 ----------------
4 files changed, 26 insertions(+), 31 deletions(-)
diff --git a/qapi/char.json b/qapi/char.json
index 60f31d83fc..b7b2a05766 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -320,6 +320,7 @@
##
{ 'struct': 'ChardevSpiceChannel', 'data': { 'type' : 'str' },
'base': 'ChardevCommon' }
+# TODO: 'if': 'defined(CONFIG_SPICE)'
##
# @ChardevSpicePort:
@@ -332,6 +333,7 @@
##
{ 'struct': 'ChardevSpicePort', 'data': { 'fqdn' : 'str' },
'base': 'ChardevCommon' }
+# TODO: 'if': 'defined(CONFIG_SPICE)'
##
# @ChardevVC:
@@ -385,8 +387,10 @@
'testdev': 'ChardevCommon',
'stdio' : 'ChardevStdio',
'console': 'ChardevCommon',
- 'spicevmc' : 'ChardevSpiceChannel',
- 'spiceport' : 'ChardevSpicePort',
+ 'spicevmc': 'ChardevSpiceChannel',
+# TODO: { 'type': 'ChardevSpiceChannel', 'if': 'defined(CONFIG_SPICE)' },
+ 'spiceport': 'ChardevSpicePort',
+# TODO: { 'type': 'ChardevSpicePort', 'if': 'defined(CONFIG_SPICE)' },
'vc' : 'ChardevVC',
'ringbuf': 'ChardevRingbuf',
# next one is just for compatibility
diff --git a/qapi/ui.json b/qapi/ui.json
index ee90c1880b..4ca91bb45a 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -118,7 +118,8 @@
{ 'struct': 'SpiceBasicInfo',
'data': { 'host': 'str',
'port': 'str',
- 'family': 'NetworkAddressFamily' } }
+ 'family': 'NetworkAddressFamily' },
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SpiceServerInfo:
@@ -131,7 +132,8 @@
##
{ 'struct': 'SpiceServerInfo',
'base': 'SpiceBasicInfo',
- 'data': { '*auth': 'str' } }
+ 'data': { '*auth': 'str' },
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SpiceChannel:
@@ -156,7 +158,8 @@
{ 'struct': 'SpiceChannel',
'base': 'SpiceBasicInfo',
'data': {'connection-id': 'int', 'channel-type': 'int', 'channel-id': 'int',
- 'tls': 'bool'} }
+ 'tls': 'bool'},
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SpiceQueryMouseMode:
@@ -175,7 +178,8 @@
# Since: 1.1
##
{ 'enum': 'SpiceQueryMouseMode',
- 'data': [ 'client', 'server', 'unknown' ] }
+ 'data': [ 'client', 'server', 'unknown' ],
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SpiceInfo:
@@ -212,7 +216,8 @@
{ 'struct': 'SpiceInfo',
'data': {'enabled': 'bool', 'migrated': 'bool', '*host': 'str', '*port': 'int',
'*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
- 'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']} }
+ 'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']},
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @query-spice:
@@ -257,7 +262,8 @@
# }
#
##
-{ 'command': 'query-spice', 'returns': 'SpiceInfo' }
+{ 'command': 'query-spice', 'returns': 'SpiceInfo',
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SPICE_CONNECTED:
@@ -282,7 +288,8 @@
##
{ 'event': 'SPICE_CONNECTED',
'data': { 'server': 'SpiceBasicInfo',
- 'client': 'SpiceBasicInfo' } }
+ 'client': 'SpiceBasicInfo' },
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SPICE_INITIALIZED:
@@ -310,7 +317,8 @@
##
{ 'event': 'SPICE_INITIALIZED',
'data': { 'server': 'SpiceServerInfo',
- 'client': 'SpiceChannel' } }
+ 'client': 'SpiceChannel' },
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SPICE_DISCONNECTED:
@@ -335,7 +343,8 @@
##
{ 'event': 'SPICE_DISCONNECTED',
'data': { 'server': 'SpiceBasicInfo',
- 'client': 'SpiceBasicInfo' } }
+ 'client': 'SpiceBasicInfo' },
+ 'if': 'defined(CONFIG_SPICE)' }
##
# @SPICE_MIGRATE_COMPLETED:
@@ -350,7 +359,8 @@
# "event": "SPICE_MIGRATE_COMPLETED" }
#
##
-{ 'event': 'SPICE_MIGRATE_COMPLETED' }
+{ 'event': 'SPICE_MIGRATE_COMPLETED',
+ 'if': 'defined(CONFIG_SPICE)' }
##
# == VNC
diff --git a/monitor.c b/monitor.c
index 0730a27172..0c2fbd32d5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1175,9 +1175,6 @@ static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
*/
static void qmp_unregister_commands_hack(void)
{
-#ifndef CONFIG_SPICE
- qmp_unregister_command(&qmp_commands, "query-spice");
-#endif
#ifndef CONFIG_REPLICATION
qmp_unregister_command(&qmp_commands, "xen-set-replication");
qmp_unregister_command(&qmp_commands, "query-xen-replication-status");
diff --git a/qmp.c b/qmp.c
index bc5537b221..162bc2e30d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -129,22 +129,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
}
}
-#ifndef CONFIG_SPICE
-/*
- * qmp_unregister_commands_hack() ensures that QMP command query-spice
- * exists only #ifdef CONFIG_SPICE. Necessary for an accurate
- * query-commands result. However, the QAPI schema is blissfully
- * unaware of that, and the QAPI code generator happily generates a
- * dead qmp_marshal_query_spice() that calls qmp_query_spice().
- * Provide it one, or else linking fails. FIXME Educate the QAPI
- * schema on CONFIG_SPICE.
- */
-SpiceInfo *qmp_query_spice(Error **errp)
-{
- abort();
-};
-#endif
-
void qmp_exit_preconfig(Error **errp)
{
if (!runstate_check(RUN_STATE_PRECONFIG)) {
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/15] tests
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 02/15] tests Marc-André Lureau
@ 2018-06-27 16:39 ` Marc-André Lureau
2018-06-28 15:35 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-27 16:39 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marc-André Lureau, Blake, Eric, Armbruster,
Markus, Dr. David Alan Gilbert, Gerd Hoffmann, Michael Roth
doh.. this had to be squashed with previous patch!
On Wed, Jun 27, 2018 at 6:35 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> ---
> tests/test-qmp-cmds.c | 8 +++++++-
> tests/qapi-schema/qapi-schema-test.json | 6 ++++++
> tests/qapi-schema/qapi-schema-test.out | 6 +++++-
> 3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index eaf30b7a0f..840530b84c 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -13,11 +13,17 @@
> static QmpCommandList qmp_commands;
>
> /* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
> -void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
> +UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
> {
> + return NULL;
> }
> /* #endif */
>
> +UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
> +{
> + return NULL;
> +}
> +
> void qmp_user_def_cmd(Error **errp)
> {
> }
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index ec0fea804f..16209b57b3 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -56,6 +56,9 @@
> 'data': { 'string0': 'str',
> 'dict1': 'UserDefTwoDict' } }
>
> +{ 'struct': 'UserDefThree',
> + 'data': { 'string0': 'str' } }
> +
> # dummy struct to force generation of array types not otherwise mentioned
> { 'struct': 'ForceArrays',
> 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
> @@ -209,7 +212,10 @@
> 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>
> { 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
> + 'returns': 'UserDefThree',
> 'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
>
> +{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
> +
> { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
> 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 34b6d546f3..ed25e5b60c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -36,6 +36,8 @@ object UserDefTwoDict
> object UserDefTwo
> member string0: str optional=False
> member dict1: UserDefTwoDict optional=False
> +object UserDefThree
> + member string0: str optional=False
> object ForceArrays
> member unused1: UserDefOneList optional=False
> member unused2: UserDefTwoList optional=False
> @@ -249,7 +251,9 @@ alternate TestIfAlternate
> case bar: TestStruct
> object q_obj_TestIfCmd-arg
> member foo: TestIfStruct optional=False
> -command TestIfCmd q_obj_TestIfCmd-arg -> None
> +command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
> + gen=True success_response=True boxed=False oob=False preconfig=False
> +command TestCmdReturnDefThree None -> UserDefThree
> gen=True success_response=True boxed=False oob=False preconfig=False
> object q_obj_TestIfEvent-arg
> member foo: TestIfStruct optional=False
> --
> 2.18.0.rc1
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions Marc-André Lureau
@ 2018-06-28 14:57 ` Markus Armbruster
2018-06-28 15:34 ` Marc-André Lureau
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-06-28 14:57 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Accept 'if' key in top-level elements, accepted as string or list of
> string type. The following patches will modify the test visitor to
> check the value is correctly saved, and generate #if/#endif code (as a
> single #if/endif line or a series for a list).
>
> Example of 'if' key:
> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> 'if': 'defined(TEST_IF_STRUCT)' }
>
> The generated code is for now *unconditional*. Later patches generate
> the conditionals.
>
> A following patch for qapi-code-gen.txt will provide more complete
> documentation for 'if' usage.
This paragraph looks obsolete now, because...
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi/common.py | 35 ++++++++++++++++++++----
> tests/test-qmp-cmds.c | 6 ++++
> docs/devel/qapi-code-gen.txt | 22 +++++++++++++++
... you update documentation right in this patch (I like that).
[...]
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 88a70e4d45..7af60b48f3 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -739,6 +739,28 @@ Example: Red Hat, Inc. controls redhat.com, and may therefore add a
> downstream command __com.redhat_drive-mirror.
>
>
> +=== Configuring the schema ===
> +
> +Top-level QAPI expressions.
This sentence no verb :) I suspect an editing accident ate "can take an
'if' key".
> The value must be a string or a list of
> +string.
s/string/strings/
> The corresponding generated code will then guard the inclusion
> +of that member in the larger struct or function with #if IFCOND
> +(or several #if lines for a list), where IFCOND is the value of the
> +'if' key.
"that member" makes little sense; top-level expressions aren't members.
> +
> +'struct', 'enum', 'union', 'alternate', 'command' and 'event'
> +top-level QAPI expressions can take an 'if' keyword like:
> +
> +{ 'struct': 'IfStruct', 'data': { 'foo': 'int' },
> + 'if': 'defined(IFCOND)' }
Repetitive.
Moreover, I'd like to see the example spell out how conditions affect
generated code.
Here's my try:
=== Configuring the schema ===
The 'struct', 'enum', 'union', 'alternate', 'command' and 'event'
top-level expressions can take an 'if' key. Its value must be a string
or a list of strings. A string is shorthand for a list containing just
that string. The code generated for the top-level expression will then
be guarded by #if COND for each COND in the list.
Example: a conditional struct
{ 'struct': 'IfStruct', 'data': { 'foo': 'int' },
'if': ['defined(CONFIG_FOO)', 'defined(HAVE_BAR)'] }
gets its generated code guarded like this:
#if defined(CONFIG_FOO)
#if defined(HAVE_BAR)
... generated code ...
#endif /* defined(HAVE_BAR) */
#endif /* defined(CONFIG_FOO) */
> +
> +Please note that you are responsible to ensure that the C code will
> +compile with an arbitrary combination of conditions, since the
> +generators are unable to check it at this point.
> +
> +The presence of 'if' keys in the schema is reflected through to the
> +introspection output depending on the build configuration.
> +
> +
> == Client JSON Protocol introspection ==
>
> Clients of a Client JSON Protocol commonly need to figure out what
[...]
I'm happy to apply these proposals without a respin, if you like them.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions
2018-06-28 14:57 ` Markus Armbruster
@ 2018-06-28 15:34 ` Marc-André Lureau
0 siblings, 0 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-06-28 15:34 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, QEMU, Gerd Hoffmann, Paolo Bonzini, Dr. David Alan Gilbert
On Thu, Jun 28, 2018 at 4:57 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Accept 'if' key in top-level elements, accepted as string or list of
>> string type. The following patches will modify the test visitor to
>> check the value is correctly saved, and generate #if/#endif code (as a
>> single #if/endif line or a series for a list).
>>
>> Example of 'if' key:
>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>> 'if': 'defined(TEST_IF_STRUCT)' }
>>
>> The generated code is for now *unconditional*. Later patches generate
>> the conditionals.
>>
>> A following patch for qapi-code-gen.txt will provide more complete
>> documentation for 'if' usage.
>
> This paragraph looks obsolete now, because...
>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/qapi/common.py | 35 ++++++++++++++++++++----
>> tests/test-qmp-cmds.c | 6 ++++
>> docs/devel/qapi-code-gen.txt | 22 +++++++++++++++
>
> ... you update documentation right in this patch (I like that).
>
> [...]
>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> index 88a70e4d45..7af60b48f3 100644
>> --- a/docs/devel/qapi-code-gen.txt
>> +++ b/docs/devel/qapi-code-gen.txt
>> @@ -739,6 +739,28 @@ Example: Red Hat, Inc. controls redhat.com, and may therefore add a
>> downstream command __com.redhat_drive-mirror.
>>
>>
>> +=== Configuring the schema ===
>> +
>> +Top-level QAPI expressions.
>
> This sentence no verb :) I suspect an editing accident ate "can take an
> 'if' key".
>
>> The value must be a string or a list of
>> +string.
>
> s/string/strings/
>
>> The corresponding generated code will then guard the inclusion
>> +of that member in the larger struct or function with #if IFCOND
>> +(or several #if lines for a list), where IFCOND is the value of the
>> +'if' key.
>
> "that member" makes little sense; top-level expressions aren't members.
>
>> +
>> +'struct', 'enum', 'union', 'alternate', 'command' and 'event'
>> +top-level QAPI expressions can take an 'if' keyword like:
>> +
>> +{ 'struct': 'IfStruct', 'data': { 'foo': 'int' },
>> + 'if': 'defined(IFCOND)' }
>
> Repetitive.
>
> Moreover, I'd like to see the example spell out how conditions affect
> generated code.
>
> Here's my try:
>
> === Configuring the schema ===
>
> The 'struct', 'enum', 'union', 'alternate', 'command' and 'event'
> top-level expressions can take an 'if' key. Its value must be a string
> or a list of strings. A string is shorthand for a list containing just
> that string. The code generated for the top-level expression will then
> be guarded by #if COND for each COND in the list.
>
> Example: a conditional struct
>
> { 'struct': 'IfStruct', 'data': { 'foo': 'int' },
> 'if': ['defined(CONFIG_FOO)', 'defined(HAVE_BAR)'] }
>
> gets its generated code guarded like this:
>
> #if defined(CONFIG_FOO)
> #if defined(HAVE_BAR)
> ... generated code ...
> #endif /* defined(HAVE_BAR) */
> #endif /* defined(CONFIG_FOO) */
>
>> +
>> +Please note that you are responsible to ensure that the C code will
>> +compile with an arbitrary combination of conditions, since the
>> +generators are unable to check it at this point.
>> +
>> +The presence of 'if' keys in the schema is reflected through to the
>> +introspection output depending on the build configuration.
>> +
>> +
>> == Client JSON Protocol introspection ==
>>
>> Clients of a Client JSON Protocol commonly need to figure out what
> [...]
>
> I'm happy to apply these proposals without a respin, if you like them.
>
Looks good to me,
(with 02/15 squashed)
thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/15] tests
2018-06-27 16:39 ` Marc-André Lureau
@ 2018-06-28 15:35 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-06-28 15:35 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Armbruster, Markus,
Dr. David Alan Gilbert, Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> doh.. this had to be squashed with previous patch!
Happy to do that when I apply.
> On Wed, Jun 27, 2018 at 6:35 PM, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> ---
>> tests/test-qmp-cmds.c | 8 +++++++-
>> tests/qapi-schema/qapi-schema-test.json | 6 ++++++
>> tests/qapi-schema/qapi-schema-test.out | 6 +++++-
>> 3 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
>> index eaf30b7a0f..840530b84c 100644
>> --- a/tests/test-qmp-cmds.c
>> +++ b/tests/test-qmp-cmds.c
>> @@ -13,11 +13,17 @@
>> static QmpCommandList qmp_commands;
>>
>> /* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
>> -void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
>> +UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
>> {
>> + return NULL;
>> }
>> /* #endif */
>>
>> +UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
>> +{
>> + return NULL;
>> +}
>> +
>> void qmp_user_def_cmd(Error **errp)
>> {
>> }
>> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>> index ec0fea804f..16209b57b3 100644
>> --- a/tests/qapi-schema/qapi-schema-test.json
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -56,6 +56,9 @@
>> 'data': { 'string0': 'str',
>> 'dict1': 'UserDefTwoDict' } }
>>
>> +{ 'struct': 'UserDefThree',
>> + 'data': { 'string0': 'str' } }
>> +
>> # dummy struct to force generation of array types not otherwise mentioned
>> { 'struct': 'ForceArrays',
>> 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
>> @@ -209,7 +212,10 @@
>> 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>>
>> { 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
>> + 'returns': 'UserDefThree',
>> 'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
>>
>> +{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
>> +
>> { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
>> 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
>> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
>> index 34b6d546f3..ed25e5b60c 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -36,6 +36,8 @@ object UserDefTwoDict
>> object UserDefTwo
>> member string0: str optional=False
>> member dict1: UserDefTwoDict optional=False
>> +object UserDefThree
>> + member string0: str optional=False
>> object ForceArrays
>> member unused1: UserDefOneList optional=False
>> member unused2: UserDefTwoList optional=False
>> @@ -249,7 +251,9 @@ alternate TestIfAlternate
>> case bar: TestStruct
>> object q_obj_TestIfCmd-arg
>> member foo: TestIfStruct optional=False
>> -command TestIfCmd q_obj_TestIfCmd-arg -> None
>> +command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
>> + gen=True success_response=True boxed=False oob=False preconfig=False
>> +command TestCmdReturnDefThree None -> UserDefThree
>> gen=True success_response=True boxed=False oob=False preconfig=False
>> object q_obj_TestIfEvent-arg
>> member foo: TestIfStruct optional=False
>> --
>> 2.18.0.rc1
>>
This provides the test coverage I asked for in review of v5's PATCH 11.
Thanks!
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check()
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check() Marc-André Lureau
@ 2018-06-28 17:45 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-06-28 17:45 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> We commonly initialize attributes to None in .init(), then set their
> real value in .check(). Accessing the attribute before .check()
> yields None. If we're lucky, the code that accesses the attribute
> prematurely chokes on None.
>
> It won't for .ifcond, because None is a legitimate value.
>
> Leave the ifcond attribute undefined until check().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Note to self: consider squashing into previous patch.
> ---
> scripts/qapi/common.py | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 4f4014b387..46e33e23e4 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1021,13 +1021,19 @@ class QAPISchemaEntity(object):
> # such place).
> self.info = info
> self.doc = doc
> - self.ifcond = listify_cond(ifcond)
> + self._ifcond = ifcond # self.ifcond is set only after .check()
>
> def c_name(self):
> return c_name(self.name)
>
> def check(self, schema):
> - pass
> + if isinstance(self._ifcond, QAPISchemaType):
> + # inherit the condition from a type
> + typ = self._ifcond
> + typ.check(schema)
> + self.ifcond = typ.ifcond
> + else:
> + self.ifcond = listify_cond(self._ifcond)
>
> def is_implicit(self):
> return not self.info
One of the questions I had on v5 of this patch is still open. I asked
whether we can restrict the inheritance feature to array elements. You
pointed to implicit object types. I asked for an example (because I
couldn't figure out what you meant). Please reply in that thread.
[...]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods Marc-André Lureau
@ 2018-06-28 17:48 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-06-28 17:48 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Dr. David Alan Gilbert, Gerd Hoffmann,
Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Modify the test visitor to check correct passing of values.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi/commands.py | 2 +-
> scripts/qapi/common.py | 31 ++++++++++++++------------
> scripts/qapi/doc.py | 10 ++++-----
> scripts/qapi/events.py | 2 +-
> scripts/qapi/introspect.py | 12 +++++-----
> scripts/qapi/types.py | 8 +++----
> scripts/qapi/visit.py | 8 +++----
> tests/qapi-schema/qapi-schema-test.out | 9 ++++++++
> tests/qapi-schema/test-qapi.py | 19 +++++++++++-----
> 9 files changed, 61 insertions(+), 40 deletions(-)
> mode change 100644 => 100755 scripts/qapi/doc.py
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 3b0867c14f..dcc03c7859 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -277,7 +277,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
> c_prefix=c_name(self._prefix, protect=False)))
> genc.add(gen_registry(self._regy, self._prefix))
>
> - def visit_command(self, name, info, arg_type, ret_type, gen,
> + def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> if not gen:
> return
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 46e33e23e4..feae646e09 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1062,26 +1062,26 @@ class QAPISchemaVisitor(object):
> def visit_builtin_type(self, name, info, json_type):
> pass
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> pass
>
> - def visit_array_type(self, name, info, element_type):
> + def visit_array_type(self, name, info, ifcond, element_type):
> pass
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, ifcond, base, members, variants):
> pass
>
> - def visit_object_type_flat(self, name, info, members, variants):
> + def visit_object_type_flat(self, name, info, ifcond, members, variants):
> pass
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> pass
>
> - def visit_command(self, name, info, arg_type, ret_type, gen,
> + def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> pass
>
> - def visit_event(self, name, info, arg_type, boxed):
> + def visit_event(self, name, info, ifcond, arg_type, boxed):
> pass
>
>
> @@ -1191,7 +1191,7 @@ class QAPISchemaEnumType(QAPISchemaType):
> return 'string'
>
> def visit(self, visitor):
> - visitor.visit_enum_type(self.name, self.info,
> + visitor.visit_enum_type(self.name, self.info, self.ifcond,
> self.member_names(), self.prefix)
>
>
> @@ -1225,7 +1225,8 @@ class QAPISchemaArrayType(QAPISchemaType):
> return 'array of ' + elt_doc_type
>
> def visit(self, visitor):
> - visitor.visit_array_type(self.name, self.info, self.element_type)
> + visitor.visit_array_type(self.name, self.info, self.ifcond,
> + self.element_type)
>
>
> class QAPISchemaObjectType(QAPISchemaType):
> @@ -1307,9 +1308,9 @@ class QAPISchemaObjectType(QAPISchemaType):
> return 'object'
>
> def visit(self, visitor):
> - visitor.visit_object_type(self.name, self.info,
> + visitor.visit_object_type(self.name, self.info, self.ifcond,
> self.base, self.local_members, self.variants)
> - visitor.visit_object_type_flat(self.name, self.info,
> + visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
> self.members, self.variants)
>
>
> @@ -1462,7 +1463,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
> return 'value'
>
> def visit(self, visitor):
> - visitor.visit_alternate_type(self.name, self.info, self.variants)
> + visitor.visit_alternate_type(self.name, self.info, self.ifcond,
> + self.variants)
>
> def is_empty(self):
> return False
> @@ -1505,7 +1507,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
> assert isinstance(self.ret_type, QAPISchemaType)
>
> def visit(self, visitor):
> - visitor.visit_command(self.name, self.info,
> + visitor.visit_command(self.name, self.info, self.ifcond,
> self.arg_type, self.ret_type,
> self.gen, self.success_response,
> self.boxed, self.allow_oob,
> @@ -1538,7 +1540,8 @@ class QAPISchemaEvent(QAPISchemaEntity):
> raise QAPISemError(self.info, "Use of 'boxed' requires 'data'")
>
> def visit(self, visitor):
> - visitor.visit_event(self.name, self.info, self.arg_type, self.boxed)
> + visitor.visit_event(self.name, self.info, self.ifcond,
> + self.arg_type, self.boxed)
>
>
> class QAPISchema(object):
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> old mode 100644
> new mode 100755
> index b5630844f9..4db6674dc3
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -204,14 +204,14 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> def write(self, output_dir):
> self._gen.write(output_dir, self._prefix + 'qapi-doc.texi')
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> doc = self.cur_doc
> self._gen.add(TYPE_FMT(type='Enum',
> name=doc.symbol,
> body=texi_entity(doc, 'Values',
> member_func=texi_enum_value)))
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, ifcond, base, members, variants):
> doc = self.cur_doc
> if base and base.is_implicit():
> base = None
> @@ -220,13 +220,13 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> body=texi_entity(doc, 'Members',
> base, variants)))
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> doc = self.cur_doc
> self._gen.add(TYPE_FMT(type='Alternate',
> name=doc.symbol,
> body=texi_entity(doc, 'Members')))
>
> - def visit_command(self, name, info, arg_type, ret_type, gen,
> + def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> doc = self.cur_doc
> if boxed:
> @@ -240,7 +240,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> name=doc.symbol,
> body=body))
>
> - def visit_event(self, name, info, arg_type, boxed):
> + def visit_event(self, name, info, ifcond, arg_type, boxed):
> doc = self.cur_doc
> self._gen.add(MSG_FMT(type='Event',
> name=doc.symbol,
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 5657524688..0a1afac134 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -184,7 +184,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
> genh.add(gen_enum(self._enum_name, self._event_names))
> genc.add(gen_enum_lookup(self._enum_name, self._event_names))
>
> - def visit_event(self, name, info, arg_type, boxed):
> + def visit_event(self, name, info, ifcond, arg_type, boxed):
> self._genh.add(gen_event_send_decl(name, arg_type, boxed))
> self._genc.add(gen_event_send(name, arg_type, boxed, self._enum_name))
> self._event_names.append(name)
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 6ad198ae5b..245cfdfb65 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -149,26 +149,26 @@ const QLitObject %(c_name)s = %(c_string)s;
> def visit_builtin_type(self, name, info, json_type):
> self._gen_qlit(name, 'builtin', {'json-type': json_type})
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> self._gen_qlit(name, 'enum', {'values': values})
>
> - def visit_array_type(self, name, info, element_type):
> + def visit_array_type(self, name, info, ifcond, element_type):
> element = self._use_type(element_type)
> self._gen_qlit('[' + element + ']', 'array', {'element-type': element})
>
> - def visit_object_type_flat(self, name, info, members, variants):
> + def visit_object_type_flat(self, name, info, ifcond, members, variants):
> obj = {'members': [self._gen_member(m) for m in members]}
> if variants:
> obj.update(self._gen_variants(variants.tag_member.name,
> variants.variants))
> self._gen_qlit(name, 'object', obj)
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> self._gen_qlit(name, 'alternate',
> {'members': [{'type': self._use_type(m.type)}
> for m in variants.variants]})
>
> - def visit_command(self, name, info, arg_type, ret_type, gen,
> + def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> @@ -178,7 +178,7 @@ const QLitObject %(c_name)s = %(c_string)s;
> 'allow-oob': allow_oob,
> 'allow-preconfig': allow_preconfig})
>
> - def visit_event(self, name, info, arg_type, boxed):
> + def visit_event(self, name, info, ifcond, arg_type, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)})
>
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index a599352e59..659075f884 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -208,16 +208,16 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> self._genh.add(gen_type_cleanup_decl(name))
> self._genc.add(gen_type_cleanup(name))
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> self._genh.preamble_add(gen_enum(name, values, prefix))
> self._genc.add(gen_enum_lookup(name, values, prefix))
>
> - def visit_array_type(self, name, info, element_type):
> + def visit_array_type(self, name, info, ifcond, element_type):
> self._genh.preamble_add(gen_fwd_object_or_array(name))
> self._genh.add(gen_array(name, element_type))
> self._gen_type_cleanup(name)
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, ifcond, base, members, variants):
> # Nothing to do for the special empty builtin
> if name == 'q_empty':
> return
> @@ -231,7 +231,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> # implicit types won't be directly allocated/freed
> self._gen_type_cleanup(name)
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> self._genh.preamble_add(gen_fwd_object_or_array(name))
> self._genh.add(gen_object(name, None,
> [variants.tag_member], variants))
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index bdcafb64ee..34fe1ef5eb 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -310,15 +310,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
> ''',
> types=types))
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> self._genh.add(gen_visit_decl(name, scalar=True))
> self._genc.add(gen_visit_enum(name))
>
> - def visit_array_type(self, name, info, element_type):
> + def visit_array_type(self, name, info, ifcond, element_type):
> self._genh.add(gen_visit_decl(name))
> self._genc.add(gen_visit_list(name, element_type))
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, ifcond, base, members, variants):
> # Nothing to do for the special empty builtin
> if name == 'q_empty':
> return
> @@ -331,7 +331,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
> self._genh.add(gen_visit_decl(name))
> self._genc.add(gen_visit_object(name, base, members, variants))
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> self._genh.add(gen_visit_decl(name))
> self._genc.add(gen_visit_alternate(name, variants))
>
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index ed25e5b60c..0da92455da 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -237,25 +237,34 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio
> gen=True success_response=True boxed=False oob=False preconfig=False
> object TestIfStruct
> member foo: int optional=False
> + if ['defined(TEST_IF_STRUCT)']
> enum TestIfEnum ['foo', 'bar']
> + if ['defined(TEST_IF_ENUM)']
> object q_obj_TestStruct-wrapper
> member data: TestStruct optional=False
> enum TestIfUnionKind ['foo']
> + if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> object TestIfUnion
> member type: TestIfUnionKind optional=False
> tag type
> case foo: q_obj_TestStruct-wrapper
> + if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> alternate TestIfAlternate
> tag type
> case foo: int
> case bar: TestStruct
> + if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
> object q_obj_TestIfCmd-arg
> member foo: TestIfStruct optional=False
> + if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
> gen=True success_response=True boxed=False oob=False preconfig=False
> + if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> command TestCmdReturnDefThree None -> UserDefThree
> gen=True success_response=True boxed=False oob=False preconfig=False
> object q_obj_TestIfEvent-arg
> member foo: TestIfStruct optional=False
> + if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> event TestIfEvent q_obj_TestIfEvent-arg
> boxed=False
> + if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 4512a41504..43aaf5a20b 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -23,12 +23,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> def visit_include(self, name, info):
> print('include %s' % name)
>
> - def visit_enum_type(self, name, info, values, prefix):
> + def visit_enum_type(self, name, info, ifcond, values, prefix):
> print('enum %s %s' % (name, values))
> if prefix:
> print(' prefix %s' % prefix)
> + self._print_if(ifcond)
>
> - def visit_object_type(self, name, info, base, members, variants):
> + def visit_object_type(self, name, info, ifcond, base, members, variants):
> print('object %s' % name)
> if base:
> print(' base %s' % base.name)
> @@ -36,21 +37,25 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> print(' member %s: %s optional=%s' % \
> (m.name, m.type.name, m.optional))
> self._print_variants(variants)
> + self._print_if(ifcond)
>
> - def visit_alternate_type(self, name, info, variants):
> + def visit_alternate_type(self, name, info, ifcond, variants):
> print('alternate %s' % name)
> self._print_variants(variants)
> + self._print_if(ifcond)
>
> - def visit_command(self, name, info, arg_type, ret_type, gen,
> + def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> print('command %s %s -> %s' % \
> (name, arg_type and arg_type.name, ret_type and ret_type.name))
> print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s' % \
> (gen, success_response, boxed, allow_oob, allow_preconfig))
> + self._print_if(ifcond)
>
> - def visit_event(self, name, info, arg_type, boxed):
> + def visit_event(self, name, info, ifcond, arg_type, boxed):
> print('event %s %s' % (name, arg_type and arg_type.name))
> print(' boxed=%s' % boxed)
> + self._print_if(ifcond)
>
> @staticmethod
> def _print_variants(variants):
> @@ -59,6 +64,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> for v in variants.variants:
> print(' case %s: %s' % (v.name, v.type.name))
>
> + @staticmethod
> + def _print_if(ifcond, indent=4):
> + if ifcond:
> + print('%sif %s' % (' ' * indent, ifcond))
>
pycodestyle points out:
tests/qapi-schema/test-qapi.py:72:1: E305 expected 2 blank lines after class or function definition, found 1
Can touch up when I apply.
> try:
> schema = QAPISchema(sys.argv[1])
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers Marc-André Lureau
@ 2018-07-03 11:53 ` Markus Armbruster
2018-07-03 12:35 ` Marc-André Lureau
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 11:53 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Add helpers to wrap generated code with #if/#endif lines.
>
> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
> inherit from it, for full C files with copyright headers etc.
It's called QAPIGenCCode now. Can touch up when I apply, along with
another instance below.
Leaves the reader wondering *why* you need to splice QAPIGenCCode
between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
Providing the class now reduces code churn, but you should explain why.
Perhaps:
A later patch wants to use QAPIGen for generating C snippets rather
than full C files with copyright headers etc. Splice in class
QAPIGenCCode between QAPIGen and QAPIGenC.
> Add a 'with' statement context manager that will be used to wrap
> generator visitor methods. The manager will check if code was
> generated before adding #if/#endif lines on QAPIGenCSnippet
> objects. Used in the following patches.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 97 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 1b56065a80..44eaf25850 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,6 +12,7 @@
> # See the COPYING file in the top-level directory.
>
> from __future__ import print_function
> +from contextlib import contextmanager
> import errno
> import os
> import re
> @@ -1974,6 +1975,40 @@ def guardend(name):
> name=guardname(name))
>
>
> +def gen_if(ifcond):
> + ret = ''
> + for ifc in ifcond:
> + ret += mcgen('''
> +#if %(cond)s
> +''', cond=ifc)
> + return ret
> +
> +
> +def gen_endif(ifcond):
> + ret = ''
> + for ifc in reversed(ifcond):
> + ret += mcgen('''
> +#endif /* %(cond)s */
> +''', cond=ifc)
> + return ret
> +
> +
> +def wrap_ifcond(ifcond, before, after):
Looks like a helper method. Name it _wrap_ifcond?
> + if ifcond is None or before == after:
> + return after
The before == after part suppresses empty conditionals. Suggest
return after # suppress empty #if ... #endif
The ifcond is None part is suppresses "non-conditionals". How can this
happen? If it can't, let's drop this part.
Another non-conditional is []. See below.
> +
> + assert after.startswith(before)
> + out = before
> + added = after[len(before):]
> + if added[0] == '\n':
> + out += '\n'
> + added = added[1:]
The conditional adjusts vertical space around #if. This might need
tweaking, but let's not worry about that now.
> + out += gen_if(ifcond)
> + out += added
> + out += gen_endif(ifcond)
There's no similar adjustment for #endif. Again, let's not worry about
that now.
> + return out
> +
> +
Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
after) returns after. Okay.
> def gen_enum_lookup(name, values, prefix=None):
> ret = mcgen('''
>
> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
> def __init__(self):
> self._preamble = ''
> self._body = ''
> + self._start_if = None
>
> def preamble_add(self, text):
> self._preamble += text
> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
> def add(self, text):
> self._body += text
>
> + def start_if(self, ifcond):
> + assert self._start_if is None
> +
Let's drop this blank line.
> + self._start_if = (ifcond, self._body, self._preamble)
It's now obvious that you can't nest .start_if() ... .endif(). Good.
> +
> + def _wrap_ifcond(self):
> + pass
> +
> + def end_if(self):
> + assert self._start_if
> +
Let's drop this blank line.
> + self._wrap_ifcond()
> + self._start_if = None
> +
> + def get_content(self, fname=None):
> + assert self._start_if is None
> + return (self._top(fname) + self._preamble + self._body
> + + self._bottom(fname))
> +
> def _top(self, fname):
> return ''
>
Note for later: all this code has no effect unless ._wrap_ifcond() is
overridden.
> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
> f = open(fd, 'r+', encoding='utf-8')
> else:
> f = os.fdopen(fd, 'r+')
> - text = (self._top(fname) + self._preamble + self._body
> - + self._bottom(fname))
> + text = self.get_content(fname)
> oldtext = f.read(len(text) + 1)
> if text != oldtext:
> f.seek(0)
> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
> f.close()
>
>
> -class QAPIGenC(QAPIGen):
> +@contextmanager
> +def ifcontext(ifcond, *args):
> + """A 'with' statement context manager to wrap with start_if()/end_if()
>
> - def __init__(self, blurb, pydoc):
> + *args: variable length argument list of QAPIGen
Perhaps: any number of QAPIGen objects
> +
> + Example::
> +
> + with ifcontext(ifcond, self._genh, self._genc):
> + modify self._genh and self._genc ...
> +
> + Is equivalent to calling::
> +
> + self._genh.start_if(ifcond)
> + self._genc.start_if(ifcond)
> + modify self._genh and self._genc ...
> + self._genh.end_if()
> + self._genc.end_if()
> +
Can we drop this blank line?
> + """
> + for arg in args:
> + arg.start_if(ifcond)
> + yield
> + for arg in args:
> + arg.end_if()
> +
> +
> +class QAPIGenCCode(QAPIGen):
> +
> + def __init__(self):
> QAPIGen.__init__(self)
> +
> + def _wrap_ifcond(self):
> + self._body = wrap_ifcond(self._start_if[0],
> + self._start_if[1], self._body)
> + self._preamble = wrap_ifcond(self._start_if[0],
> + self._start_if[2], self._preamble)
Can you explain why you put the machinery for conditionals in QAPIGen
and not QAPIGenCCode?
> +
> +
> +class QAPIGenC(QAPIGenCCode):
> +
> + def __init__(self, blurb, pydoc):
> + QAPIGenCCode.__init__(self)
> self._blurb = blurb
> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
> re.MULTILINE))
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit Marc-André Lureau
@ 2018-07-03 12:09 ` Markus Armbruster
2018-07-03 13:11 ` Marc-André Lureau
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 12:09 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Dr. David Alan Gilbert, Gerd Hoffmann,
Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> The generator will take (obj, condition) tuples to wrap generated QLit
> objects for 'obj' with #if/#endif conditions.
>
> This commit adds 'ifcond' condition to top-level QLit objects.
>
> See generated tests/test-qmp-introspect.c. Example diff after this patch:
>
> --- before 2018-01-08 11:55:24.757083654 +0100
> +++ tests/test-qmp-introspect.c 2018-01-08 13:08:44.477641629 +0100
> @@ -51,6 +51,8 @@
> { "name", QLIT_QSTR("EVENT_F"), },
> {}
> })),
> +#if defined(TEST_IF_CMD)
> +#if defined(TEST_IF_STRUCT)
> QLIT_QDICT(((QLitDictEntry[]) {
> { "arg-type", QLIT_QSTR("5"), },
> { "meta-type", QLIT_QSTR("command"), },
> @@ -58,12 +60,16 @@
> { "ret-type", QLIT_QSTR("0"), },
> {}
> })),
> +#endif /* defined(TEST_IF_STRUCT) */
> +#endif /* defined(TEST_IF_CMD) */
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/introspect.py | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index bd7e1219be..71d4a779ce 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -18,6 +18,15 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
> def indent(level):
> return level * 4 * ' '
>
> + if isinstance(obj, tuple):
> + ifobj, ifcond = obj
> + ret = gen_if(ifcond)
> + ret += to_qlit(ifobj, level)
> + endif = gen_endif(ifcond)
> + if endif:
> + ret += '\n' + endif
> + return ret
> +
> ret = ''
> if not suppress_first_indent:
> ret += indent(level)
@obj represents a JSON object. It consists of dictionaries, lists,
strings, booleans and None. Numbers aren't implemented. Your patch
splices in tuples to represent conditionals at any level.
So far, tuples occur only as elements of the outermost list (see
._gen_qlit() below), but to_qlit() supports them anywhere. I figure
that will be needed later. Can you point me to such a later use?
I'm asking because if there is one, the whole thing is a bit of a hack,
but at least it's a simple hack. If not, then I'd like to look for a
solution that feels less hackish, possibly in a follow-up patch.
> @@ -26,7 +35,7 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
> elif isinstance(obj, str):
> ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
> elif isinstance(obj, list):
> - elts = [to_qlit(elt, level + 1)
> + elts = [to_qlit(elt, level + 1).strip('\n')
> for elt in obj]
> elts.append(indent(level + 1) + "{}")
> ret += 'QLIT_QLIST(((QLitObject[]) {\n'
The .strip('\n') looks odd. Reminds me of the asymmetry I noted in
PATCH 08: you take care to adjust vertical space around #if there, but
not around #endif. Let's not worry about it now.
> @@ -128,12 +137,12 @@ const QLitObject %(c_name)s = %(c_string)s;
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> - def _gen_qlit(self, name, mtype, obj):
> + def _gen_qlit(self, name, mtype, obj, ifcond):
> if mtype not in ('command', 'event', 'builtin', 'array'):
> name = self._name(name)
> obj['name'] = name
> obj['meta-type'] = mtype
> - self._qlits.append(obj)
> + self._qlits.append((obj, ifcond))
This is where we produce ._qlits. It's also the only place where we
create tuples. Thus, all elements of ._qlits are tuples, and no tuples
occur at deeper levels.
>
> def _gen_member(self, member):
> ret = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -149,26 +158,27 @@ const QLitObject %(c_name)s = %(c_string)s;
> return {'case': variant.name, 'type': self._use_type(variant.type)}
>
> def visit_builtin_type(self, name, info, json_type):
> - self._gen_qlit(name, 'builtin', {'json-type': json_type})
> + self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
>
> def visit_enum_type(self, name, info, ifcond, values, prefix):
> - self._gen_qlit(name, 'enum', {'values': values})
> + self._gen_qlit(name, 'enum', {'values': values}, ifcond)
>
> def visit_array_type(self, name, info, ifcond, element_type):
> element = self._use_type(element_type)
> - self._gen_qlit('[' + element + ']', 'array', {'element-type': element})
> + self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
> + ifcond)
>
> def visit_object_type_flat(self, name, info, ifcond, members, variants):
> obj = {'members': [self._gen_member(m) for m in members]}
> if variants:
> obj.update(self._gen_variants(variants.tag_member.name,
> variants.variants))
> - self._gen_qlit(name, 'object', obj)
> + self._gen_qlit(name, 'object', obj, ifcond)
>
> def visit_alternate_type(self, name, info, ifcond, variants):
> self._gen_qlit(name, 'alternate',
> {'members': [{'type': self._use_type(m.type)}
> - for m in variants.variants]})
> + for m in variants.variants]}, ifcond)
>
> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> @@ -178,11 +188,12 @@ const QLitObject %(c_name)s = %(c_string)s;
> {'arg-type': self._use_type(arg_type),
> 'ret-type': self._use_type(ret_type),
> 'allow-oob': allow_oob,
> - 'allow-preconfig': allow_preconfig})
> + 'allow-preconfig': allow_preconfig}, ifcond)
>
> def visit_event(self, name, info, ifcond, arg_type, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> - self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)})
> + self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)},
> + ifcond)
>
>
> def gen_introspect(schema, output_dir, prefix, opt_unmask):
I still prefer the approach I explored in
[PATCH RFC 09/14] qapi: Pass ifcond to visitors
[PATCH RFC 13/14] qapi-introspect: Add #if conditions to introspection value
because it separates the concerns. I'd love to complete my exploration
so we can compare apples to apples, but I'm willing to take this to help
things move forward.
I'd like to get an answer to the question I asked in my first paragraph
before I give my R-by, though.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-07-03 11:53 ` Markus Armbruster
@ 2018-07-03 12:35 ` Marc-André Lureau
2018-07-03 13:37 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-07-03 12:35 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, QEMU, Gerd Hoffmann, Paolo Bonzini, Dr. David Alan Gilbert
Hi
On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Add helpers to wrap generated code with #if/#endif lines.
>>
>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>> inherit from it, for full C files with copyright headers etc.
>
> It's called QAPIGenCCode now. Can touch up when I apply, along with
> another instance below.
>
> Leaves the reader wondering *why* you need to splice QAPIGenCCode
> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
> Providing the class now reduces code churn, but you should explain why.
> Perhaps:
>
> A later patch wants to use QAPIGen for generating C snippets rather
> than full C files with copyright headers etc. Splice in class
> QAPIGenCCode between QAPIGen and QAPIGenC.
sure, thanks
>
>> Add a 'with' statement context manager that will be used to wrap
>> generator visitor methods. The manager will check if code was
>> generated before adding #if/#endif lines on QAPIGenCSnippet
>> objects. Used in the following patches.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 1b56065a80..44eaf25850 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -12,6 +12,7 @@
>> # See the COPYING file in the top-level directory.
>>
>> from __future__ import print_function
>> +from contextlib import contextmanager
>> import errno
>> import os
>> import re
>> @@ -1974,6 +1975,40 @@ def guardend(name):
>> name=guardname(name))
>>
>>
>> +def gen_if(ifcond):
>> + ret = ''
>> + for ifc in ifcond:
>> + ret += mcgen('''
>> +#if %(cond)s
>> +''', cond=ifc)
>> + return ret
>> +
>> +
>> +def gen_endif(ifcond):
>> + ret = ''
>> + for ifc in reversed(ifcond):
>> + ret += mcgen('''
>> +#endif /* %(cond)s */
>> +''', cond=ifc)
>> + return ret
>> +
>> +
>> +def wrap_ifcond(ifcond, before, after):
>
> Looks like a helper method. Name it _wrap_ifcond?
Not fond of functions with _. Iirc, it was suggested by a linter to
make it a top-level function. I don't mind if you change it on commit.
>
>> + if ifcond is None or before == after:
>> + return after
>
> The before == after part suppresses empty conditionals. Suggest
>
> return after # suppress empty #if ... #endif
>
> The ifcond is None part is suppresses "non-conditionals". How can this
> happen? If it can't, let's drop this part.
because the function can be called without additional checks on ifcond
is None. I don't see what would be the benefit in moving this to the
caller responsibility.
>
> Another non-conditional is []. See below.
>
>> +
>> + assert after.startswith(before)
>> + out = before
>> + added = after[len(before):]
>> + if added[0] == '\n':
>> + out += '\n'
>> + added = added[1:]
>
> The conditional adjusts vertical space around #if. This might need
> tweaking, but let's not worry about that now.
>
>> + out += gen_if(ifcond)
>> + out += added
>> + out += gen_endif(ifcond)
>
> There's no similar adjustment for #endif. Again, let's not worry about
> that now.
>
>> + return out
>> +
>> +
>
> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
> after) returns after. Okay.
>
>> def gen_enum_lookup(name, values, prefix=None):
>> ret = mcgen('''
>>
>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>> def __init__(self):
>> self._preamble = ''
>> self._body = ''
>> + self._start_if = None
>>
>> def preamble_add(self, text):
>> self._preamble += text
>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>> def add(self, text):
>> self._body += text
>>
>> + def start_if(self, ifcond):
>> + assert self._start_if is None
>> +
>
> Let's drop this blank line.
I prefer to have preconditions separated from the code, but ok
>
>> + self._start_if = (ifcond, self._body, self._preamble)
>
> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>
>> +
>> + def _wrap_ifcond(self):
>> + pass
>> +
>> + def end_if(self):
>> + assert self._start_if
>> +
>
> Let's drop this blank line.
>
>> + self._wrap_ifcond()
>> + self._start_if = None
>> +
>> + def get_content(self, fname=None):
>> + assert self._start_if is None
>> + return (self._top(fname) + self._preamble + self._body
>> + + self._bottom(fname))
>> +
>> def _top(self, fname):
>> return ''
>>
>
> Note for later: all this code has no effect unless ._wrap_ifcond() is
> overridden.
>
>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>> f = open(fd, 'r+', encoding='utf-8')
>> else:
>> f = os.fdopen(fd, 'r+')
>> - text = (self._top(fname) + self._preamble + self._body
>> - + self._bottom(fname))
>> + text = self.get_content(fname)
>> oldtext = f.read(len(text) + 1)
>> if text != oldtext:
>> f.seek(0)
>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>> f.close()
>>
>>
>> -class QAPIGenC(QAPIGen):
>> +@contextmanager
>> +def ifcontext(ifcond, *args):
>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>
>> - def __init__(self, blurb, pydoc):
>> + *args: variable length argument list of QAPIGen
>
> Perhaps: any number of QAPIGen objects
>
sure
>> +
>> + Example::
>> +
>> + with ifcontext(ifcond, self._genh, self._genc):
>> + modify self._genh and self._genc ...
>> +
>> + Is equivalent to calling::
>> +
>> + self._genh.start_if(ifcond)
>> + self._genc.start_if(ifcond)
>> + modify self._genh and self._genc ...
>> + self._genh.end_if()
>> + self._genc.end_if()
>> +
>
> Can we drop this blank line?
>
I guess we can, I haven't tested the rst formatting this rigorously.
Hopefully the linter does it.
>> + """
>> + for arg in args:
>> + arg.start_if(ifcond)
>> + yield
>> + for arg in args:
>> + arg.end_if()
>> +
>> +
>> +class QAPIGenCCode(QAPIGen):
>> +
>> + def __init__(self):
>> QAPIGen.__init__(self)
>> +
>> + def _wrap_ifcond(self):
>> + self._body = wrap_ifcond(self._start_if[0],
>> + self._start_if[1], self._body)
>> + self._preamble = wrap_ifcond(self._start_if[0],
>> + self._start_if[2], self._preamble)
>
> Can you explain why you put the machinery for conditionals in QAPIGen
> and not QAPIGenCCode?
Iirc, this has to do with the fact that QAPIGenDoc is used for
visiting, and thus QAPIGen start_if()/end_if() could be handled there,
while we only want to generate #ifdef wrapping in QAPIGenCCode. I
guess I could move more of start_if()/end_if() data to QAPIGenCCode
(make them virtual / 'pass' in QAPIGen) Is that what you would like to
see?
>
>> +
>> +
>> +class QAPIGenC(QAPIGenCCode):
>> +
>> + def __init__(self, blurb, pydoc):
>> + QAPIGenCCode.__init__(self)
>> self._blurb = blurb
>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>> re.MULTILINE))
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit
2018-07-03 12:09 ` Markus Armbruster
@ 2018-07-03 13:11 ` Marc-André Lureau
2018-07-03 13:47 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-07-03 13:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Gerd Hoffmann, QEMU, Dr. David Alan Gilbert, Michael Roth
Hi
On Tue, Jul 3, 2018 at 2:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> The generator will take (obj, condition) tuples to wrap generated QLit
>> objects for 'obj' with #if/#endif conditions.
>>
>> This commit adds 'ifcond' condition to top-level QLit objects.
>>
>> See generated tests/test-qmp-introspect.c. Example diff after this patch:
>>
>> --- before 2018-01-08 11:55:24.757083654 +0100
>> +++ tests/test-qmp-introspect.c 2018-01-08 13:08:44.477641629 +0100
>> @@ -51,6 +51,8 @@
>> { "name", QLIT_QSTR("EVENT_F"), },
>> {}
>> })),
>> +#if defined(TEST_IF_CMD)
>> +#if defined(TEST_IF_STRUCT)
>> QLIT_QDICT(((QLitDictEntry[]) {
>> { "arg-type", QLIT_QSTR("5"), },
>> { "meta-type", QLIT_QSTR("command"), },
>> @@ -58,12 +60,16 @@
>> { "ret-type", QLIT_QSTR("0"), },
>> {}
>> })),
>> +#endif /* defined(TEST_IF_STRUCT) */
>> +#endif /* defined(TEST_IF_CMD) */
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 31 +++++++++++++++++++++----------
>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index bd7e1219be..71d4a779ce 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -18,6 +18,15 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>> def indent(level):
>> return level * 4 * ' '
>>
>> + if isinstance(obj, tuple):
>> + ifobj, ifcond = obj
>> + ret = gen_if(ifcond)
>> + ret += to_qlit(ifobj, level)
>> + endif = gen_endif(ifcond)
>> + if endif:
>> + ret += '\n' + endif
>> + return ret
>> +
>> ret = ''
>> if not suppress_first_indent:
>> ret += indent(level)
>
> @obj represents a JSON object. It consists of dictionaries, lists,
> strings, booleans and None. Numbers aren't implemented. Your patch
> splices in tuples to represent conditionals at any level.
>
> So far, tuples occur only as elements of the outermost list (see
> ._gen_qlit() below), but to_qlit() supports them anywhere. I figure
> that will be needed later. Can you point me to such a later use?
It's added in the introspect.py part from "qapi: add #if conditions to
generated code members"
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-07-03 12:35 ` Marc-André Lureau
@ 2018-07-03 13:37 ` Markus Armbruster
2018-07-03 15:08 ` Marc-André Lureau
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 13:37 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, Paolo Bonzini, Gerd Hoffmann, Michael Roth,
Dr. David Alan Gilbert, QEMU
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> Add helpers to wrap generated code with #if/#endif lines.
>>>
>>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>>> inherit from it, for full C files with copyright headers etc.
>>
>> It's called QAPIGenCCode now. Can touch up when I apply, along with
>> another instance below.
>>
>> Leaves the reader wondering *why* you need to splice QAPIGenCCode
>> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
>> Providing the class now reduces code churn, but you should explain why.
>> Perhaps:
>>
>> A later patch wants to use QAPIGen for generating C snippets rather
>> than full C files with copyright headers etc. Splice in class
>> QAPIGenCCode between QAPIGen and QAPIGenC.
>
> sure, thanks
>
>>
>>> Add a 'with' statement context manager that will be used to wrap
>>> generator visitor methods. The manager will check if code was
>>> generated before adding #if/#endif lines on QAPIGenCSnippet
>>> objects. Used in the following patches.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 1b56065a80..44eaf25850 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -12,6 +12,7 @@
>>> # See the COPYING file in the top-level directory.
>>>
>>> from __future__ import print_function
>>> +from contextlib import contextmanager
>>> import errno
>>> import os
>>> import re
>>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>> name=guardname(name))
>>>
>>>
>>> +def gen_if(ifcond):
>>> + ret = ''
>>> + for ifc in ifcond:
>>> + ret += mcgen('''
>>> +#if %(cond)s
>>> +''', cond=ifc)
>>> + return ret
>>> +
>>> +
>>> +def gen_endif(ifcond):
>>> + ret = ''
>>> + for ifc in reversed(ifcond):
>>> + ret += mcgen('''
>>> +#endif /* %(cond)s */
>>> +''', cond=ifc)
>>> + return ret
>>> +
>>> +
>>> +def wrap_ifcond(ifcond, before, after):
>>
>> Looks like a helper method. Name it _wrap_ifcond?
>
> Not fond of functions with _. Iirc, it was suggested by a linter to
> make it a top-level function. I don't mind if you change it on commit.
It's just how Python defines a module's public names. I don't like the
proliferation of leading underscores either, but I feel it's best to go
with the flow here.
>>> + if ifcond is None or before == after:
>>> + return after
>>
>> The before == after part suppresses empty conditionals. Suggest
>>
>> return after # suppress empty #if ... #endif
>>
>> The ifcond is None part is suppresses "non-conditionals". How can this
>> happen? If it can't, let's drop this part.
>
> because the function can be called without additional checks on ifcond
> is None. I don't see what would be the benefit in moving this to the
> caller responsibility.
Why stop at None? Why not empty string? Empty containers other than
[]? False? Numeric zero?
To answer my own question: because a precise function signatures reduce
complexity. "The first argument is a list of strings, no ifs, no buts"
is simpler than "the first argument may be None, or a list of I don't
remember what exactly, but if you screw it up, the function hopefully
throws up before it does real damage" ;)
>> Another non-conditional is []. See below.
>>
>>> +
>>> + assert after.startswith(before)
>>> + out = before
>>> + added = after[len(before):]
>>> + if added[0] == '\n':
>>> + out += '\n'
>>> + added = added[1:]
>>
>> The conditional adjusts vertical space around #if. This might need
>> tweaking, but let's not worry about that now.
>>
>>> + out += gen_if(ifcond)
>>> + out += added
>>> + out += gen_endif(ifcond)
>>
>> There's no similar adjustment for #endif. Again, let's not worry about
>> that now.
>>
>>> + return out
>>> +
>>> +
>>
>> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
>> after) returns after. Okay.
>>
>>> def gen_enum_lookup(name, values, prefix=None):
>>> ret = mcgen('''
>>>
>>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>> def __init__(self):
>>> self._preamble = ''
>>> self._body = ''
>>> + self._start_if = None
>>>
>>> def preamble_add(self, text):
>>> self._preamble += text
>>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>> def add(self, text):
>>> self._body += text
>>>
>>> + def start_if(self, ifcond):
>>> + assert self._start_if is None
>>> +
>>
>> Let's drop this blank line.
>
> I prefer to have preconditions separated from the code, but ok
I sympathize, but not if both are one-liners.
>>
>>> + self._start_if = (ifcond, self._body, self._preamble)
>>
>> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>>
>>> +
>>> + def _wrap_ifcond(self):
>>> + pass
>>> +
>>> + def end_if(self):
>>> + assert self._start_if
>>> +
>>
>> Let's drop this blank line.
>>
>>> + self._wrap_ifcond()
>>> + self._start_if = None
>>> +
>>> + def get_content(self, fname=None):
>>> + assert self._start_if is None
>>> + return (self._top(fname) + self._preamble + self._body
>>> + + self._bottom(fname))
>>> +
>>> def _top(self, fname):
>>> return ''
>>>
>>
>> Note for later: all this code has no effect unless ._wrap_ifcond() is
>> overridden.
>>
>>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>> f = open(fd, 'r+', encoding='utf-8')
>>> else:
>>> f = os.fdopen(fd, 'r+')
>>> - text = (self._top(fname) + self._preamble + self._body
>>> - + self._bottom(fname))
>>> + text = self.get_content(fname)
>>> oldtext = f.read(len(text) + 1)
>>> if text != oldtext:
>>> f.seek(0)
>>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>>> f.close()
>>>
>>>
>>> -class QAPIGenC(QAPIGen):
>>> +@contextmanager
>>> +def ifcontext(ifcond, *args):
>>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>>
>>> - def __init__(self, blurb, pydoc):
>>> + *args: variable length argument list of QAPIGen
>>
>> Perhaps: any number of QAPIGen objects
>>
>
> sure
>
>>> +
>>> + Example::
>>> +
>>> + with ifcontext(ifcond, self._genh, self._genc):
>>> + modify self._genh and self._genc ...
>>> +
>>> + Is equivalent to calling::
>>> +
>>> + self._genh.start_if(ifcond)
>>> + self._genc.start_if(ifcond)
>>> + modify self._genh and self._genc ...
>>> + self._genh.end_if()
>>> + self._genc.end_if()
>>> +
>>
>> Can we drop this blank line?
>>
>
> I guess we can, I haven't tested the rst formatting this rigorously.
> Hopefully the linter does it.
>
>>> + """
>>> + for arg in args:
>>> + arg.start_if(ifcond)
>>> + yield
>>> + for arg in args:
>>> + arg.end_if()
>>> +
>>> +
>>> +class QAPIGenCCode(QAPIGen):
>>> +
>>> + def __init__(self):
>>> QAPIGen.__init__(self)
>>> +
>>> + def _wrap_ifcond(self):
>>> + self._body = wrap_ifcond(self._start_if[0],
>>> + self._start_if[1], self._body)
>>> + self._preamble = wrap_ifcond(self._start_if[0],
>>> + self._start_if[2], self._preamble)
>>
>> Can you explain why you put the machinery for conditionals in QAPIGen
>> and not QAPIGenCCode?
>
> Iirc, this has to do with the fact that QAPIGenDoc is used for
> visiting, and thus QAPIGen start_if()/end_if() could be handled there,
Can you point me to calls of .start_if(), .end_if(), .get_content() for
anything but QAPIGenCCode and its subtypes?
> while we only want to generate #ifdef wrapping in QAPIGenCCode. I
> guess I could move more of start_if()/end_if() data to QAPIGenCCode
> (make them virtual / 'pass' in QAPIGen) Is that what you would like to
> see?
If there are no such calls, we should simply move the whole shebang to
QAPIGenCCode.
If there are such calls, the common supertype QAPIGen has to provide the
methods.
If we expect subtypes other than QAPIGenCCode to implement conditional
code generation the same way, except for a different ._wrap_ifcond(),
then the patch is okay as is.
If we don't, the transformation you described looks like an improvement
to me, because it keeps the actual code closer together.
What's your take on it?
I think I could make the transformation when I apply.
>>
>>> +
>>> +
>>> +class QAPIGenC(QAPIGenCCode):
>>> +
>>> + def __init__(self, blurb, pydoc):
>>> + QAPIGenCCode.__init__(self)
>>> self._blurb = blurb
>>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>>> re.MULTILINE))
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit
2018-07-03 13:11 ` Marc-André Lureau
@ 2018-07-03 13:47 ` Markus Armbruster
2018-07-03 15:08 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 13:47 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, Michael Roth, Paolo Bonzini, Gerd Hoffmann,
Dr. David Alan Gilbert, QEMU
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Jul 3, 2018 at 2:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> The generator will take (obj, condition) tuples to wrap generated QLit
>>> objects for 'obj' with #if/#endif conditions.
>>>
>>> This commit adds 'ifcond' condition to top-level QLit objects.
>>>
>>> See generated tests/test-qmp-introspect.c. Example diff after this patch:
>>>
>>> --- before 2018-01-08 11:55:24.757083654 +0100
>>> +++ tests/test-qmp-introspect.c 2018-01-08 13:08:44.477641629 +0100
>>> @@ -51,6 +51,8 @@
>>> { "name", QLIT_QSTR("EVENT_F"), },
>>> {}
>>> })),
>>> +#if defined(TEST_IF_CMD)
>>> +#if defined(TEST_IF_STRUCT)
>>> QLIT_QDICT(((QLitDictEntry[]) {
>>> { "arg-type", QLIT_QSTR("5"), },
>>> { "meta-type", QLIT_QSTR("command"), },
>>> @@ -58,12 +60,16 @@
>>> { "ret-type", QLIT_QSTR("0"), },
>>> {}
>>> })),
>>> +#endif /* defined(TEST_IF_STRUCT) */
>>> +#endif /* defined(TEST_IF_CMD) */
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 31 +++++++++++++++++++++----------
>>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index bd7e1219be..71d4a779ce 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -18,6 +18,15 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>>> def indent(level):
>>> return level * 4 * ' '
>>>
>>> + if isinstance(obj, tuple):
>>> + ifobj, ifcond = obj
>>> + ret = gen_if(ifcond)
>>> + ret += to_qlit(ifobj, level)
>>> + endif = gen_endif(ifcond)
>>> + if endif:
>>> + ret += '\n' + endif
>>> + return ret
>>> +
>>> ret = ''
>>> if not suppress_first_indent:
>>> ret += indent(level)
>>
>> @obj represents a JSON object. It consists of dictionaries, lists,
>> strings, booleans and None. Numbers aren't implemented. Your patch
>> splices in tuples to represent conditionals at any level.
>>
>> So far, tuples occur only as elements of the outermost list (see
>> ._gen_qlit() below), but to_qlit() supports them anywhere. I figure
>> that will be needed later. Can you point me to such a later use?
>
> It's added in the introspect.py part from "qapi: add #if conditions to
> generated code members"
Thanks!
Let's add a hint to the commit message. Pergaps:
qapi-introspect: add preprocessor conditions to generated QLit
This commit adds 'ifcond' conditions to top-level QLit objects.
Future work will add them to object and enum type members, i.e. within
QLit objects.
Extend the QLit generator to_qlit() to accept (@obj, @cond) tuples in
addition to just @obj. The tuple causes the QLit generated for
objects for @obj with #if/#endif conditions for @cond.
See generated tests/test-qmp-introspect.c. Example diff after this
patch:
[...]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands Marc-André Lureau
@ 2018-07-03 14:43 ` Markus Armbruster
2018-07-03 15:09 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 14:43 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Dr. David Alan Gilbert, Gerd Hoffmann,
Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Wrap generated code with #if/#endif using an 'ifcontext' on
> QAPIGenCSnippet objects.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/commands.py | 21 ++++++++++++---------
> tests/test-qmp-cmds.c | 5 +++--
> 2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index dcc03c7859..72749c7fc5 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -239,7 +239,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> QAPISchemaModularCVisitor.__init__(
> self, prefix, 'qapi-commands',
> ' * Schema-defined QAPI/QMP commands', __doc__)
> - self._regy = ''
> + self._regy = QAPIGenCCode()
> self._visited_ret_types = {}
>
> def _begin_module(self, name):
> @@ -275,20 +275,23 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
> ''',
> c_prefix=c_name(self._prefix, protect=False)))
> - genc.add(gen_registry(self._regy, self._prefix))
> + genc.add(gen_registry(self._regy.get_content(), self._prefix))
>
> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> if not gen:
> return
> - self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> - if ret_type and ret_type not in self._visited_ret_types[self._genc]:
> + if ret_type and \
> + ret_type not in self._visited_ret_types[self._genc]:
PEP8 prefers parenthesis over backslash. Can touch this up when I
apply.
> self._visited_ret_types[self._genc].add(ret_type)
> - self._genc.add(gen_marshal_output(ret_type))
> - self._genh.add(gen_marshal_decl(name))
> - self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> - self._regy += gen_register_command(name, success_response, allow_oob,
> - allow_preconfig)
> + with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
> + self._genc.add(gen_marshal_output(ret_type))
> + with ifcontext(ifcond, self._genh, self._genc, self._regy):
> + self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> + self._genh.add(gen_marshal_decl(name))
> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> + self._regy.add(gen_register_command(name, success_response,
> + allow_oob, allow_preconfig))
Okay, now it falls apart differently :) Let's step through the code
real slow.
The first time we visit a command C returning type T...
if ret_type and \
ret_type not in self._visited_ret_types[self._genc]:
self._visited_ret_types[self._genc].add(ret_type)
with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
self._genc.add(gen_marshal_output(ret_type))
... we generate qmp_marshal_output_T() wrapped in T's condition.
with ifcontext(ifcond, self._genh, self._genc, self._regy):
self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
self._genh.add(gen_marshal_decl(name))
self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
self._regy.add(gen_register_command(name, success_response,
allow_oob, allow_preconfig))
We generate qmp_marshal_C() wrapped in C's condition. This is what
calls qmp_marshal_output_T().
If T is a user-defined type, the user is responsible for making this
work, i.e. to make T's condition the conjunction of the T-returning
commands' conditions.
If T is a built-in type, this isn't possible: the qmp_marshal_output_T()
will be generated unconditionally.
I append a crude patch to provide test coverage (crude because it
removes coverage of another case). With it applied, make check dies
with
tests/test-qapi-commands.c:470:13: warning: ‘qmp_marshal_output_str’ defined but not used [-Wunused-function]
I think the issue is obscure enough to justify postponing a proper fix,
and just add a FIXME here now. I can do that when I apply.
>
>
> def gen_commands(schema, output_dir, prefix):
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index 840530b84c..bd27353908 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -12,12 +12,13 @@
>
> static QmpCommandList qmp_commands;
>
> -/* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
> +#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
> UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
> +void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
Whoops!
Not caught by make check since it compiles with none of the conditionals
defined. Improvement welcome, but not necessary to get this series in.
I can fix the editing accident when I apply.
> {
> return NULL;
> }
> -/* #endif */
> +#endif
>
> UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
> {
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 16209b57b3..4dd1ce3703 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -8,7 +8,8 @@
# Commands allowed to return a non-dictionary:
'returns-whitelist': [
'guest-get-time',
- 'guest-sync' ] } }
+ 'guest-sync',
+ 'TestIfCmd' ] } }
{ 'struct': 'TestStruct',
'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } }
@@ -56,9 +57,6 @@
'data': { 'string0': 'str',
'dict1': 'UserDefTwoDict' } }
-{ 'struct': 'UserDefThree',
- 'data': { 'string0': 'str' } }
-
# dummy struct to force generation of array types not otherwise mentioned
{ 'struct': 'ForceArrays',
'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
@@ -212,10 +210,8 @@
'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
- 'returns': 'UserDefThree',
+ 'returns': 'str',
'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
-{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
-
{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 0da92455da..aafa40c226 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -36,8 +36,6 @@ object UserDefTwoDict
object UserDefTwo
member string0: str optional=False
member dict1: UserDefTwoDict optional=False
-object UserDefThree
- member string0: str optional=False
object ForceArrays
member unused1: UserDefOneList optional=False
member unused2: UserDefTwoList optional=False
@@ -257,11 +255,9 @@ alternate TestIfAlternate
object q_obj_TestIfCmd-arg
member foo: TestIfStruct optional=False
if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
+command TestIfCmd q_obj_TestIfCmd-arg -> str
gen=True success_response=True boxed=False oob=False preconfig=False
if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestCmdReturnDefThree None -> UserDefThree
- gen=True success_response=True boxed=False oob=False preconfig=False
object q_obj_TestIfEvent-arg
member foo: TestIfStruct optional=False
if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index bd27353908..6ba73fd53c 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -13,18 +13,12 @@
static QmpCommandList qmp_commands;
#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
-UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
-void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
+char *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
{
return NULL;
}
#endif
-UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
-{
- return NULL;
-}
-
void qmp_user_def_cmd(Error **errp)
{
}
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events Marc-André Lureau
@ 2018-07-03 14:47 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 14:47 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Wrap generated code with #if/#endif using an 'ifcontext' on
> QAPIGenCSnippet objects.
>
> This makes a conditional event's qapi_event_send_FOO() compile-time
> conditional, but its enum QAPIEvent member remains unconditional for
> now. A follow up patch "qapi-event: add 'if' condition to implicit
> event enum" will improve this.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors Marc-André Lureau
@ 2018-07-03 14:50 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 14:50 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Dr. David Alan Gilbert, Gerd Hoffmann,
Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> From: Markus Armbruster <armbru@redhat.com>
>
> Types & visitors are coupled and must be handled together to avoid
> temporary build regression.
>
> Wrap generated types/visitor code with #if/#endif using the context
> helpers.
I'll insert here:
Derived from a patch by Marc-André.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation Marc-André Lureau
@ 2018-07-03 14:59 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 14:59 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> The documentation is generated only once, and doesn't know C
> pre-conditions. Add 'If:' sections for top-level entities.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/doc.py | 22 ++++++++++++----------
> tests/qapi-schema/doc-good.json | 2 +-
> tests/qapi-schema/doc-good.out | 1 +
> tests/qapi-schema/doc-good.texi | 2 ++
> 4 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 4db6674dc3..987fd3c943 100755
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -174,7 +174,7 @@ def texi_members(doc, what, base, variants, member_func):
> return '\n@b{%s:}\n@table @asis\n%s@end table\n' % (what, items)
>
>
> -def texi_sections(doc):
> +def texi_sections(doc, ifcond):
> """Format additional sections following arguments"""
> body = ''
> for section in doc.sections:
> @@ -185,14 +185,16 @@ def texi_sections(doc):
> body += texi_example(section.text)
> else:
> body += texi_format(section.text)
> + if ifcond:
> + body += '\n\n@b{If:} @code{%s}' % ", ".join(ifcond)
> return body
>
>
> -def texi_entity(doc, what, base=None, variants=None,
> +def texi_entity(doc, what, ifcond, base=None, variants=None,
> member_func=texi_member):
> return (texi_body(doc)
> + texi_members(doc, what, base, variants, member_func)
> - + texi_sections(doc))
> + + texi_sections(doc, ifcond))
>
>
> class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> @@ -208,7 +210,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> doc = self.cur_doc
> self._gen.add(TYPE_FMT(type='Enum',
> name=doc.symbol,
> - body=texi_entity(doc, 'Values',
> + body=texi_entity(doc, 'Values', ifcond,
> member_func=texi_enum_value)))
>
> def visit_object_type(self, name, info, ifcond, base, members, variants):
> @@ -217,14 +219,14 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> base = None
> self._gen.add(TYPE_FMT(type='Object',
> name=doc.symbol,
> - body=texi_entity(doc, 'Members',
> + body=texi_entity(doc, 'Members', ifcond,
> base, variants)))
>
> def visit_alternate_type(self, name, info, ifcond, variants):
> doc = self.cur_doc
> self._gen.add(TYPE_FMT(type='Alternate',
> name=doc.symbol,
> - body=texi_entity(doc, 'Members')))
> + body=texi_entity(doc, 'Members', ifcond)))
>
> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> success_response, boxed, allow_oob, allow_preconfig):
> @@ -233,9 +235,9 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> body = texi_body(doc)
> body += ('\n@b{Arguments:} the members of @code{%s}\n'
> % arg_type.name)
> - body += texi_sections(doc)
> + body += texi_sections(doc, ifcond)
> else:
> - body = texi_entity(doc, 'Arguments')
> + body = texi_entity(doc, 'Arguments', ifcond)
> self._gen.add(MSG_FMT(type='Command',
> name=doc.symbol,
> body=body))
> @@ -244,7 +246,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> doc = self.cur_doc
> self._gen.add(MSG_FMT(type='Event',
> name=doc.symbol,
> - body=texi_entity(doc, 'Arguments')))
> + body=texi_entity(doc, 'Arguments', ifcond)))
>
> def symbol(self, doc, entity):
> if self._gen._body:
> @@ -257,7 +259,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
> assert not doc.args
> if self._gen._body:
> self._gen.add('\n')
> - self._gen.add(texi_body(doc) + texi_sections(doc))
> + self._gen.add(texi_body(doc) + texi_sections(doc, None))
>
>
> def gen_doc(schema, output_dir, prefix):
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index 97ab4625ff..984cd8ed06 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -55,7 +55,7 @@
> #
> # @two is undocumented
> ##
> -{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
> +{ 'enum': 'Enum', 'data': [ 'one', 'two' ], 'if': 'defined(IFCOND)' }
>
> ##
> # @Base:
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 9c8a4838e1..35f3f1164c 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -3,6 +3,7 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
> prefix QTYPE
> module doc-good.json
> enum Enum ['one', 'two']
> + if ['defined(IFCOND)']
> object Base
> member base1: Enum optional=False
> object Variant1
> diff --git a/tests/qapi-schema/doc-good.texi b/tests/qapi-schema/doc-good.texi
> index 0aed2300a5..e42eace474 100644
> --- a/tests/qapi-schema/doc-good.texi
> +++ b/tests/qapi-schema/doc-good.texi
> @@ -89,6 +89,8 @@ Not documented
> @end table
> @code{two} is undocumented
>
> +
> +@b{If:} @code{defined(IFCOND)}
> @end deftp
One blank line would be better, but that's as pervasive as it is minor.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
For the record, wanted follow-up work, from v5's review:
* The big comment in qapi-schema.json should explain the documentation
format, including the meaning of tags like 'If:', 'Since:' and so
forth
* Better test coverage
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema Marc-André Lureau
@ 2018-07-03 15:04 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 15:04 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, Dr. David Alan Gilbert, Gerd Hoffmann,
Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> query-qmp-schema no longer reports the command/events etc as
> available when disabled at compile.
>
> Commands made conditional:
>
> * query-vnc, query-vnc-servers, change-vnc-password
>
> Before the patch, the commands for !CONFIG_VNC are stubs that fail
> like this:
>
> {"error": {"class": "GenericError",
> "desc": "The feature 'vnc' is not enabled"}}
>
> Afterwards, they fail like this:
>
> {"error": {"class": "CommandNotFound",
> "desc": "The command FOO has not been found"}}
>
> I call that an improvement, because it lets clients distinguish
> between command unavailable (class CommandNotFound) and command failed
> (class GenericError).
>
> Events made conditional:
>
> * VNC_CONNECTED, VNC_INITIALIZED, VNC_DISCONNECTED
>
> HMP change:
>
> * info vnc
>
> Will return "unknown command: 'info vnc'" when VNC is compiled
> out (same as error for spice when --disable-spice)
>
> Occurrences of VNC (case insensitive) in the schema that aren't
> covered by this change:
>
> * add_client
>
> Command has other uses, including "socket bases character devices".
> These are unconditional as far as I can tell.
>
> * set_password, expire_password
>
> In theory, these commands could be used for managing any service's
> password. In practice, they're used for VNC and SPICE services.
> They're documented for "remote display session" / "remote display
> server".
>
> The service is selected by argument @protocol. The code special-cases
> protocol-specific argument checking, then calls a protocol-specific
> function to do the work. If it fails, the command fails with "Could
> not set password". It does when the service isn't compiled in (it's a
> stub then).
>
> We could make these commands conditional on the conjunction of all
> services [currently: defined(CONFIG_VNC) || defined(CONFIG_SPICE)],
> but I doubt it's worthwhile.
>
> * change
>
> Command has other uses, namely changing media.
> This patch inlines a stub; no functional change.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE type/commands/events on the schema
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE " Marc-André Lureau
@ 2018-07-03 15:05 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 15:05 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael Roth, armbru, Dr. David Alan Gilbert,
Gerd Hoffmann, Paolo Bonzini
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Add #if defined(CONFIG_SPICE) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> query-qmp-schema no longer reports the command/events etc as
> available when disabled at compile time.
>
> Commands made conditional:
>
> * query-spice
>
> Before the patch, the command for !CONFIG_SPICE is unregistered. It
> will fail with the same error.
>
> Events made conditional:
>
> * SPICE_CONNECTED, SPICE_INITIALIZED, SPICE_DISCONNECTED,
> SPICE_MIGRATE_COMPLETED
>
> Add TODO for conditional SPICE chardevs, delayed until the supports
> for conditional members lands.
>
> No HMP change, the code was already conditional.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit
2018-07-03 13:47 ` Markus Armbruster
@ 2018-07-03 15:08 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 15:08 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael Roth, Paolo Bonzini, Gerd Hoffmann, Dr. David Alan Gilbert, QEMU
Markus Armbruster <armbru@redhat.com> writes:
[...]
> Let's add a hint to the commit message. Pergaps:
>
> qapi-introspect: add preprocessor conditions to generated QLit
>
> This commit adds 'ifcond' conditions to top-level QLit objects.
> Future work will add them to object and enum type members, i.e. within
> QLit objects.
>
> Extend the QLit generator to_qlit() to accept (@obj, @cond) tuples in
> addition to just @obj. The tuple causes the QLit generated for
> objects for @obj with #if/#endif conditions for @cond.
>
> See generated tests/test-qmp-introspect.c. Example diff after this
> patch:
> [...]
With that,
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-07-03 13:37 ` Markus Armbruster
@ 2018-07-03 15:08 ` Marc-André Lureau
2018-07-03 15:34 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Marc-André Lureau @ 2018-07-03 15:08 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Gerd Hoffmann, Michael Roth, Dr. David Alan Gilbert, QEMU
Hi
On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
>> Hi
>>
>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>
>>>> Add helpers to wrap generated code with #if/#endif lines.
>>>>
>>>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>>>> inherit from it, for full C files with copyright headers etc.
>>>
>>> It's called QAPIGenCCode now. Can touch up when I apply, along with
>>> another instance below.
>>>
>>> Leaves the reader wondering *why* you need to splice QAPIGenCCode
>>> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
>>> Providing the class now reduces code churn, but you should explain why.
>>> Perhaps:
>>>
>>> A later patch wants to use QAPIGen for generating C snippets rather
>>> than full C files with copyright headers etc. Splice in class
>>> QAPIGenCCode between QAPIGen and QAPIGenC.
>>
>> sure, thanks
>>
>>>
>>>> Add a 'with' statement context manager that will be used to wrap
>>>> generator visitor methods. The manager will check if code was
>>>> generated before adding #if/#endif lines on QAPIGenCSnippet
>>>> objects. Used in the following patches.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>>> index 1b56065a80..44eaf25850 100644
>>>> --- a/scripts/qapi/common.py
>>>> +++ b/scripts/qapi/common.py
>>>> @@ -12,6 +12,7 @@
>>>> # See the COPYING file in the top-level directory.
>>>>
>>>> from __future__ import print_function
>>>> +from contextlib import contextmanager
>>>> import errno
>>>> import os
>>>> import re
>>>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>>> name=guardname(name))
>>>>
>>>>
>>>> +def gen_if(ifcond):
>>>> + ret = ''
>>>> + for ifc in ifcond:
>>>> + ret += mcgen('''
>>>> +#if %(cond)s
>>>> +''', cond=ifc)
>>>> + return ret
>>>> +
>>>> +
>>>> +def gen_endif(ifcond):
>>>> + ret = ''
>>>> + for ifc in reversed(ifcond):
>>>> + ret += mcgen('''
>>>> +#endif /* %(cond)s */
>>>> +''', cond=ifc)
>>>> + return ret
>>>> +
>>>> +
>>>> +def wrap_ifcond(ifcond, before, after):
>>>
>>> Looks like a helper method. Name it _wrap_ifcond?
>>
>> Not fond of functions with _. Iirc, it was suggested by a linter to
>> make it a top-level function. I don't mind if you change it on commit.
>
> It's just how Python defines a module's public names. I don't like the
> proliferation of leading underscores either, but I feel it's best to go
> with the flow here.
ok
>
>>>> + if ifcond is None or before == after:
>>>> + return after
>>>
>>> The before == after part suppresses empty conditionals. Suggest
>>>
>>> return after # suppress empty #if ... #endif
>>>
>>> The ifcond is None part is suppresses "non-conditionals". How can this
>>> happen? If it can't, let's drop this part.
>>
>> because the function can be called without additional checks on ifcond
>> is None. I don't see what would be the benefit in moving this to the
>> caller responsibility.
>
> Why stop at None? Why not empty string? Empty containers other than
> []? False? Numeric zero?
>
> To answer my own question: because a precise function signatures reduce
> complexity. "The first argument is a list of strings, no ifs, no buts"
> is simpler than "the first argument may be None, or a list of I don't
> remember what exactly, but if you screw it up, the function hopefully
> throws up before it does real damage" ;)
So you prefer if not ifcond or before == after ? ok
>
>>> Another non-conditional is []. See below.
>>>
>>>> +
>>>> + assert after.startswith(before)
>>>> + out = before
>>>> + added = after[len(before):]
>>>> + if added[0] == '\n':
>>>> + out += '\n'
>>>> + added = added[1:]
>>>
>>> The conditional adjusts vertical space around #if. This might need
>>> tweaking, but let's not worry about that now.
>>>
>>>> + out += gen_if(ifcond)
>>>> + out += added
>>>> + out += gen_endif(ifcond)
>>>
>>> There's no similar adjustment for #endif. Again, let's not worry about
>>> that now.
>>>
>>>> + return out
>>>> +
>>>> +
>>>
>>> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
>>> after) returns after. Okay.
>>>
>>>> def gen_enum_lookup(name, values, prefix=None):
>>>> ret = mcgen('''
>>>>
>>>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>>> def __init__(self):
>>>> self._preamble = ''
>>>> self._body = ''
>>>> + self._start_if = None
>>>>
>>>> def preamble_add(self, text):
>>>> self._preamble += text
>>>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>>> def add(self, text):
>>>> self._body += text
>>>>
>>>> + def start_if(self, ifcond):
>>>> + assert self._start_if is None
>>>> +
>>>
>>> Let's drop this blank line.
>>
>> I prefer to have preconditions separated from the code, but ok
>
> I sympathize, but not if both are one-liners.
ok
>
>>>
>>>> + self._start_if = (ifcond, self._body, self._preamble)
>>>
>>> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>>>
>>>> +
>>>> + def _wrap_ifcond(self):
>>>> + pass
>>>> +
>>>> + def end_if(self):
>>>> + assert self._start_if
>>>> +
>>>
>>> Let's drop this blank line.
>>>
>>>> + self._wrap_ifcond()
>>>> + self._start_if = None
>>>> +
>>>> + def get_content(self, fname=None):
>>>> + assert self._start_if is None
>>>> + return (self._top(fname) + self._preamble + self._body
>>>> + + self._bottom(fname))
>>>> +
>>>> def _top(self, fname):
>>>> return ''
>>>>
>>>
>>> Note for later: all this code has no effect unless ._wrap_ifcond() is
>>> overridden.
>>>
>>>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>>> f = open(fd, 'r+', encoding='utf-8')
>>>> else:
>>>> f = os.fdopen(fd, 'r+')
>>>> - text = (self._top(fname) + self._preamble + self._body
>>>> - + self._bottom(fname))
>>>> + text = self.get_content(fname)
>>>> oldtext = f.read(len(text) + 1)
>>>> if text != oldtext:
>>>> f.seek(0)
>>>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>>>> f.close()
>>>>
>>>>
>>>> -class QAPIGenC(QAPIGen):
>>>> +@contextmanager
>>>> +def ifcontext(ifcond, *args):
>>>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>>>
>>>> - def __init__(self, blurb, pydoc):
>>>> + *args: variable length argument list of QAPIGen
>>>
>>> Perhaps: any number of QAPIGen objects
>>>
>>
>> sure
>>
>>>> +
>>>> + Example::
>>>> +
>>>> + with ifcontext(ifcond, self._genh, self._genc):
>>>> + modify self._genh and self._genc ...
>>>> +
>>>> + Is equivalent to calling::
>>>> +
>>>> + self._genh.start_if(ifcond)
>>>> + self._genc.start_if(ifcond)
>>>> + modify self._genh and self._genc ...
>>>> + self._genh.end_if()
>>>> + self._genc.end_if()
>>>> +
>>>
>>> Can we drop this blank line?
>>>
>>
>> I guess we can, I haven't tested the rst formatting this rigorously.
>> Hopefully the linter does it.
>>
>>>> + """
>>>> + for arg in args:
>>>> + arg.start_if(ifcond)
>>>> + yield
>>>> + for arg in args:
>>>> + arg.end_if()
>>>> +
>>>> +
>>>> +class QAPIGenCCode(QAPIGen):
>>>> +
>>>> + def __init__(self):
>>>> QAPIGen.__init__(self)
>>>> +
>>>> + def _wrap_ifcond(self):
>>>> + self._body = wrap_ifcond(self._start_if[0],
>>>> + self._start_if[1], self._body)
>>>> + self._preamble = wrap_ifcond(self._start_if[0],
>>>> + self._start_if[2], self._preamble)
>>>
>>> Can you explain why you put the machinery for conditionals in QAPIGen
>>> and not QAPIGenCCode?
>>
>> Iirc, this has to do with the fact that QAPIGenDoc is used for
>> visiting, and thus QAPIGen start_if()/end_if() could be handled there,
>
> Can you point me to calls of .start_if(), .end_if(), .get_content() for
> anything but QAPIGenCCode and its subtypes?
>
>> while we only want to generate #ifdef wrapping in QAPIGenCCode. I
>> guess I could move more of start_if()/end_if() data to QAPIGenCCode
>> (make them virtual / 'pass' in QAPIGen) Is that what you would like to
>> see?
>
> If there are no such calls, we should simply move the whole shebang to
> QAPIGenCCode.
done
>
> If there are such calls, the common supertype QAPIGen has to provide the
> methods.
>
> If we expect subtypes other than QAPIGenCCode to implement conditional
> code generation the same way, except for a different ._wrap_ifcond(),
> then the patch is okay as is.
>
> If we don't, the transformation you described looks like an improvement
> to me, because it keeps the actual code closer together.
>
> What's your take on it?
>
> I think I could make the transformation when I apply.
>
>>>
>>>> +
>>>> +
>>>> +class QAPIGenC(QAPIGenCCode):
>>>> +
>>>> + def __init__(self, blurb, pydoc):
>>>> + QAPIGenCCode.__init__(self)
>>>> self._blurb = blurb
>>>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>>>> re.MULTILINE))
>>>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands
2018-07-03 14:43 ` Markus Armbruster
@ 2018-07-03 15:09 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 15:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Michael Roth,
Dr. David Alan Gilbert, Gerd Hoffmann, Paolo Bonzini
Markus Armbruster <armbru@redhat.com> writes:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Wrap generated code with #if/#endif using an 'ifcontext' on
>> QAPIGenCSnippet objects.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> scripts/qapi/commands.py | 21 ++++++++++++---------
>> tests/test-qmp-cmds.c | 5 +++--
>> 2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index dcc03c7859..72749c7fc5 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -239,7 +239,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>> QAPISchemaModularCVisitor.__init__(
>> self, prefix, 'qapi-commands',
>> ' * Schema-defined QAPI/QMP commands', __doc__)
>> - self._regy = ''
>> + self._regy = QAPIGenCCode()
>> self._visited_ret_types = {}
>>
>> def _begin_module(self, name):
>> @@ -275,20 +275,23 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>> void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>> ''',
>> c_prefix=c_name(self._prefix, protect=False)))
>> - genc.add(gen_registry(self._regy, self._prefix))
>> + genc.add(gen_registry(self._regy.get_content(), self._prefix))
>>
>> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>> success_response, boxed, allow_oob, allow_preconfig):
>> if not gen:
>> return
>> - self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>> - if ret_type and ret_type not in self._visited_ret_types[self._genc]:
>> + if ret_type and \
>> + ret_type not in self._visited_ret_types[self._genc]:
>
> PEP8 prefers parenthesis over backslash. Can touch this up when I
> apply.
>
>> self._visited_ret_types[self._genc].add(ret_type)
>> - self._genc.add(gen_marshal_output(ret_type))
>> - self._genh.add(gen_marshal_decl(name))
>> - self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>> - self._regy += gen_register_command(name, success_response, allow_oob,
>> - allow_preconfig)
>> + with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
>> + self._genc.add(gen_marshal_output(ret_type))
>> + with ifcontext(ifcond, self._genh, self._genc, self._regy):
>> + self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>> + self._genh.add(gen_marshal_decl(name))
>> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>> + self._regy.add(gen_register_command(name, success_response,
>> + allow_oob, allow_preconfig))
>
> Okay, now it falls apart differently :) Let's step through the code
> real slow.
>
> The first time we visit a command C returning type T...
>
> if ret_type and \
> ret_type not in self._visited_ret_types[self._genc]:
> self._visited_ret_types[self._genc].add(ret_type)
> with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
> self._genc.add(gen_marshal_output(ret_type))
>
> ... we generate qmp_marshal_output_T() wrapped in T's condition.
>
> with ifcontext(ifcond, self._genh, self._genc, self._regy):
> self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> self._genh.add(gen_marshal_decl(name))
> self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> self._regy.add(gen_register_command(name, success_response,
> allow_oob, allow_preconfig))
>
> We generate qmp_marshal_C() wrapped in C's condition. This is what
> calls qmp_marshal_output_T().
>
> If T is a user-defined type, the user is responsible for making this
> work, i.e. to make T's condition the conjunction of the T-returning
> commands' conditions.
>
> If T is a built-in type, this isn't possible: the qmp_marshal_output_T()
> will be generated unconditionally.
>
> I append a crude patch to provide test coverage (crude because it
> removes coverage of another case). With it applied, make check dies
> with
>
> tests/test-qapi-commands.c:470:13: warning: ‘qmp_marshal_output_str’ defined but not used [-Wunused-function]
>
> I think the issue is obscure enough to justify postponing a proper fix,
> and just add a FIXME here now. I can do that when I apply.
With the PEP8 tweak and a suitable FIXME
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-07-03 15:08 ` Marc-André Lureau
@ 2018-07-03 15:34 ` Markus Armbruster
2018-07-03 15:43 ` Marc-André Lureau
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-03 15:34 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, QEMU, Paolo Bonzini, Gerd Hoffmann,
Dr. David Alan Gilbert, Michael Roth
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>>> Hi
>>>
>>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>>
>>>>> Add helpers to wrap generated code with #if/#endif lines.
>>>>>
>>>>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>>>>> inherit from it, for full C files with copyright headers etc.
>>>>
>>>> It's called QAPIGenCCode now. Can touch up when I apply, along with
>>>> another instance below.
>>>>
>>>> Leaves the reader wondering *why* you need to splice QAPIGenCCode
>>>> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
>>>> Providing the class now reduces code churn, but you should explain why.
>>>> Perhaps:
>>>>
>>>> A later patch wants to use QAPIGen for generating C snippets rather
>>>> than full C files with copyright headers etc. Splice in class
>>>> QAPIGenCCode between QAPIGen and QAPIGenC.
>>>
>>> sure, thanks
>>>
>>>>
>>>>> Add a 'with' statement context manager that will be used to wrap
>>>>> generator visitor methods. The manager will check if code was
>>>>> generated before adding #if/#endif lines on QAPIGenCSnippet
>>>>> objects. Used in the following patches.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>>>> index 1b56065a80..44eaf25850 100644
>>>>> --- a/scripts/qapi/common.py
>>>>> +++ b/scripts/qapi/common.py
>>>>> @@ -12,6 +12,7 @@
>>>>> # See the COPYING file in the top-level directory.
>>>>>
>>>>> from __future__ import print_function
>>>>> +from contextlib import contextmanager
>>>>> import errno
>>>>> import os
>>>>> import re
>>>>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>>>> name=guardname(name))
>>>>>
>>>>>
>>>>> +def gen_if(ifcond):
>>>>> + ret = ''
>>>>> + for ifc in ifcond:
>>>>> + ret += mcgen('''
>>>>> +#if %(cond)s
>>>>> +''', cond=ifc)
>>>>> + return ret
>>>>> +
>>>>> +
>>>>> +def gen_endif(ifcond):
>>>>> + ret = ''
>>>>> + for ifc in reversed(ifcond):
>>>>> + ret += mcgen('''
>>>>> +#endif /* %(cond)s */
>>>>> +''', cond=ifc)
>>>>> + return ret
>>>>> +
>>>>> +
>>>>> +def wrap_ifcond(ifcond, before, after):
>>>>
>>>> Looks like a helper method. Name it _wrap_ifcond?
>>>
>>> Not fond of functions with _. Iirc, it was suggested by a linter to
>>> make it a top-level function. I don't mind if you change it on commit.
>>
>> It's just how Python defines a module's public names. I don't like the
>> proliferation of leading underscores either, but I feel it's best to go
>> with the flow here.
>
> ok
>
>>
>>>>> + if ifcond is None or before == after:
>>>>> + return after
>>>>
>>>> The before == after part suppresses empty conditionals. Suggest
>>>>
>>>> return after # suppress empty #if ... #endif
>>>>
>>>> The ifcond is None part is suppresses "non-conditionals". How can this
>>>> happen? If it can't, let's drop this part.
>>>
>>> because the function can be called without additional checks on ifcond
>>> is None. I don't see what would be the benefit in moving this to the
>>> caller responsibility.
>>
>> Why stop at None? Why not empty string? Empty containers other than
>> []? False? Numeric zero?
>>
>> To answer my own question: because a precise function signatures reduce
>> complexity. "The first argument is a list of strings, no ifs, no buts"
>> is simpler than "the first argument may be None, or a list of I don't
>> remember what exactly, but if you screw it up, the function hopefully
>> throws up before it does real damage" ;)
>
> So you prefer if not ifcond or before == after ? ok
I'd recommend
+ if before == after:
+ return after
If we pass crap, we die in gen_if()'s for ifc in ifcond. The difference
is now crap includes None.
>>>> Another non-conditional is []. See below.
>>>>
>>>>> +
>>>>> + assert after.startswith(before)
>>>>> + out = before
>>>>> + added = after[len(before):]
>>>>> + if added[0] == '\n':
>>>>> + out += '\n'
>>>>> + added = added[1:]
>>>>
>>>> The conditional adjusts vertical space around #if. This might need
>>>> tweaking, but let's not worry about that now.
>>>>
>>>>> + out += gen_if(ifcond)
>>>>> + out += added
>>>>> + out += gen_endif(ifcond)
>>>>
>>>> There's no similar adjustment for #endif. Again, let's not worry about
>>>> that now.
>>>>
>>>>> + return out
>>>>> +
>>>>> +
>>>>
>>>> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
>>>> after) returns after. Okay.
>>>>
>>>>> def gen_enum_lookup(name, values, prefix=None):
>>>>> ret = mcgen('''
>>>>>
>>>>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>>>> def __init__(self):
>>>>> self._preamble = ''
>>>>> self._body = ''
>>>>> + self._start_if = None
>>>>>
>>>>> def preamble_add(self, text):
>>>>> self._preamble += text
>>>>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>>>> def add(self, text):
>>>>> self._body += text
>>>>>
>>>>> + def start_if(self, ifcond):
>>>>> + assert self._start_if is None
>>>>> +
>>>>
>>>> Let's drop this blank line.
>>>
>>> I prefer to have preconditions separated from the code, but ok
>>
>> I sympathize, but not if both are one-liners.
>
> ok
>
>>
>>>>
>>>>> + self._start_if = (ifcond, self._body, self._preamble)
>>>>
>>>> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>>>>
>>>>> +
>>>>> + def _wrap_ifcond(self):
>>>>> + pass
>>>>> +
>>>>> + def end_if(self):
>>>>> + assert self._start_if
>>>>> +
>>>>
>>>> Let's drop this blank line.
>>>>
>>>>> + self._wrap_ifcond()
>>>>> + self._start_if = None
>>>>> +
>>>>> + def get_content(self, fname=None):
>>>>> + assert self._start_if is None
>>>>> + return (self._top(fname) + self._preamble + self._body
>>>>> + + self._bottom(fname))
>>>>> +
>>>>> def _top(self, fname):
>>>>> return ''
>>>>>
>>>>
>>>> Note for later: all this code has no effect unless ._wrap_ifcond() is
>>>> overridden.
>>>>
>>>>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>>>> f = open(fd, 'r+', encoding='utf-8')
>>>>> else:
>>>>> f = os.fdopen(fd, 'r+')
>>>>> - text = (self._top(fname) + self._preamble + self._body
>>>>> - + self._bottom(fname))
>>>>> + text = self.get_content(fname)
>>>>> oldtext = f.read(len(text) + 1)
>>>>> if text != oldtext:
>>>>> f.seek(0)
>>>>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>>>>> f.close()
>>>>>
>>>>>
>>>>> -class QAPIGenC(QAPIGen):
>>>>> +@contextmanager
>>>>> +def ifcontext(ifcond, *args):
>>>>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>>>>
>>>>> - def __init__(self, blurb, pydoc):
>>>>> + *args: variable length argument list of QAPIGen
>>>>
>>>> Perhaps: any number of QAPIGen objects
>>>>
>>>
>>> sure
>>>
>>>>> +
>>>>> + Example::
>>>>> +
>>>>> + with ifcontext(ifcond, self._genh, self._genc):
>>>>> + modify self._genh and self._genc ...
>>>>> +
>>>>> + Is equivalent to calling::
>>>>> +
>>>>> + self._genh.start_if(ifcond)
>>>>> + self._genc.start_if(ifcond)
>>>>> + modify self._genh and self._genc ...
>>>>> + self._genh.end_if()
>>>>> + self._genc.end_if()
>>>>> +
>>>>
>>>> Can we drop this blank line?
>>>>
>>>
>>> I guess we can, I haven't tested the rst formatting this rigorously.
>>> Hopefully the linter does it.
>>>
>>>>> + """
>>>>> + for arg in args:
>>>>> + arg.start_if(ifcond)
>>>>> + yield
>>>>> + for arg in args:
>>>>> + arg.end_if()
>>>>> +
>>>>> +
>>>>> +class QAPIGenCCode(QAPIGen):
>>>>> +
>>>>> + def __init__(self):
>>>>> QAPIGen.__init__(self)
>>>>> +
>>>>> + def _wrap_ifcond(self):
>>>>> + self._body = wrap_ifcond(self._start_if[0],
>>>>> + self._start_if[1], self._body)
>>>>> + self._preamble = wrap_ifcond(self._start_if[0],
>>>>> + self._start_if[2], self._preamble)
>>>>
>>>> Can you explain why you put the machinery for conditionals in QAPIGen
>>>> and not QAPIGenCCode?
>>>
>>> Iirc, this has to do with the fact that QAPIGenDoc is used for
>>> visiting, and thus QAPIGen start_if()/end_if() could be handled there,
>>
>> Can you point me to calls of .start_if(), .end_if(), .get_content() for
>> anything but QAPIGenCCode and its subtypes?
>>
>>> while we only want to generate #ifdef wrapping in QAPIGenCCode. I
>>> guess I could move more of start_if()/end_if() data to QAPIGenCCode
>>> (make them virtual / 'pass' in QAPIGen) Is that what you would like to
>>> see?
>>
>> If there are no such calls, we should simply move the whole shebang to
>> QAPIGenCCode.
>
> done
Does that mean I should expect v7 from you?
>> If there are such calls, the common supertype QAPIGen has to provide the
>> methods.
>>
>> If we expect subtypes other than QAPIGenCCode to implement conditional
>> code generation the same way, except for a different ._wrap_ifcond(),
>> then the patch is okay as is.
>>
>> If we don't, the transformation you described looks like an improvement
>> to me, because it keeps the actual code closer together.
>>
>> What's your take on it?
>>
>> I think I could make the transformation when I apply.
>>
>>>>
>>>>> +
>>>>> +
>>>>> +class QAPIGenC(QAPIGenCCode):
>>>>> +
>>>>> + def __init__(self, blurb, pydoc):
>>>>> + QAPIGenCCode.__init__(self)
>>>>> self._blurb = blurb
>>>>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>>>>> re.MULTILINE))
>>>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers
2018-07-03 15:34 ` Markus Armbruster
@ 2018-07-03 15:43 ` Marc-André Lureau
0 siblings, 0 replies; 39+ messages in thread
From: Marc-André Lureau @ 2018-07-03 15:43 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU, Paolo Bonzini, Gerd Hoffmann, Dr. David Alan Gilbert, Michael Roth
Hi
On Tue, Jul 3, 2018 at 5:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
>> Hi
>>
>> On Tue, Jul 3, 2018 at 3:37 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>>
>>>> Hi
>>>>
>>>> On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>>>
>>>>>> Add helpers to wrap generated code with #if/#endif lines.
>>>>>>
>>>>>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>>>>>> inherit from it, for full C files with copyright headers etc.
>>>>>
>>>>> It's called QAPIGenCCode now. Can touch up when I apply, along with
>>>>> another instance below.
>>>>>
>>>>> Leaves the reader wondering *why* you need to splice QAPIGenCCode
>>>>> between QAPIGen and QAPIGenC. Becomes clear only in PATCH 10.
>>>>> Providing the class now reduces code churn, but you should explain why.
>>>>> Perhaps:
>>>>>
>>>>> A later patch wants to use QAPIGen for generating C snippets rather
>>>>> than full C files with copyright headers etc. Splice in class
>>>>> QAPIGenCCode between QAPIGen and QAPIGenC.
>>>>
>>>> sure, thanks
>>>>
>>>>>
>>>>>> Add a 'with' statement context manager that will be used to wrap
>>>>>> generator visitor methods. The manager will check if code was
>>>>>> generated before adding #if/#endif lines on QAPIGenCSnippet
>>>>>> objects. Used in the following patches.
>>>>>>
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>> ---
>>>>>> scripts/qapi/common.py | 101 +++++++++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>>>>> index 1b56065a80..44eaf25850 100644
>>>>>> --- a/scripts/qapi/common.py
>>>>>> +++ b/scripts/qapi/common.py
>>>>>> @@ -12,6 +12,7 @@
>>>>>> # See the COPYING file in the top-level directory.
>>>>>>
>>>>>> from __future__ import print_function
>>>>>> +from contextlib import contextmanager
>>>>>> import errno
>>>>>> import os
>>>>>> import re
>>>>>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>>>>> name=guardname(name))
>>>>>>
>>>>>>
>>>>>> +def gen_if(ifcond):
>>>>>> + ret = ''
>>>>>> + for ifc in ifcond:
>>>>>> + ret += mcgen('''
>>>>>> +#if %(cond)s
>>>>>> +''', cond=ifc)
>>>>>> + return ret
>>>>>> +
>>>>>> +
>>>>>> +def gen_endif(ifcond):
>>>>>> + ret = ''
>>>>>> + for ifc in reversed(ifcond):
>>>>>> + ret += mcgen('''
>>>>>> +#endif /* %(cond)s */
>>>>>> +''', cond=ifc)
>>>>>> + return ret
>>>>>> +
>>>>>> +
>>>>>> +def wrap_ifcond(ifcond, before, after):
>>>>>
>>>>> Looks like a helper method. Name it _wrap_ifcond?
>>>>
>>>> Not fond of functions with _. Iirc, it was suggested by a linter to
>>>> make it a top-level function. I don't mind if you change it on commit.
>>>
>>> It's just how Python defines a module's public names. I don't like the
>>> proliferation of leading underscores either, but I feel it's best to go
>>> with the flow here.
>>
>> ok
>>
>>>
>>>>>> + if ifcond is None or before == after:
>>>>>> + return after
>>>>>
>>>>> The before == after part suppresses empty conditionals. Suggest
>>>>>
>>>>> return after # suppress empty #if ... #endif
>>>>>
>>>>> The ifcond is None part is suppresses "non-conditionals". How can this
>>>>> happen? If it can't, let's drop this part.
>>>>
>>>> because the function can be called without additional checks on ifcond
>>>> is None. I don't see what would be the benefit in moving this to the
>>>> caller responsibility.
>>>
>>> Why stop at None? Why not empty string? Empty containers other than
>>> []? False? Numeric zero?
>>>
>>> To answer my own question: because a precise function signatures reduce
>>> complexity. "The first argument is a list of strings, no ifs, no buts"
>>> is simpler than "the first argument may be None, or a list of I don't
>>> remember what exactly, but if you screw it up, the function hopefully
>>> throws up before it does real damage" ;)
>>
>> So you prefer if not ifcond or before == after ? ok
>
> I'd recommend
>
> + if before == after:
> + return after
>
> If we pass crap, we die in gen_if()'s for ifc in ifcond. The difference
> is now crap includes None.
>
makes sense, I didn't realize it no longers pass None.
>>>>> Another non-conditional is []. See below.
>>>>>
>>>>>> +
>>>>>> + assert after.startswith(before)
>>>>>> + out = before
>>>>>> + added = after[len(before):]
>>>>>> + if added[0] == '\n':
>>>>>> + out += '\n'
>>>>>> + added = added[1:]
>>>>>
>>>>> The conditional adjusts vertical space around #if. This might need
>>>>> tweaking, but let's not worry about that now.
>>>>>
>>>>>> + out += gen_if(ifcond)
>>>>>> + out += added
>>>>>> + out += gen_endif(ifcond)
>>>>>
>>>>> There's no similar adjustment for #endif. Again, let's not worry about
>>>>> that now.
>>>>>
>>>>>> + return out
>>>>>> +
>>>>>> +
>>>>>
>>>>> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
>>>>> after) returns after. Okay.
>>>>>
>>>>>> def gen_enum_lookup(name, values, prefix=None):
>>>>>> ret = mcgen('''
>>>>>>
>>>>>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>>>>> def __init__(self):
>>>>>> self._preamble = ''
>>>>>> self._body = ''
>>>>>> + self._start_if = None
>>>>>>
>>>>>> def preamble_add(self, text):
>>>>>> self._preamble += text
>>>>>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>>>>> def add(self, text):
>>>>>> self._body += text
>>>>>>
>>>>>> + def start_if(self, ifcond):
>>>>>> + assert self._start_if is None
>>>>>> +
>>>>>
>>>>> Let's drop this blank line.
>>>>
>>>> I prefer to have preconditions separated from the code, but ok
>>>
>>> I sympathize, but not if both are one-liners.
>>
>> ok
>>
>>>
>>>>>
>>>>>> + self._start_if = (ifcond, self._body, self._preamble)
>>>>>
>>>>> It's now obvious that you can't nest .start_if() ... .endif(). Good.
>>>>>
>>>>>> +
>>>>>> + def _wrap_ifcond(self):
>>>>>> + pass
>>>>>> +
>>>>>> + def end_if(self):
>>>>>> + assert self._start_if
>>>>>> +
>>>>>
>>>>> Let's drop this blank line.
>>>>>
>>>>>> + self._wrap_ifcond()
>>>>>> + self._start_if = None
>>>>>> +
>>>>>> + def get_content(self, fname=None):
>>>>>> + assert self._start_if is None
>>>>>> + return (self._top(fname) + self._preamble + self._body
>>>>>> + + self._bottom(fname))
>>>>>> +
>>>>>> def _top(self, fname):
>>>>>> return ''
>>>>>>
>>>>>
>>>>> Note for later: all this code has no effect unless ._wrap_ifcond() is
>>>>> overridden.
>>>>>
>>>>>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>>>>> f = open(fd, 'r+', encoding='utf-8')
>>>>>> else:
>>>>>> f = os.fdopen(fd, 'r+')
>>>>>> - text = (self._top(fname) + self._preamble + self._body
>>>>>> - + self._bottom(fname))
>>>>>> + text = self.get_content(fname)
>>>>>> oldtext = f.read(len(text) + 1)
>>>>>> if text != oldtext:
>>>>>> f.seek(0)
>>>>>> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>>>>>> f.close()
>>>>>>
>>>>>>
>>>>>> -class QAPIGenC(QAPIGen):
>>>>>> +@contextmanager
>>>>>> +def ifcontext(ifcond, *args):
>>>>>> + """A 'with' statement context manager to wrap with start_if()/end_if()
>>>>>>
>>>>>> - def __init__(self, blurb, pydoc):
>>>>>> + *args: variable length argument list of QAPIGen
>>>>>
>>>>> Perhaps: any number of QAPIGen objects
>>>>>
>>>>
>>>> sure
>>>>
>>>>>> +
>>>>>> + Example::
>>>>>> +
>>>>>> + with ifcontext(ifcond, self._genh, self._genc):
>>>>>> + modify self._genh and self._genc ...
>>>>>> +
>>>>>> + Is equivalent to calling::
>>>>>> +
>>>>>> + self._genh.start_if(ifcond)
>>>>>> + self._genc.start_if(ifcond)
>>>>>> + modify self._genh and self._genc ...
>>>>>> + self._genh.end_if()
>>>>>> + self._genc.end_if()
>>>>>> +
>>>>>
>>>>> Can we drop this blank line?
>>>>>
>>>>
>>>> I guess we can, I haven't tested the rst formatting this rigorously.
>>>> Hopefully the linter does it.
>>>>
>>>>>> + """
>>>>>> + for arg in args:
>>>>>> + arg.start_if(ifcond)
>>>>>> + yield
>>>>>> + for arg in args:
>>>>>> + arg.end_if()
>>>>>> +
>>>>>> +
>>>>>> +class QAPIGenCCode(QAPIGen):
>>>>>> +
>>>>>> + def __init__(self):
>>>>>> QAPIGen.__init__(self)
>>>>>> +
>>>>>> + def _wrap_ifcond(self):
>>>>>> + self._body = wrap_ifcond(self._start_if[0],
>>>>>> + self._start_if[1], self._body)
>>>>>> + self._preamble = wrap_ifcond(self._start_if[0],
>>>>>> + self._start_if[2], self._preamble)
>>>>>
>>>>> Can you explain why you put the machinery for conditionals in QAPIGen
>>>>> and not QAPIGenCCode?
>>>>
>>>> Iirc, this has to do with the fact that QAPIGenDoc is used for
>>>> visiting, and thus QAPIGen start_if()/end_if() could be handled there,
>>>
>>> Can you point me to calls of .start_if(), .end_if(), .get_content() for
>>> anything but QAPIGenCCode and its subtypes?
>>>
>>>> while we only want to generate #ifdef wrapping in QAPIGenCCode. I
>>>> guess I could move more of start_if()/end_if() data to QAPIGenCCode
>>>> (make them virtual / 'pass' in QAPIGen) Is that what you would like to
>>>> see?
>>>
>>> If there are no such calls, we should simply move the whole shebang to
>>> QAPIGenCCode.
>>
>> done
>
> Does that mean I should expect v7 from you?
yes
>
>>> If there are such calls, the common supertype QAPIGen has to provide the
>>> methods.
>>>
>>> If we expect subtypes other than QAPIGenCCode to implement conditional
>>> code generation the same way, except for a different ._wrap_ifcond(),
>>> then the patch is okay as is.
>>>
>>> If we don't, the transformation you described looks like an improvement
>>> to me, because it keeps the actual code closer together.
>>>
>>> What's your take on it?
>>>
>>> I think I could make the transformation when I apply.
>>>
>>>>>
>>>>>> +
>>>>>> +
>>>>>> +class QAPIGenC(QAPIGenCCode):
>>>>>> +
>>>>>> + def __init__(self, blurb, pydoc):
>>>>>> + QAPIGenCCode.__init__(self)
>>>>>> self._blurb = blurb
>>>>>> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>>>>>> re.MULTILINE))
>>>>>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2018-07-03 15:43 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-27 16:35 [Qemu-devel] [PATCH v6 00/15] qapi: add #if pre-processor conditions to generated code (part 1) Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions Marc-André Lureau
2018-06-28 14:57 ` Markus Armbruster
2018-06-28 15:34 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 02/15] tests Marc-André Lureau
2018-06-27 16:39 ` Marc-André Lureau
2018-06-28 15:35 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 03/15] qapi: pass 'if' condition into QAPISchemaEntity objects Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 04/15] qapi: leave the ifcond attribute undefined until check() Marc-André Lureau
2018-06-28 17:45 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 05/15] qapi: add 'ifcond' to visitor methods Marc-André Lureau
2018-06-28 17:48 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 06/15] qapi: mcgen() shouldn't indent # lines Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers Marc-André Lureau
2018-07-03 11:53 ` Markus Armbruster
2018-07-03 12:35 ` Marc-André Lureau
2018-07-03 13:37 ` Markus Armbruster
2018-07-03 15:08 ` Marc-André Lureau
2018-07-03 15:34 ` Markus Armbruster
2018-07-03 15:43 ` Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 08/15] qapi-introspect: modify to_qlit() to append ', ' on level > 0 Marc-André Lureau
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit Marc-André Lureau
2018-07-03 12:09 ` Markus Armbruster
2018-07-03 13:11 ` Marc-André Lureau
2018-07-03 13:47 ` Markus Armbruster
2018-07-03 15:08 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands Marc-André Lureau
2018-07-03 14:43 ` Markus Armbruster
2018-07-03 15:09 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 11/15] qapi/events: add #if conditions to events Marc-André Lureau
2018-07-03 14:47 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 12/15] qapi-types: add #if conditions to types & visitors Marc-André Lureau
2018-07-03 14:50 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 13/15] qapi: add 'If:' section to generated documentation Marc-André Lureau
2018-07-03 14:59 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema Marc-André Lureau
2018-07-03 15:04 ` Markus Armbruster
2018-06-27 16:35 ` [Qemu-devel] [PATCH v6 15/15] qapi: add conditions to SPICE " Marc-André Lureau
2018-07-03 15:05 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.