Re: [Qemu-devel] [PATCH for-4.0 v7 26/27] qapi: add more conditions to SPICE

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Now that member can be made conditional, let's make SPICE chardev
> conditional:
>
> * spiceport, spicevmc
>
>   Before and after the patch for !CONFIG_SPICE, the error is the
>   same ('spiceport' is not a valid char driver name).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/char.json | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index 24628331ec..77ed847972 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -332,8 +332,8 @@
>  ##
>  { 'struct': 'ChardevSpiceChannel',
>    'data': { 'type': 'str' },
> -  'base': 'ChardevCommon' }
> -# TODO: 'if': 'defined(CONFIG_SPICE)'
> +  'base': 'ChardevCommon',
> +  'if': 'defined(CONFIG_SPICE)' }
>  
>  ##
>  # @ChardevSpicePort:
> @@ -346,8 +346,8 @@
>  ##
>  { 'struct': 'ChardevSpicePort',
>    'data': { 'fqdn': 'str' },
> -  'base': 'ChardevCommon' }
> -# TODO: 'if': 'defined(CONFIG_SPICE)'
> +  'base': 'ChardevCommon',
> +  'if': 'defined(CONFIG_SPICE)' }
>  
>  ##
>  # @ChardevVC:
> @@ -404,10 +404,10 @@
>              'testdev': 'ChardevCommon',
>              'stdio': 'ChardevStdio',
>              'console': 'ChardevCommon',
> -            'spicevmc': 'ChardevSpiceChannel',
> -# TODO: { 'type': 'ChardevSpiceChannel', 'if': 'defined(CONFIG_SPICE)' },
> -            'spiceport': 'ChardevSpicePort',
> -# TODO: { 'type': 'ChardevSpicePort', 'if': 'defined(CONFIG_SPICE)' },
> +            'spicevmc': { 'type': 'ChardevSpiceChannel',
> +                          'if': 'defined(CONFIG_SPICE)' },
> +            'spiceport': { 'type': 'ChardevSpicePort',
> +                           'if': 'defined(CONFIG_SPICE)' },
>              'vc': 'ChardevVC',
>              'ringbuf': 'ChardevRingbuf',
>              # next one is just for compatibility

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Reviewing this patch's effect on generated files led me to a few things.
None of them need to be addressed to get this series accepted.  I
actually recommend not to address them in this series, because that
could lead to further delays.

There's an opportunity for minor output beautification: back-to-back
#endif / #if like

    #endif /* defined(CONFIG_SPICE) */
    #if defined(CONFIG_SPICE)

in generated C could be omitted.

The generated documentation shows conditionals as

    If: 'defined(CONFIG_SPICE)'

We should at least explain this notation in the introduction.  Other
notation, too.  Predates this series.

Should we *strip* documentation for disabled stuff instead?
Complication: disabled everywhere, or just for a target?  We build the
documentation only once, and that's a feature.  We can therefore only
strip documentation for stuff that's disabled target-independently.
That's a bit more involved.