Re: Can we unpoison CONFIG_FOO macros?

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 07/02/2023 16.39, Markus Armbruster wrote:
>> We have a boatload of CONFIG_FOO macros that may only be used in
>> target-dependent code.  We use generated config-poison.h to enforce.
>> This is a bit annoying in the QAPI schema.  Let me demonstrate with an
>> example: QMP commands query-rocker, query-rocker-ports, and so forth.
>> These commands are useful only with "rocker" devices.  They are
>> compile-time optional.  hw/net/Kconfig:
>>      config ROCKER
>>          bool
>>          default y if PCI_DEVICES
>>          depends on PCI && MSI_NONBROKEN
>> The rocker device and QMP code is actually target-independent:
>> hw/net/meson.build puts it into softmmu_ss.
>> Disabling the "rocker" device type ideally disables the rocker QMP
>> commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
>> schema.
>> Except that makes the entire code QAPI generates for rocker.json
>> device-dependent: it now contains #if defined(CONFIG_ROCKER), and
>> CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
>> commands also becomes device-dependent, because it includes generated
>> headers.  We compile all that per target for no sane reason at all.
>
> Well, CONFIG_ROCKER depends on the target config, so that is a sane reason in my eyes. Depending on which target you currently compile, it's either set or not set - there is no reasonable way to compile generic code and still test the CONFIG_ROCKER macro.
>
> So adding such a switch to rocker.json itself just cannot work.
>
> But at the "make" level, we distinguish between config-all-devices.mak and *-softmmu-config-devices.mak , so if you want to disable the rocker QMP code depending on whether CONFIG_ROCKER is set in *any* target binary or not, you would need a similar mechanism to config-all-devices.mak here, e.g. something that would switch the inclusion of rocker.json on or off depending on the switch in config-all-devices.mak ...
> Would it be possible to add such a switch to qapi/meson.build ?
> ... and to qapi/qapi-schema.json, I guess, since the rocker.json gets included from there? Something like that:
>
> diff --git a/qapi/meson.build b/qapi/meson.build
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -59,9 +59,11 @@ if have_system
>      'qdev',
>      'pci',
>      'rdma',
> -    'rocker',
>      'tpm',
>    ]
> +  if config_all_devices.has_key('CONFIG_ROCKER')
> +    qapi_all_modules += [ 'rocker', ]
> +  endif
>  endif
>  if have_system or have_tools
>    qapi_all_modules += [
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -72,7 +72,7 @@
>  { 'include': 'job.json' }
>  { 'include': 'net.json' }
>  { 'include': 'rdma.json' }
> -{ 'include': 'rocker.json' }
> +{ 'include': 'rocker.json', 'if': 'CONFIG_ROCKER' }
>  { 'include': 'tpm.json' }
>  { 'include': 'ui.json' }
>  { 'include': 'authz.json' }
>
> No clue whether the qapi parser could be extended like that, though...

Feels feasible.

I had vague ideas going this direction, too.  Thanks for fleshing them
out for me!

Hmm, we have somewhat hairy code in qapi/meson.build to enumerate the
QAPI modules and generated source files, and to add them to the right
source sets.  Perhaps we could have qapi-gen generate some of that.