Re: [Qemu-devel] [PATCH v2 53/54] qapi: make query-cpu-model-expansion depend on s390 or x86

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Aug 23, 2017 at 06:21:16AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Aug 22, 2017 at 03:22:54PM +0200, Marc-André Lureau wrote:
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  qapi-schema.json                       |  4 +++-
> > >  include/sysemu/arch_init.h             |  3 ---
> > >  monitor.c                              |  3 ---
> > >  qmp.c                                  |  7 -------
> > >  stubs/arch-query-cpu-model-expansion.c | 12 ------------
> > >  target/i386/cpu.c                      |  2 +-
> > >  target/s390x/cpu_models.c              |  3 ++-
> > >  stubs/Makefile.objs                    |  1 -
> > >  8 files changed, 6 insertions(+), 29 deletions(-)
> > >  delete mode 100644 stubs/arch-query-cpu-model-expansion.c
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 6c1adb35b5..127a2c71c6 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4535,7 +4535,9 @@
> > >  { 'command': 'query-cpu-model-expansion',
> > >    'data': { 'type': 'CpuModelExpansionType',
> > >              'model': 'CpuModelInfo' },
> > > -  'returns': 'CpuModelExpansionInfo' }
> > > +  'returns': 'CpuModelExpansionInfo',
> > > +  'if': ['defined(NEED_CPU_H)',
> > > +         'defined(TARGET_S390X) || defined(TARGET_I386)']}
> > 
> > Maybe this is already documented somewhere in the series (I'm
> > still going through the other patches), but: why exactly is
> > 'defined(NEED_CPU_H)' in the list, too?
> 
> The point of this series is to make qapi schema configurable.
> 
> Some types/commands/events are target-specifc. In order to
> #ifdef on poisoined symbols, we make most of QAPI generated
> code built per-target in patch 49/54. But the common code still
> need to compile some units, that's why #ifdef NEED_CPU_U. The
> clean solution is probably to split the generated schema to
> common & per-target, that's not covered in the series.

I see.  I'm worried about one thing: what if the QMP dispatch
code is incorrectly moved back to common code later?  It will
silently skip all the arch-dependent commands without any
warnings.

e.g.: I just applied the following change on top of your series,
and all arch-dependent commands are silently skipped:

  diff --git a/Makefile.objs b/Makefile.objs
  index 2664720..24a4ea0 100644
  --- a/Makefile.objs
  +++ b/Makefile.objs
  @@ -2,7 +2,7 @@
   # Common libraries for tools and emulators
   stub-obj-y = stubs/ crypto/
   util-obj-y = util/ qobject/ qapi/
  -util-obj-y += qapi-types.o qapi-visit.o
  +util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
   
   chardev-obj-y = chardev/
   
  @@ -72,6 +72,13 @@ common-obj-y += chardev/
   common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
   
   common-obj-$(CONFIG_FDT) += device_tree.o
  +
  +######################################################################
  +# qapi
  +
  +common-obj-y += qmp-marshal.o
  +common-obj-y += qmp-introspect.o
  +common-obj-y += qmp.o hmp.o
   endif
   
   #######################################################################
  diff --git a/Makefile.target b/Makefile.target
  index c5f8ded..7f42c45 100644
  --- a/Makefile.target
  +++ b/Makefile.target
  @@ -157,10 +157,6 @@ endif
   
   GENERATED_FILES += hmp-commands.h hmp-commands-info.h
   
  -obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
  -obj-y += qmp-marshal.o qmp-introspect.o
  -obj-y += qmp.o hmp.o
  -
   endif # CONFIG_SOFTMMU
   
   # Workaround for http://gcc.gnu.org/PR55489, see configure.


We wouldn't need to check NEED_CPU_H at all if the condition on
QMP commands affect only the actual QMP dispatch code, and not
the generated types or declarations.  e.g.:


diff --git a/qapi-schema.json b/qapi-schema.json
index 194859f..67b46ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3578,7 +3578,7 @@
 ##
 { 'command': 'dump-skeys',
   'data': { 'filename': 'str' },
-  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
+  'if': ['defined(TARGET_S390X)']}
 
 ##
 # @netdev_add:
@@ -4434,8 +4434,7 @@
 # Since: 1.2.0
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
-  'if': ['defined(NEED_CPU_H)',
-         'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)'] }
+  'if': ['defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)'] }
 
 ##
 # @CpuModelInfo:
@@ -4538,8 +4537,7 @@
   'data': { 'type': 'CpuModelExpansionType',
             'model': 'CpuModelInfo' },
   'returns': 'CpuModelExpansionInfo',
-  'if': ['defined(NEED_CPU_H)',
-         'defined(TARGET_S390X) || defined(TARGET_I386)']}
+  'if': ['defined(TARGET_S390X) || defined(TARGET_I386)']}
 
 ##
 # @CpuModelCompareResult:
@@ -4627,7 +4625,7 @@
 { 'command': 'query-cpu-model-comparison',
   'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
   'returns': 'CpuModelCompareInfo',
-  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
+  'if': ['defined(TARGET_S390X)']}
 
 
 ##
@@ -4681,7 +4679,7 @@
   'data': { 'modela': 'CpuModelInfo',
             'modelb': 'CpuModelInfo' },
   'returns': 'CpuModelBaselineInfo',
-  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
+  'if': ['defined(TARGET_S390X)']}
 
 ##
 # @AddfdInfo:
@@ -6303,7 +6301,7 @@
 #
 ##
 { 'command': 'rtc-reset-reinjection',
-  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_I386)'] }
+  'if': ['defined(TARGET_I386)'] }
 
 # Rocker ethernet network switch
 { 'include': 'qapi/rocker.json' }
@@ -6466,7 +6464,7 @@
 #
 ##
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
-  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_ARM)']}
+  'if': ['defined(TARGET_ARM)']}
 
 ##
 # @CpuInstanceProperties:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index bcd8d37..769a730 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1720,8 +1720,7 @@ class QAPISchema(object):
         ifcond = expr.get('if')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, doc, 'arg', self._make_members(data, info),
-                ifcond)
+                name, info, doc, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index db45415..2baa73f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -228,7 +228,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
         self.defn = None
         self._regy = None
         self._visited_ret_types = None
-        self.if_members = ['decl', 'defn', '_regy']
+        self.if_members = ['defn', '_regy']
 
     def visit_begin(self, schema):
         self.decl = ''


-- 
Eduardo