Re: [PATCH v5 28/69] qapi: Move definitions related to accelerators in their own file

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jul 16, 2025 at 10:23:26AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > Hi Markus,
> 
> I missed this one, sorry!
> 
> > On 3/7/25 12:54, Philippe Mathieu-Daudé wrote:
> >> Extract TCG and KVM definitions from machine.json to accelerator.json.
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> >> diff --git a/qapi/accelerator.json b/qapi/accelerator.json
> >> new file mode 100644
> >> index 00000000000..00d25427059
> >> --- /dev/null
> >> +++ b/qapi/accelerator.json
> >> @@ -0,0 +1,57 @@
> >> +# -*- Mode: Python -*-
> >> +# vim: filetype=python
> >> +#
> >> +# SPDX-License-Identifier: GPL-2.0-or-later
> >> +
> >> +##
> >> +# = Accelerators
> >> +##
> >> +
> >> +{ 'include': 'common.json' }
> >
> > common.json defines @HumanReadableText, ...
> >
> > [...]
> >
> >> +##
> >> +# @x-query-jit:
> >> +#
> >> +# Query TCG compiler statistics
> >> +#
> >> +# Features:
> >> +#
> >> +# @unstable: This command is meant for debugging.
> >> +#
> >> +# Returns: TCG compiler statistics
> >> +#
> >> +# Since: 6.2
> >> +##
> >> +{ 'command': 'x-query-jit',
> >> +  'returns': 'HumanReadableText',
> >> +  'if': 'CONFIG_TCG',
> >
> > ... which is *optionally* used here, triggering when
> > TCG is not built in:
> >
> > qapi/qapi-commands-accelerator.c:85:13: error: ‘qmp_marshal_output_HumanReadableText’ defined but not used [-Werror=unused-function]
> >    85 | static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
> >       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> 
> This is a defect in the QAPI code generator.  More below.
> 
> > We previously discussed that issue:
> > https://mail.gnu.org/archive/html/qemu-devel/2021-06/msg02667.html
> >
> > where you said:
> >
> > "conditional commands returning an unconditional type is a bit
> > of a code smell". Is it however a "non-smelly instances of this pattern"?
> 
> The instance discussed there wasn't.
> 
> You ran into it when you made TPM commands conditional on CONFIG_TPM
> without also making the types they return conditional.  The proper
> solution was to make the types conditional, too.  Avoided generating
> dead code.  I told you "The user is responsible for making T's 'if' the
> conjunction of the commands'."
> 
> Some of the commands returning HumanReadableText are unconditional, so
> said conjunction is also unconditional.  So how do we end up with unused
> qmp_marshal_output_HumanReadableText()?
> 
> A qmp_marshal_output_T() is only ever called by qmp_marshal_C() for a
> command C that returns T.
> 
> We've always generated it as a static function on demand, i.e. when we
> generate a call.

..snip..

> I need to ponder this to decide on a solution.

Functionally the redundat function is harmless, so the least effort
option is to change the generated QAPI headers to look like

  #pragma GCC diagnostic push
  #pragma GCC ignored "-Wunused-function"

  ... rest of QAPI header...

  #pragma GCC diagnostic pop

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|