[PATCH 0/6] qapi: Module fixes and cleanups

[PATCH 0/6] qapi: Module fixes and cleanups

[Markus Armbruster]

Kevin recently posted a minimally invasive fix for empty QAPI
modules[*].  This is my attempt at a fix that also addresses the
design weakness that led to the bug.

Markus Armbruster (6):
  qapi: Tweak "command returns a nice type" check for clarity
  tests/Makefile.include: Fix missing test-qapi-emit-events.[ch]
  qapi: Generate command registration stuff into separate files
  qapi: Proper intermediate representation for modules
  qapi: Fix code generation for empty modules
  qapi: Simplify QAPISchemaModularCVisitor

 docs/devel/qapi-code-gen.txt             | 19 ++++-
 Makefile                                 |  4 +-
 monitor/misc.c                           |  7 +-
 qga/main.c                               |  2 +-
 tests/test-qmp-cmds.c                    |  1 +
 .gitignore                               |  1 +
 qapi/Makefile.objs                       |  1 +
 qga/Makefile.objs                        |  1 +
 scripts/qapi/commands.py                 | 17 +++--
 scripts/qapi/events.py                   |  2 +-
 scripts/qapi/gen.py                      | 28 ++++----
 scripts/qapi/schema.py                   | 92 +++++++++++++++---------
 scripts/qapi/types.py                    |  5 +-
 scripts/qapi/visit.py                    |  8 +--
 tests/.gitignore                         |  1 +
 tests/Makefile.include                   |  9 ++-
 tests/qapi-schema/empty.out              |  1 +
 tests/qapi-schema/include-repetition.out |  6 +-
 tests/qapi-schema/qapi-schema-test.out   | 24 +++----
 19 files changed, 144 insertions(+), 85 deletions(-)

-- 
2.21.0

[PATCH 1/6] qapi: Tweak "command returns a nice type" check for clarity

[Markus Armbruster]

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cf0045f34e..cfb574c85d 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -711,10 +711,11 @@ class QAPISchemaCommand(QAPISchemaEntity):
             self.ret_type = schema.resolve_type(
                 self._ret_type_name, self.info, "command's 'returns'")
             if self.name not in self.info.pragma.returns_whitelist:
-                if not (isinstance(self.ret_type, QAPISchemaObjectType)
-                        or (isinstance(self.ret_type, QAPISchemaArrayType)
-                            and isinstance(self.ret_type.element_type,
-                                           QAPISchemaObjectType))):
+                typ = self.ret_type
+                if isinstance(typ, QAPISchemaArrayType):
+                    typ = self.ret_type.element_type
+                    assert typ
+                if not isinstance(typ, QAPISchemaObjectType):
                     raise QAPISemError(
                         self.info,
                         "command's 'returns' cannot take %s"
-- 
2.21.0

[PATCH 2/6] tests/Makefile.include: Fix missing test-qapi-emit-events.[ch]

[Markus Armbruster]

Commit 5d75648b56 "qapi: Generate QAPIEvent stuff into separate files"
added tests/test-qapi-emit-events.[ch] to the set of generated files,
but neglected to update tests/.gitignore and tests/Makefile.include.
Commit a0af8cee3c "tests/.gitignore: ignore test-qapi-emit-events.[ch]
for in-tree builds" fixed the former.  Now fix the latter.

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

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8566f5f119..75b377d1a9 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -503,6 +503,7 @@ generated-files-y += tests/test-qapi-visit-sub-sub-module.h
 generated-files-y += tests/test-qapi-commands.h
 generated-files-y += tests/include/test-qapi-commands-sub-module.h
 generated-files-y += tests/test-qapi-commands-sub-sub-module.h
+generated-files-y += tests/test-qapi-emit-events.h
 generated-files-y += tests/test-qapi-events.h
 generated-files-y += tests/include/test-qapi-events-sub-module.h
 generated-files-y += tests/test-qapi-events-sub-sub-module.h
@@ -610,6 +611,7 @@ tests/include/test-qapi-commands-sub-module.h \
 tests/include/test-qapi-commands-sub-module.c \
 tests/test-qapi-commands-sub-sub-module.h \
 tests/test-qapi-commands-sub-sub-module.c \
+tests/test-qapi-emit-events.c tests/test-qapi-emit-events.h \
 tests/test-qapi-events.c tests/test-qapi-events.h \
 tests/include/test-qapi-events-sub-module.c \
 tests/include/test-qapi-events-sub-module.h \
@@ -637,7 +639,7 @@ tests/qapi-schema/doc-good.test.texi: $(SRC_PATH)/tests/qapi-schema/doc-good.jso
 
 tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(test-qapi-obj-y)
 tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y)
-tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) tests/test-qapi-events.o
+tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) tests/test-qapi-emit-events.o tests/test-qapi-events.o
 tests/test-qobject-output-visitor$(EXESUF): tests/test-qobject-output-visitor.o $(test-qapi-obj-y)
 tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y)
 tests/test-qobject-input-visitor$(EXESUF): tests/test-qobject-input-visitor.o $(test-qapi-obj-y)
-- 
2.21.0

[PATCH 3/6] qapi: Generate command registration stuff into separate files

[Markus Armbruster]

Having to include qapi-commands.h just for qmp_init_marshal() is
suboptimal.  Generate it into separate files.  This lets
monitor/misc.c, qga/main.c, and the generated qapi-commands-FOO.h
include less.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 19 ++++++++++++++++---
 Makefile                     |  4 +++-
 monitor/misc.c               |  7 ++++++-
 qga/main.c                   |  2 +-
 tests/test-qmp-cmds.c        |  1 +
 .gitignore                   |  1 +
 qapi/Makefile.objs           |  1 +
 qga/Makefile.objs            |  1 +
 scripts/qapi/commands.py     | 15 +++++++++++----
 tests/.gitignore             |  1 +
 tests/Makefile.include       |  5 ++++-
 11 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 45c93a43cc..3f37339d16 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1493,6 +1493,10 @@ $(prefix)qapi-commands.c: Command marshal/dispatch functions for each
 $(prefix)qapi-commands.h: Function prototypes for the QMP commands
                           specified in the schema
 
+$(prefix)qapi-init-commands.h - Command initialization prototype
+
+$(prefix)qapi-init-commands.h - Command initialization code
+
 Example:
 
     $ cat qapi-generated/example-qapi-commands.h
@@ -1502,11 +1506,9 @@ Example:
     #define EXAMPLE_QAPI_COMMANDS_H
 
     #include "example-qapi-types.h"
-    #include "qapi/qmp/dispatch.h"
 
     UserDefOne *qmp_my_command(UserDefOneList *arg1, Error **errp);
     void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp);
-    void example_qmp_init_marshal(QmpCommandList *cmds);
 
     #endif /* EXAMPLE_QAPI_COMMANDS_H */
     $ cat qapi-generated/example-qapi-commands.c
@@ -1566,7 +1568,19 @@ Example:
         visit_end_struct(v, NULL);
         visit_free(v);
     }
+[Uninteresting stuff omitted...]
+    $ cat qapi-generated/example-qapi-init-commands.h
+[Uninteresting stuff omitted...]
+    #ifndef EXAMPLE_QAPI_INIT_COMMANDS_H
+    #define EXAMPLE_QAPI_INIT_COMMANDS_H
 
+    #include "qapi/qmp/dispatch.h"
+
+    void example_qmp_init_marshal(QmpCommandList *cmds);
+
+    #endif /* EXAMPLE_QAPI_INIT_COMMANDS_H */
+    $ cat qapi-generated/example-qapi-init-commands.
+[Uninteresting stuff omitted...]
     void example_qmp_init_marshal(QmpCommandList *cmds)
     {
         QTAILQ_INIT(cmds);
@@ -1574,7 +1588,6 @@ Example:
         qmp_register_command(cmds, "my-command",
                              qmp_marshal_my_command, QCO_NO_OPTIONS);
     }
-
 [Uninteresting stuff omitted...]
 
 For a modular QAPI schema (see section Include directives), code for
diff --git a/Makefile b/Makefile
index b437a346d7..8dad949483 100644
--- a/Makefile
+++ b/Makefile
@@ -117,6 +117,7 @@ GENERATED_QAPI_FILES += qapi/qapi-builtin-visit.h qapi/qapi-builtin-visit.c
 GENERATED_QAPI_FILES += qapi/qapi-visit.h qapi/qapi-visit.c
 GENERATED_QAPI_FILES += $(QAPI_MODULES:%=qapi/qapi-visit-%.h)
 GENERATED_QAPI_FILES += $(QAPI_MODULES:%=qapi/qapi-visit-%.c)
+GENERATED_QAPI_FILES += qapi/qapi-init-commands.h qapi/qapi-init-commands.c
 GENERATED_QAPI_FILES += qapi/qapi-commands.h qapi/qapi-commands.c
 GENERATED_QAPI_FILES += $(QAPI_MODULES:%=qapi/qapi-commands-%.h)
 GENERATED_QAPI_FILES += $(QAPI_MODULES:%=qapi/qapi-commands-%.c)
@@ -610,6 +611,7 @@ $(SRC_PATH)/scripts/qapi-gen.py
 qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
 qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \
 qga/qapi-generated/qga-qapi-commands.h qga/qapi-generated/qga-qapi-commands.c \
+qga/qapi-generated/qga-qapi-init-commands.h qga/qapi-generated/qga-qapi-init-commands.c \
 qga/qapi-generated/qga-qapi-doc.texi: \
 qga/qapi-generated/qapi-gen-timestamp ;
 qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json $(qapi-py)
@@ -628,7 +630,7 @@ qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
 		"GEN","$(@:%-timestamp=%)")
 	@>$@
 
-QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h)
+QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h)
 $(qga-obj-y): $(QGALIB_GEN)
 
 qemu-ga$(EXESUF): $(qga-obj-y) $(COMMON_LDADDS)
diff --git a/monitor/misc.c b/monitor/misc.c
index 3baa15f3bf..7c4b599342 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -66,8 +66,13 @@
 #include "qemu/option.h"
 #include "qemu/thread.h"
 #include "block/qapi.h"
-#include "qapi/qapi-commands.h"
+#include "qapi/qapi-commands-char.h"
+#include "qapi/qapi-commands-migration.h"
+#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-commands-trace.h"
 #include "qapi/qapi-emit-events.h"
+#include "qapi/qapi-init-commands.h"
 #include "qapi/error.h"
 #include "qapi/qmp-event.h"
 #include "qapi/qapi-introspect.h"
diff --git a/qga/main.c b/qga/main.c
index c35c2a2120..e5c39c189a 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -24,7 +24,7 @@
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qstring.h"
 #include "guest-agent-core.h"
-#include "qga-qapi-commands.h"
+#include "qga-qapi-init-commands.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "channel.h"
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 27b0afe55a..79507d9e54 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -7,6 +7,7 @@
 #include "tests/test-qapi-types.h"
 #include "tests/test-qapi-visit.h"
 #include "test-qapi-commands.h"
+#include "test-qapi-init-commands.h"
 
 static QmpCommandList qmp_commands;
 
diff --git a/.gitignore b/.gitignore
index 7de868d1ea..efad605e1a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -37,6 +37,7 @@
 /qapi/qapi-emit-events.[ch]
 /qapi/qapi-events-*.[ch]
 /qapi/qapi-events.[ch]
+/qapi/qapi-init-commands.[ch]
 /qapi/qapi-introspect.[ch]
 /qapi/qapi-types-*.[ch]
 /qapi/qapi-types.[ch]
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index dd3f5e6f94..a8f1f4c35e 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -30,3 +30,4 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
 obj-y += qapi-events.o
 obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
 obj-y += qapi-commands.o
+obj-y += qapi-init-commands.o
diff --git a/qga/Makefile.objs b/qga/Makefile.objs
index 80e6bb3c2e..9c558ae51c 100644
--- a/qga/Makefile.objs
+++ b/qga/Makefile.objs
@@ -5,5 +5,6 @@ qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o
 qga-obj-$(CONFIG_WIN32) += vss-win32.o
 qga-obj-y += qapi-generated/qga-qapi-types.o qapi-generated/qga-qapi-visit.o
 qga-obj-y += qapi-generated/qga-qapi-commands.o
+qga-obj-y += qapi-generated/qga-qapi-init-commands.o
 
 qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32/
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index ab98e504f3..47f4a18cfe 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -263,18 +263,25 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
                              commands=commands, visit=visit))
         self._genh.add(mcgen('''
 #include "%(types)s.h"
-#include "qapi/qmp/dispatch.h"
 
 ''',
                              types=types))
 
     def visit_end(self):
-        (genc, genh) = self._module[self._main_module]
-        genh.add(mcgen('''
+        self._add_system_module('init', ' * QAPI Commands initialization')
+        self._genh.add(mcgen('''
+#include "qapi/qmp/dispatch.h"
+
 void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 ''',
                        c_prefix=c_name(self._prefix, protect=False)))
-        genc.add(gen_registry(self._regy.get_content(), self._prefix))
+        self._genc.preamble_add(mcgen('''
+#include "qemu/osdep.h"
+#include "%(prefix)sqapi-commands.h"
+#include "%(prefix)sqapi-init-commands.h"
+''',
+                                      prefix=self._prefix))
+        self._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,
diff --git a/tests/.gitignore b/tests/.gitignore
index f9c0170881..7306866f21 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -12,6 +12,7 @@ test-*
 !test-*.c
 !docker/test-*
 test-qapi-commands.[ch]
+test-qapi-init-commands.[ch]
 include/test-qapi-commands-sub-module.[ch]
 test-qapi-commands-sub-sub-module.[ch]
 test-qapi-emit-events.[ch]
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 75b377d1a9..ce854ee556 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -501,6 +501,7 @@ generated-files-y += tests/test-qapi-visit.h
 generated-files-y += tests/include/test-qapi-visit-sub-module.h
 generated-files-y += tests/test-qapi-visit-sub-sub-module.h
 generated-files-y += tests/test-qapi-commands.h
+generated-files-y += tests/test-qapi-init-commands.h
 generated-files-y += tests/include/test-qapi-commands-sub-module.h
 generated-files-y += tests/test-qapi-commands-sub-sub-module.h
 generated-files-y += tests/test-qapi-emit-events.h
@@ -613,6 +614,8 @@ tests/test-qapi-commands-sub-sub-module.h \
 tests/test-qapi-commands-sub-sub-module.c \
 tests/test-qapi-emit-events.c tests/test-qapi-emit-events.h \
 tests/test-qapi-events.c tests/test-qapi-events.h \
+tests/test-qapi-init-commands.c \
+tests/test-qapi-init-commands.h \
 tests/include/test-qapi-events-sub-module.c \
 tests/include/test-qapi-events-sub-module.h \
 tests/test-qapi-events-sub-sub-module.c \
@@ -643,7 +646,7 @@ tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) tests/t
 tests/test-qobject-output-visitor$(EXESUF): tests/test-qobject-output-visitor.o $(test-qapi-obj-y)
 tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y)
 tests/test-qobject-input-visitor$(EXESUF): tests/test-qobject-input-visitor.o $(test-qapi-obj-y)
-tests/test-qmp-cmds$(EXESUF): tests/test-qmp-cmds.o tests/test-qapi-commands.o $(test-qapi-obj-y)
+tests/test-qmp-cmds$(EXESUF): tests/test-qmp-cmds.o tests/test-qapi-commands.o tests/test-qapi-init-commands.o $(test-qapi-obj-y)
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
 tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
 
-- 
2.21.0

[PATCH 4/6] qapi: Proper intermediate representation for modules

[Markus Armbruster]

Modules are represented only by their names so far.  Introduce class
QAPISchemaModule.  So far, it merely wraps the name.  The next patch
will put it to more interesting use.

Once again, arrays spice up the patch a bit.  For any other type,
@info points to the definition, which lets us map from @info to
module.  For arrays, there is no definition, and @info points to the
first use instead.  We have to use the element type's module instead,
which is only available after .check().

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cfb574c85d..0f2e0dcfce 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -50,9 +50,6 @@ class QAPISchemaEntity(object):
 
     def check(self, schema):
         assert not self._checked
-        if self.info:
-            self._module = os.path.relpath(self.info.fname,
-                                           os.path.dirname(schema.fname))
         seen = {}
         for f in self.features:
             f.check_clash(self.info, seen)
@@ -68,6 +65,13 @@ class QAPISchemaEntity(object):
         if self.doc:
             self.doc.check()
 
+    def _set_module(self, schema, info):
+        assert self._checked
+        self._module = schema.module_by_fname(info and info.fname)
+
+    def set_module(self, schema):
+        self._set_module(schema, self.info)
+
     @property
     def ifcond(self):
         assert self._checked
@@ -75,7 +79,7 @@ class QAPISchemaEntity(object):
 
     @property
     def module(self):
-        assert self._checked
+        assert self._module or not self.info
         return self._module
 
     def is_implicit(self):
@@ -135,15 +139,19 @@ class QAPISchemaVisitor(object):
         pass
 
 
+class QAPISchemaModule(object):
+    def __init__(self, name):
+        self.name = name
+
+
 class QAPISchemaInclude(QAPISchemaEntity):
-
-    def __init__(self, fname, info):
+    def __init__(self, sub_module, info):
         QAPISchemaEntity.__init__(self, None, info, None)
-        self.fname = fname
+        self._sub_module = sub_module
 
     def visit(self, visitor):
         QAPISchemaEntity.visit(self, visitor)
-        visitor.visit_include(self.fname, self.info)
+        visitor.visit_include(self._sub_module.name, self.info)
 
 
 class QAPISchemaType(QAPISchemaEntity):
@@ -276,16 +284,14 @@ class QAPISchemaArrayType(QAPISchemaType):
             self.info and self.info.defn_meta)
         assert not isinstance(self.element_type, QAPISchemaArrayType)
 
+    def set_module(self, schema):
+        self._set_module(schema, self.element_type.info)
+
     @property
     def ifcond(self):
         assert self._checked
         return self.element_type.ifcond
 
-    @property
-    def module(self):
-        assert self._checked
-        return self.element_type.module
-
     def is_implicit(self):
         return True
 
@@ -783,6 +789,10 @@ class QAPISchema(object):
         self.docs = parser.docs
         self._entity_list = []
         self._entity_dict = {}
+        self._module_dict = {}
+        self._schema_dir = os.path.dirname(fname)
+        self._make_module(None) # built-ins
+        self._make_module(fname)
         self._predefining = True
         self._def_predefineds()
         self._predefining = False
@@ -826,14 +836,26 @@ class QAPISchema(object):
                 info, "%s uses unknown type '%s'" % (what, name))
         return typ
 
+    def _module_name(self, fname):
+        if fname is None:
+            return None
+        return os.path.relpath(fname, self._schema_dir)
+
+    def _make_module(self, fname):
+        name = self._module_name(fname)
+        if not name in self._module_dict:
+            self._module_dict[name] = QAPISchemaModule(name)
+        return self._module_dict[name]
+
+    def module_by_fname(self, fname):
+        name = self._module_name(fname)
+        assert name in self._module_dict
+        return self._module_dict[name]
+
     def _def_include(self, expr, info, doc):
         include = expr['include']
         assert doc is None
-        main_info = info
-        while main_info.parent:
-            main_info = main_info.parent
-        fname = os.path.relpath(include, os.path.dirname(main_info.fname))
-        self._def_entity(QAPISchemaInclude(fname, info))
+        self._def_entity(QAPISchemaInclude(self._make_module(include), info))
 
     def _def_builtin_type(self, name, json_type, c_type):
         self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
@@ -1065,15 +1087,16 @@ class QAPISchema(object):
             ent.check(self)
             ent.connect_doc()
             ent.check_doc()
+        for ent in self._entity_list:
+            ent.set_module(self)
 
     def visit(self, visitor):
         visitor.visit_begin(self)
         module = None
-        visitor.visit_module(module)
         for entity in self._entity_list:
             if visitor.visit_needed(entity):
                 if entity.module != module:
                     module = entity.module
-                    visitor.visit_module(module)
+                    visitor.visit_module(module.name)
                 entity.visit(visitor)
         visitor.visit_end()
-- 
2.21.0

[PATCH 5/6] qapi: Fix code generation for empty modules

[Markus Armbruster]

When a sub-module doesn't contain any definitions, we don't generate
code for it, but we do generate the #include.

We generate code only for modules that get visited.
QAPISchema.visit() visits only modules that have definitions.  It can
visit modules multiple times.

Clean this up as follows.  Collect entities in their QAPISchemaModule.
Have QAPISchema.visit() call QAPISchemaModule.visit() for each module.
Have QAPISchemaModule.visit() call .visit_module() for itself, and
QAPISchemaEntity.visit() for each of its entities.  This way, we visit
each module exactly once.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/schema.py                   | 24 +++++++++++++-----------
 tests/qapi-schema/empty.out              |  1 +
 tests/qapi-schema/include-repetition.out |  6 ++----
 tests/qapi-schema/qapi-schema-test.out   | 24 ++++++++++--------------
 4 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0f2e0dcfce..0bfc5256fb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -68,6 +68,7 @@ class QAPISchemaEntity(object):
     def _set_module(self, schema, info):
         assert self._checked
         self._module = schema.module_by_fname(info and info.fname)
+        self._module.add_entity(self)
 
     def set_module(self, schema):
         self._set_module(schema, self.info)
@@ -77,11 +78,6 @@ class QAPISchemaEntity(object):
         assert self._checked
         return self._ifcond
 
-    @property
-    def module(self):
-        assert self._module or not self.info
-        return self._module
-
     def is_implicit(self):
         return not self.info
 
@@ -142,6 +138,16 @@ class QAPISchemaVisitor(object):
 class QAPISchemaModule(object):
     def __init__(self, name):
         self.name = name
+        self._entity_list = []
+
+    def add_entity(self, ent):
+        self._entity_list.append(ent)
+
+    def visit(self, visitor):
+        visitor.visit_module(self.name)
+        for entity in self._entity_list:
+            if visitor.visit_needed(entity):
+                entity.visit(visitor)
 
 
 class QAPISchemaInclude(QAPISchemaEntity):
@@ -1093,10 +1099,6 @@ class QAPISchema(object):
     def visit(self, visitor):
         visitor.visit_begin(self)
         module = None
-        for entity in self._entity_list:
-            if visitor.visit_needed(entity):
-                if entity.module != module:
-                    module = entity.module
-                    visitor.visit_module(module.name)
-                entity.visit(visitor)
+        for mod in self._module_dict.values():
+            mod.visit(visitor)
         visitor.visit_end()
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 5b53d00702..69666c39ad 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -9,3 +9,4 @@ enum QType
     member qdict
     member qlist
     member qbool
+module empty.json
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index 5423983239..0b654ddebb 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -11,15 +11,13 @@ enum QType
     member qbool
 module include-repetition.json
 include comments.json
+include include-repetition-sub.json
+include comments.json
 module comments.json
 enum Status
     member good
     member bad
     member ugly
-module include-repetition.json
-include include-repetition-sub.json
 module include-repetition-sub.json
 include comments.json
 include comments.json
-module include-repetition.json
-include comments.json
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3660e75a48..9bd3c4a490 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -153,9 +153,6 @@ object q_obj_sizeList-wrapper
     member data: sizeList optional=False
 object q_obj_anyList-wrapper
     member data: anyList optional=False
-module sub-sub-module.json
-array StatusList Status
-module qapi-schema-test.json
 object q_obj_StatusList-wrapper
     member data: StatusList optional=False
 enum UserDefListUnionKind
@@ -193,17 +190,6 @@ object UserDefListUnion
     case any: q_obj_anyList-wrapper
     case user: q_obj_StatusList-wrapper
 include include/sub-module.json
-module include/sub-module.json
-include sub-sub-module.json
-module sub-sub-module.json
-enum Status
-    member good
-    member bad
-    member ugly
-module include/sub-module.json
-object SecondArrayRef
-    member s: StatusList optional=False
-module qapi-schema-test.json
 command user_def_cmd None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_user_def_cmd1-arg
@@ -435,3 +421,13 @@ command test-command-cond-features3 None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
     feature feature1
         if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
+module include/sub-module.json
+include sub-sub-module.json
+object SecondArrayRef
+    member s: StatusList optional=False
+module sub-sub-module.json
+array StatusList Status
+enum Status
+    member good
+    member bad
+    member ugly
-- 
2.21.0

[PATCH 6/6] qapi: Simplify QAPISchemaModularCVisitor

[Markus Armbruster]

Since the previous commit, QAPISchemaVisitor.visit_module() is called
just once.  Simplify QAPISchemaModularCVisitor accordingly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/commands.py |  2 +-
 scripts/qapi/events.py   |  2 +-
 scripts/qapi/gen.py      | 28 ++++++++++++++--------------
 scripts/qapi/types.py    |  5 +++--
 scripts/qapi/visit.py    |  8 ++++----
 5 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 47f4a18cfe..afa55b055c 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -239,7 +239,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
     def __init__(self, prefix):
         QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-commands',
-            ' * Schema-defined QAPI/QMP commands', __doc__)
+            ' * Schema-defined QAPI/QMP commands', None, __doc__)
         self._regy = QAPIGenCCode(None)
         self._visited_ret_types = {}
 
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 10fc509fa9..2bde3e6128 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -140,7 +140,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
     def __init__(self, prefix):
         QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-events',
-            ' * Schema-defined QAPI/QMP events', __doc__)
+            ' * Schema-defined QAPI/QMP events', None, __doc__)
         self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
         self._event_enum_members = []
         self._event_emit_name = c_name(prefix + 'qapi_event_emit')
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 112b6d94c5..95afae0615 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -201,10 +201,11 @@ class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
 
 class QAPISchemaModularCVisitor(QAPISchemaVisitor):
 
-    def __init__(self, prefix, what, blurb, pydoc):
+    def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
         self._prefix = prefix
         self._what = what
-        self._blurb = blurb
+        self._user_blurb = user_blurb
+        self._builtin_blurb = builtin_blurb
         self._pydoc = pydoc
         self._genc = None
         self._genh = None
@@ -245,7 +246,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
         self._module[name] = (genc, genh)
-        self._set_module(name)
+        self._genc, self._genh = self._module[name]
 
     def _add_user_module(self, name, blurb):
         assert self._is_user_module(name)
@@ -256,9 +257,6 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
     def _add_system_module(self, name, blurb):
         self._add_module(name and './' + name, blurb)
 
-    def _set_module(self, name):
-        self._genc, self._genh = self._module[name]
-
     def write(self, output_dir, opt_builtins=False):
         for name in self._module:
             if self._is_builtin_module(name) and not opt_builtins:
@@ -271,15 +269,17 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
         pass
 
     def visit_module(self, name):
-        if name in self._module:
-            self._set_module(name)
-        elif self._is_builtin_module(name):
-            # The built-in module has not been created.  No code may
-            # be generated.
-            self._genc = None
-            self._genh = None
+        if name is None:
+            if self._builtin_blurb:
+                self._add_system_module(None, self._builtin_blurb)
+                self._begin_system_module(name)
+            else:
+                # The built-in module has not been created.  No code may
+                # be generated.
+                self._genc = None
+                self._genh = None
         else:
-            self._add_user_module(name, self._blurb)
+            self._add_user_module(name, self._user_blurb)
             self._begin_user_module(name)
 
     def visit_include(self, name, info):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index d8751daa04..99dcaf7074 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -243,8 +243,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
     def __init__(self, prefix):
         QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-types', ' * Schema-defined QAPI types',
-            __doc__)
-        self._add_system_module(None, ' * Built-in QAPI types')
+            ' * Built-in QAPI types', __doc__)
+
+    def _begin_system_module(self, name):
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c72f2bc5c0..4efce62b0c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -285,8 +285,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
     def __init__(self, prefix):
         QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
-            __doc__)
-        self._add_system_module(None, ' * Built-in QAPI visitors')
+            ' * Built-in QAPI visitors', __doc__)
+
+    def _begin_system_module(self, name):
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/error.h"
@@ -296,8 +297,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
 #include "qapi/visitor.h"
 #include "qapi/qapi-builtin-types.h"
 
-''',
-                                      prefix=prefix))
+'''))
 
     def _begin_user_module(self, name):
         types = self._module_basename('qapi-types', name)
-- 
2.21.0

Re: [PATCH 1/6] qapi: Tweak "command returns a nice type" check for clarity

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/schema.py | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

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

> 
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cf0045f34e..cfb574c85d 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -711,10 +711,11 @@ class QAPISchemaCommand(QAPISchemaEntity):
>               self.ret_type = schema.resolve_type(
>                   self._ret_type_name, self.info, "command's 'returns'")
>               if self.name not in self.info.pragma.returns_whitelist:
> -                if not (isinstance(self.ret_type, QAPISchemaObjectType)
> -                        or (isinstance(self.ret_type, QAPISchemaArrayType)
> -                            and isinstance(self.ret_type.element_type,
> -                                           QAPISchemaObjectType))):
> +                typ = self.ret_type
> +                if isinstance(typ, QAPISchemaArrayType):
> +                    typ = self.ret_type.element_type
> +                    assert typ
> +                if not isinstance(typ, QAPISchemaObjectType):
>                       raise QAPISemError(
>                           self.info,
>                           "command's 'returns' cannot take %s"
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/6] tests/Makefile.include: Fix missing test-qapi-emit-events.[ch]

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> Commit 5d75648b56 "qapi: Generate QAPIEvent stuff into separate files"
> added tests/test-qapi-emit-events.[ch] to the set of generated files,
> but neglected to update tests/.gitignore and tests/Makefile.include.
> Commit a0af8cee3c "tests/.gitignore: ignore test-qapi-emit-events.[ch]
> for in-tree builds" fixed the former.  Now fix the latter.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/Makefile.include | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/6] qapi: Generate command registration stuff into separate files

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> Having to include qapi-commands.h just for qmp_init_marshal() is
> suboptimal.  Generate it into separate files.  This lets
> monitor/misc.c, qga/main.c, and the generated qapi-commands-FOO.h
> include less.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/docs/devel/qapi-code-gen.txt
> @@ -1493,6 +1493,10 @@ $(prefix)qapi-commands.c: Command marshal/dispatch functions for each
>   $(prefix)qapi-commands.h: Function prototypes for the QMP commands
>                             specified in the schema
>   
> +$(prefix)qapi-init-commands.h - Command initialization prototype
> +
> +$(prefix)qapi-init-commands.h - Command initialization code

Looks like you meant s/h/c/


> +    #endif /* EXAMPLE_QAPI_INIT_COMMANDS_H */
> +    $ cat qapi-generated/example-qapi-init-commands.
> +[Uninteresting stuff omitted...]

missing a 'c'

> +++ b/Makefile

>   
> -QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h)
> +QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h)

Worth using \ for line-wrapping?

With those addressed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/6] qapi: Module fixes and cleanups

[Markus Armbruster]

Fat-fingered Kevin's e-mail address...

Markus Armbruster <armbru@redhat.com> writes:

> Kevin recently posted a minimally invasive fix for empty QAPI
> modules[*].  This is my attempt at a fix that also addresses the
> design weakness that led to the bug.
>
> Markus Armbruster (6):
>   qapi: Tweak "command returns a nice type" check for clarity
>   tests/Makefile.include: Fix missing test-qapi-emit-events.[ch]
>   qapi: Generate command registration stuff into separate files
>   qapi: Proper intermediate representation for modules
>   qapi: Fix code generation for empty modules
>   qapi: Simplify QAPISchemaModularCVisitor
>
>  docs/devel/qapi-code-gen.txt             | 19 ++++-
>  Makefile                                 |  4 +-
>  monitor/misc.c                           |  7 +-
>  qga/main.c                               |  2 +-
>  tests/test-qmp-cmds.c                    |  1 +
>  .gitignore                               |  1 +
>  qapi/Makefile.objs                       |  1 +
>  qga/Makefile.objs                        |  1 +
>  scripts/qapi/commands.py                 | 17 +++--
>  scripts/qapi/events.py                   |  2 +-
>  scripts/qapi/gen.py                      | 28 ++++----
>  scripts/qapi/schema.py                   | 92 +++++++++++++++---------
>  scripts/qapi/types.py                    |  5 +-
>  scripts/qapi/visit.py                    |  8 +--
>  tests/.gitignore                         |  1 +
>  tests/Makefile.include                   |  9 ++-
>  tests/qapi-schema/empty.out              |  1 +
>  tests/qapi-schema/include-repetition.out |  6 +-
>  tests/qapi-schema/qapi-schema-test.out   | 24 +++----
>  19 files changed, 144 insertions(+), 85 deletions(-)

Re: [PATCH 4/6] qapi: Proper intermediate representation for modules

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> Modules are represented only by their names so far.  Introduce class
> QAPISchemaModule.  So far, it merely wraps the name.  The next patch
> will put it to more interesting use.
> 
> Once again, arrays spice up the patch a bit.  For any other type,
> @info points to the definition, which lets us map from @info to
> module.  For arrays, there is no definition, and @info points to the
> first use instead.  We have to use the element type's module instead,
> which is only available after .check().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/schema.py | 63 ++++++++++++++++++++++++++++--------------
>   1 file changed, 43 insertions(+), 20 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 5/6] qapi: Fix code generation for empty modules

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> When a sub-module doesn't contain any definitions, we don't generate
> code for it, but we do generate the #include.
> 
> We generate code only for modules that get visited.
> QAPISchema.visit() visits only modules that have definitions.  It can
> visit modules multiple times.
> 
> Clean this up as follows.  Collect entities in their QAPISchemaModule.
> Have QAPISchema.visit() call QAPISchemaModule.visit() for each module.
> Have QAPISchemaModule.visit() call .visit_module() for itself, and
> QAPISchemaEntity.visit() for each of its entities.  This way, we visit
> each module exactly once.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/6] qapi: Simplify QAPISchemaModularCVisitor

[Eric Blake]

On 11/20/19 12:25 PM, Markus Armbruster wrote:
> Since the previous commit, QAPISchemaVisitor.visit_module() is called
> just once.  Simplify QAPISchemaModularCVisitor accordingly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/6] qapi: Module fixes and cleanups

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191120182551.23795-1-armbru@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    iotest-qcow2: 268
Failures: 192
Failed 1 of 108 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7206e3a304c44c7187ff2b4aa7fff8d6', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-wk8w7tot/src/docker-src.2019-11-20-18.46.04.18266:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7206e3a304c44c7187ff2b4aa7fff8d6
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-wk8w7tot/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    12m12.040s
user    0m8.724s


The full log is available at
http://patchew.org/logs/20191120182551.23795-1-armbru@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/6] qapi: Module fixes and cleanups

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin recently posted a minimally invasive fix for empty QAPI
> modules[*].  This is my attempt at a fix that also addresses the
> design weakness that led to the bug.

[*] Subject: [RFC PATCH 15/18] qapi: Support empty modules
Message-Id: <20191017130204.16131-16-kwolf@redhat.com>

Re: [PATCH 3/6] qapi: Generate command registration stuff into separate files

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/20/19 12:25 PM, Markus Armbruster wrote:
>> Having to include qapi-commands.h just for qmp_init_marshal() is
>> suboptimal.  Generate it into separate files.  This lets
>> monitor/misc.c, qga/main.c, and the generated qapi-commands-FOO.h
>> include less.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/docs/devel/qapi-code-gen.txt
>> @@ -1493,6 +1493,10 @@ $(prefix)qapi-commands.c: Command marshal/dispatch functions for each
>>   $(prefix)qapi-commands.h: Function prototypes for the QMP commands
>>                             specified in the schema
>>   +$(prefix)qapi-init-commands.h - Command initialization prototype
>> +
>> +$(prefix)qapi-init-commands.h - Command initialization code
>
> Looks like you meant s/h/c/

I did.  Thanks!

>> +    #endif /* EXAMPLE_QAPI_INIT_COMMANDS_H */
>> +    $ cat qapi-generated/example-qapi-init-commands.
>> +[Uninteresting stuff omitted...]
>
> missing a 'c'

Yes.

>> +++ b/Makefile
>
>>   -QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h
>> qga-qapi-visit.h qga-qapi-commands.h)
>> +QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h)
>
> Worth using \ for line-wrapping?

Hmm, it's used only in the following line, could just as well inline
there and line-wrap that:

    $(qga-obj-y): $(QGALIB_GEN)

Hmm^2, why do we even have that line?  The compiler-generated .d should
take care of dependencies.  Dig, dig, ...

    commit 016c77ad62a8ad607dd4349d8cb8ad1365bab831
    Author: Michael Roth <mdroth@linux.vnet.ibm.com>
    Date:   Tue Jul 26 11:39:24 2011 -0500

        Makefile: add missing deps on $(GENERATED_HEADERS)

        This fixes a build issue with make -j6+ due to qapi-generated files
        being built before $(GENERATED_HEADERS) have been created.

        Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
        Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
        Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Yes, but the same issue exists for all the other generated headers, and
we solve it differently: put them into $(generated-files-y), have
Makefile depend on them.

I'll clean this up on top.  No need to beautify lines now that will go
away then.

> With those addressed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Markus Armbruster]

Generated .h need to be generated before compiling any .c using them.
To know which .h a .c uses, we need to compile it.

Since commit 4115852bb0 "build: do not sprinkle around
GENERATED_HEADERS dependencies", we break this circular dependency the
simple & stupid way: the generated headers are a prerequisite of
Makefile, which causes Make to generate them first, then start over.

Except for qga we still use the older method of making all its .o
summarily depend on all its generated .h (commit 016c77ad62 "Makefile:
add missing deps on $(GENERATED_HEADERS)").

Add qga's generated files to generated-files-y to get rid of this
exception.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 Makefile | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 8dad949483..d4138343cd 100644
--- a/Makefile
+++ b/Makefile
@@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
 
 generated-files-y += $(GENERATED_QAPI_FILES)
 
+GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
+GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
+GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
+GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
+GENERATED_QGA_FILES += qga-qapi-doc.texi
+GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))
+
+generated-files-y += $(GENERATED_QGA_FILES)
+
 generated-files-y += trace/generated-tcg-tracers.h
 
 generated-files-y += trace/generated-helpers-wrappers.h
@@ -608,12 +617,7 @@ $(SRC_PATH)/scripts/qapi/types.py \
 $(SRC_PATH)/scripts/qapi/visit.py \
 $(SRC_PATH)/scripts/qapi-gen.py
 
-qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
-qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \
-qga/qapi-generated/qga-qapi-commands.h qga/qapi-generated/qga-qapi-commands.c \
-qga/qapi-generated/qga-qapi-init-commands.h qga/qapi-generated/qga-qapi-init-commands.c \
-qga/qapi-generated/qga-qapi-doc.texi: \
-qga/qapi-generated/qapi-gen-timestamp ;
+$(GENERATED_QGA_FILES): qga/qapi-generated/qapi-gen-timestamp ;
 qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
 		-o qga/qapi-generated -p "qga-" $<, \
@@ -630,9 +634,6 @@ qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
 		"GEN","$(@:%-timestamp=%)")
 	@>$@
 
-QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h)
-$(qga-obj-y): $(QGALIB_GEN)
-
 qemu-ga$(EXESUF): $(qga-obj-y) $(COMMON_LDADDS)
 	$(call LINK, $^)
 
@@ -722,7 +723,7 @@ clean: recurse-clean
 	rm -f trace/generated-tracers-dtrace.h*
 	rm -f $(foreach f,$(generated-files-y),$(f) $(f)-timestamp)
 	rm -f qapi-gen-timestamp
-	rm -rf qga/qapi-generated
+	rm -f qga/qapi-generated/qapi-gen-timestamp
 	rm -f config-all-devices.mak
 
 VERSION ?= $(shell cat VERSION)
-- 
2.21.0

Re: [PATCH 0/6] qapi: Module fixes and cleanups

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin recently posted a minimally invasive fix for empty QAPI
> modules[*].  This is my attempt at a fix that also addresses the
> design weakness that led to the bug.

Queued for 5.0.

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Eric Blake]

On 11/29/19 3:59 AM, Markus Armbruster wrote:
> Generated .h need to be generated before compiling any .c using them.
> To know which .h a .c uses, we need to compile it.
> 
> Since commit 4115852bb0 "build: do not sprinkle around
> GENERATED_HEADERS dependencies", we break this circular dependency the
> simple & stupid way: the generated headers are a prerequisite of
> Makefile, which causes Make to generate them first, then start over.

which is a pain when using 'make --dry-run' to see what would get built 
(a dependency of Makefile _is_ rebuilt if Makefile itself has to be 
updated), but not the fault of this patch.

> 
> Except for qga we still use the older method of making all its .o
> summarily depend on all its generated .h (commit 016c77ad62 "Makefile:
> add missing deps on $(GENERATED_HEADERS)").
> 
> Add qga's generated files to generated-files-y to get rid of this
> exception.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   Makefile | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)

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


> +++ b/Makefile
> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
>   
>   generated-files-y += $(GENERATED_QAPI_FILES)
>   
> +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
> +GENERATED_QGA_FILES += qga-qapi-doc.texi
> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))

Would it be worth using two separate variable names (maybe 
GENERATED_QGA_BASEFILES for the first list) rather than exploiting the 
arcane knowledge that consecutive use of := causes GNU make to rewrite 
an existing variable with new contents?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/29/19 3:59 AM, Markus Armbruster wrote:
>> Generated .h need to be generated before compiling any .c using them.
>> To know which .h a .c uses, we need to compile it.
>>
>> Since commit 4115852bb0 "build: do not sprinkle around
>> GENERATED_HEADERS dependencies", we break this circular dependency the
>> simple & stupid way: the generated headers are a prerequisite of
>> Makefile, which causes Make to generate them first, then start over.
>
> which is a pain when using 'make --dry-run' to see what would get
> built (a dependency of Makefile _is_ rebuilt if Makefile itself has to
> be updated), but not the fault of this patch.

Yes.

>> Except for qga we still use the older method of making all its .o
>> summarily depend on all its generated .h (commit 016c77ad62 "Makefile:
>> add missing deps on $(GENERATED_HEADERS)").
>>
>> Add qga's generated files to generated-files-y to get rid of this
>> exception.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   Makefile | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

>> +++ b/Makefile
>> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
>>     generated-files-y += $(GENERATED_QAPI_FILES)
>>   +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
>> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
>> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
>> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
>> +GENERATED_QGA_FILES += qga-qapi-doc.texi
>> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))
>
> Would it be worth using two separate variable names (maybe
> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
> arcane knowledge that consecutive use of := causes GNU make to rewrite
> an existing variable with new contents?

Our rules.mak relies on this already.  It's full of magic, which
admittedly diminishes its suitability to serve as a good example.

Your worry might be rooted in old "=" burns.  "=" makes the variable
recursively expanded, and

    GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))

would be an infinite loop.  ":=" makes it simply expanded; there's not
even a loop, let alone an infinite one.  The GNU Make manual explains
this clearly at
https://www.gnu.org/software/make/manual/html_node/Flavors.html

Aside: there's a reason one of the two flavors is called "simple".  It
could additionally be called "not as slow".  One of my pet makefile
peeves: unthinking use of recursively expanded variables, complicating
semantics and slowing down builds.

Back to this patch.  I had started to write the thing in longhand, but
got tired of repeating qga/qapi-generated/, so I factored that out.
Would longhand be easier to understand?

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Eric Blake]

On 12/4/19 12:56 AM, Markus Armbruster wrote:

>>> +++ b/Makefile
>>> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
>>>      generated-files-y += $(GENERATED_QAPI_FILES)
>>>    +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
>>> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
>>> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
>>> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
>>> +GENERATED_QGA_FILES += qga-qapi-doc.texi
>>> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))
>>
>> Would it be worth using two separate variable names (maybe
>> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
>> arcane knowledge that consecutive use of := causes GNU make to rewrite
>> an existing variable with new contents?
> 
> Our rules.mak relies on this already.  It's full of magic, which
> admittedly diminishes its suitability to serve as a good example.
> 
> Your worry might be rooted in old "=" burns.  "=" makes the variable
> recursively expanded, and
> 
>      GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))

Indeed, but I have to refer to the manual to remind myself of whether = 
or := is what I want in a given situation.

> 
> would be an infinite loop.  ":=" makes it simply expanded; there's not
> even a loop, let alone an infinite one.  The GNU Make manual explains
> this clearly at
> https://www.gnu.org/software/make/manual/html_node/Flavors.html
> 
> Aside: there's a reason one of the two flavors is called "simple".  It
> could additionally be called "not as slow".  One of my pet makefile
> peeves: unthinking use of recursively expanded variables, complicating
> semantics and slowing down builds.
> 
> Back to this patch.  I had started to write the thing in longhand, but
> got tired of repeating qga/qapi-generated/, so I factored that out.
> Would longhand be easier to understand?

It's more verbose.  My suggestion was more:

GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
...
GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, 
$(GENERATED_QGA_BASENAMES))

to avoid the reassignment-to-self issue altogether, while still 
remaining concise compared to longhand.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/4/19 12:56 AM, Markus Armbruster wrote:
>
>>>> +++ b/Makefile
>>>> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
>>>>      generated-files-y += $(GENERATED_QAPI_FILES)
>>>>    +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
>>>> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
>>>> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
>>>> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
>>>> +GENERATED_QGA_FILES += qga-qapi-doc.texi
>>>> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))
>>>
>>> Would it be worth using two separate variable names (maybe
>>> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
>>> arcane knowledge that consecutive use of := causes GNU make to rewrite
>>> an existing variable with new contents?
>>
>> Our rules.mak relies on this already.  It's full of magic, which
>> admittedly diminishes its suitability to serve as a good example.
>>
>> Your worry might be rooted in old "=" burns.  "=" makes the variable
>> recursively expanded, and
>>
>>      GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))
>
> Indeed, but I have to refer to the manual to remind myself of whether
> = or := is what I want in a given situation.

Trust me, you're either sure you want "=", or you want ":=".

On a green field, I recommend a hard rule "no = without a comment
explaining why".

>> would be an infinite loop.  ":=" makes it simply expanded; there's not
>> even a loop, let alone an infinite one.  The GNU Make manual explains
>> this clearly at
>> https://www.gnu.org/software/make/manual/html_node/Flavors.html
>>
>> Aside: there's a reason one of the two flavors is called "simple".  It
>> could additionally be called "not as slow".  One of my pet makefile
>> peeves: unthinking use of recursively expanded variables, complicating
>> semantics and slowing down builds.
>>
>> Back to this patch.  I had started to write the thing in longhand, but
>> got tired of repeating qga/qapi-generated/, so I factored that out.
>> Would longhand be easier to understand?
>
> It's more verbose.  My suggestion was more:
>
> GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
> GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
> ...
> GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/,
> $(GENERATED_QGA_BASENAMES))
>
> to avoid the reassignment-to-self issue altogether, while still
> remaining concise compared to longhand.

Either way, we use multiple assignments to build GENERATED_QGA_FILES.
The only difference is that the version using two variables would also
work with recursive expansion, due to the magic of +=.