Re: [Qemu-devel] [PATCH v6 14/15] qapi: add conditions to VNC type/commands/events on the schema

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> query-qmp-schema no longer reports the command/events etc as
> available when disabled at compile.
>
> Commands made conditional:
>
> * query-vnc, query-vnc-servers, change-vnc-password
>
>   Before the patch, the commands for !CONFIG_VNC are stubs that fail
>   like this:
>
>     {"error": {"class": "GenericError",
>                "desc": "The feature 'vnc' is not enabled"}}
>
>   Afterwards, they fail like this:
>
>     {"error": {"class": "CommandNotFound",
>                "desc": "The command FOO has not been found"}}
>
>   I call that an improvement, because it lets clients distinguish
>   between command unavailable (class CommandNotFound) and command failed
>   (class GenericError).
>
> Events made conditional:
>
> * VNC_CONNECTED, VNC_INITIALIZED, VNC_DISCONNECTED
>
> HMP change:
>
> * info vnc
>
>   Will return "unknown command: 'info vnc'" when VNC is compiled
>   out (same as error for spice when --disable-spice)
>
> Occurrences of VNC (case insensitive) in the schema that aren't
> covered by this change:
>
> * add_client
>
>   Command has other uses, including "socket bases character devices".
>   These are unconditional as far as I can tell.
>
> * set_password, expire_password
>
>   In theory, these commands could be used for managing any service's
>   password.  In practice, they're used for VNC and SPICE services.
>   They're documented for "remote display session" / "remote display
>   server".
>
>   The service is selected by argument @protocol.  The code special-cases
>   protocol-specific argument checking, then calls a protocol-specific
>   function to do the work.  If it fails, the command fails with "Could
>   not set password".  It does when the service isn't compiled in (it's a
>   stub then).
>
>   We could make these commands conditional on the conjunction of all
>   services [currently: defined(CONFIG_VNC) || defined(CONFIG_SPICE)],
>   but I doubt it's worthwhile.
>
> * change
>
>   Command has other uses, namely changing media.
>   This patch inlines a stub; no functional change.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Gerd Hoffmann <address@hidden>

Reviewed-by: Markus Armbruster <address@hidden>