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

[Markus Armbruster]

Copying our resident VNC maintainer^Wodd fixer Gerd.

Also copying Dan for QCryptoCipherAlgorithm.

Gerd, Dan, this patch is about making VNC support visible in
query-qmp-schema, by having the QAPI generators generate suitable
ifdeffery.  Bonus: no need for QMP command stubs for
!defined(CONFIG_VNC).

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

> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  qapi-schema.json | 34 ++++++++++++++++++++++------------
>  qapi/event.json  |  9 ++++++---
>  hmp.c            | 14 +++++++++++++-
>  qmp.c            | 30 ++++--------------------------
>  4 files changed, 45 insertions(+), 42 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9c6c3e1a53..829c66f9eb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1660,7 +1660,8 @@
>    'data': { 'host': 'str',
>              'service': 'str',
>              'family': 'NetworkAddressFamily',
> -            'websocket': 'bool' } }
> +            'websocket': 'bool' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VncServerInfo:
> @@ -1674,7 +1675,8 @@
>  ##
>  { 'struct': 'VncServerInfo',
>    'base': 'VncBasicInfo',
> -  'data': { '*auth': 'str' } }
> +  'data': { '*auth': 'str' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VncClientInfo:
> @@ -1691,7 +1693,8 @@
>  ##
>  { 'struct': 'VncClientInfo',
>    'base': 'VncBasicInfo',
> -  'data': { '*x509_dname': 'str', '*sasl_username': 'str' } }
> +  'data': { '*x509_dname': 'str', '*sasl_username': 'str' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VncInfo:
> @@ -1732,7 +1735,8 @@
>  { 'struct': 'VncInfo',
>    'data': {'enabled': 'bool', '*host': 'str',
>             '*family': 'NetworkAddressFamily',
> -           '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} 
> }
> +           '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']},
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VncPrimaryAuth:
> @@ -1743,7 +1747,8 @@
>  ##
>  { 'enum': 'VncPrimaryAuth',
>    'data': [ 'none', 'vnc', 'ra2', 'ra2ne', 'tight', 'ultra',
> -            'tls', 'vencrypt', 'sasl' ] }
> +            'tls', 'vencrypt', 'sasl' ],
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VncVencryptSubAuth:
> @@ -1757,7 +1762,8 @@
>              'tls-none',  'x509-none',
>              'tls-vnc',   'x509-vnc',
>              'tls-plain', 'x509-plain',
> -            'tls-sasl',  'x509-sasl' ] }
> +            'tls-sasl',  'x509-sasl' ],
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  
>  ##
> @@ -1775,7 +1781,8 @@
>  { 'struct': 'VncServerInfo2',
>    'base': 'VncBasicInfo',
>    'data': { 'auth'      : 'VncPrimaryAuth',
> -            '*vencrypt' : 'VncVencryptSubAuth' } }
> +            '*vencrypt' : 'VncVencryptSubAuth' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  
>  ##
> @@ -1808,7 +1815,8 @@
>              'clients'   : ['VncClientInfo'],
>              'auth'      : 'VncPrimaryAuth',
>              '*vencrypt' : 'VncVencryptSubAuth',
> -            '*display'  : 'str' } }
> +            '*display'  : 'str' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @query-vnc:
> @@ -1839,7 +1847,8 @@
>  #    }
>  #
>  ##
> -{ 'command': 'query-vnc', 'returns': 'VncInfo' }
> +{ 'command': 'query-vnc', 'returns': 'VncInfo',
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @query-vnc-servers:
> @@ -1850,7 +1859,8 @@
>  #
>  # Since: 2.3
>  ##
> -{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'] }
> +{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'],
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @SpiceBasicInfo:
> @@ -3077,8 +3087,8 @@
>  # Notes:  An empty password in this command will set the password to the 
> empty
>  #         string.  Existing clients are unaffected by executing this command.
>  ##
> -{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> -
> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'},
> +  'if': 'defined(CONFIG_VNC)' }
>  ##
>  # @change:
>  #
> diff --git a/qapi/event.json b/qapi/event.json
> index 6d22b025cc..c8b8e9f384 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -263,7 +263,8 @@
>  ##
>  { 'event': 'VNC_CONNECTED',
>    'data': { 'server': 'VncServerInfo',
> -            'client': 'VncBasicInfo' } }
> +            'client': 'VncBasicInfo' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VNC_INITIALIZED:
> @@ -290,7 +291,8 @@
>  ##
>  { 'event': 'VNC_INITIALIZED',
>    'data': { 'server': 'VncServerInfo',
> -            'client': 'VncClientInfo' } }
> +            'client': 'VncClientInfo' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @VNC_DISCONNECTED:
> @@ -316,7 +318,8 @@
>  ##
>  { 'event': 'VNC_DISCONNECTED',
>    'data': { 'server': 'VncServerInfo',
> -            'client': 'VncClientInfo' } }
> +            'client': 'VncClientInfo' },
> +  'if': 'defined(CONFIG_VNC)' }
>  
>  ##
>  # @SPICE_CONNECTED:
> diff --git a/hmp.c b/hmp.c
> index fd80dce758..9454c634bd 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>      qapi_free_BlockStatsList(stats_list);
>  }
>  
> +#ifdef CONFIG_VNC
>  /* Helper for hmp_info_vnc_clients, _servers */
>  static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
>                                    const char *name)
> @@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>      qapi_free_VncInfo2List(info2l);
>  
>  }
> +#else
> +void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> +{
> +    warn_report("VNC support is disabled");
> +}
> +#endif
>  
>  #ifdef CONFIG_SPICE
>  void hmp_info_spice(Monitor *mon, const QDict *qdict)
> @@ -1708,12 +1715,14 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +#ifdef CONFIG_VNC
>  static void hmp_change_read_arg(void *opaque, const char *password,
>                                  void *readline_opaque)
>  {
>      qmp_change_vnc_password(password, NULL);
>      monitor_read_command(opaque, 1);
>  }
> +#endif
>  
>  void hmp_change(Monitor *mon, const QDict *qdict)
>  {
> @@ -1724,6 +1733,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>      BlockdevChangeReadOnlyMode read_only_mode = 0;
>      Error *err = NULL;
>  
> +#ifdef CONFIG_VNC
>      if (strcmp(device, "vnc") == 0) {
>          if (read_only) {
>              monitor_printf(mon,
> @@ -1738,7 +1748,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>              }
>          }
>          qmp_change("vnc", target, !!arg, arg, &err);
> -    } else {
> +    } else
> +#endif
> +    {
>          if (read_only) {
>              read_only_mode =
>                  qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,

On HMP, see my reply to Dave's review.

> diff --git a/qmp.c b/qmp.c
> index b86201e349..2c90dacb56 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -130,22 +130,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
>      }
>  }
>  
> -#ifndef CONFIG_VNC
> -/* If VNC support is enabled, the "true" query-vnc command is
> -   defined in the VNC subsystem */
> -VncInfo *qmp_query_vnc(Error **errp)
> -{
> -    error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -    return NULL;
> -};
> -
> -VncInfo2List *qmp_query_vnc_servers(Error **errp)
> -{
> -    error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -    return NULL;
> -};
> -#endif
> -
>  #ifndef CONFIG_SPICE
>  /*
>   * qmp-commands.hx ensures that QMP command query-spice exists only
> @@ -403,23 +387,17 @@ static void qmp_change_vnc(const char *target, bool 
> has_arg, const char *arg,
>          qmp_change_vnc_listen(target, errp);
>      }
>  }
> -#else
> -void qmp_change_vnc_password(const char *password, Error **errp)
> -{
> -    error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
> -static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> -                           Error **errp)
> -{
> -    error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
>  #endif /* !CONFIG_VNC */
>  
>  void qmp_change(const char *device, const char *target,
>                  bool has_arg, const char *arg, Error **errp)
>  {
>      if (strcmp(device, "vnc") == 0) {
> +#ifdef CONFIG_VNC
>          qmp_change_vnc(target, has_arg, arg, errp);
> +#else
> +        error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> +#endif
>      } else {
>          qmp_blockdev_change_medium(true, device, false, NULL, target,
>                                     has_arg, arg, false, 0, errp);

Commands you make 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 you make conditional:

* VNC_CONNECTED, VNC_INITIALIZED, VNC_DISCONNECTED

Now let me check for completeness.  Occurrences of VNC (case
insensitive) in the schema that aren't covered by your changes:

* add_client

  Command has other uses, including "socket bases character devices".
  These are unconditional as far as I can tell.  Good.

* 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.

  Okay.

* change

  Command has other uses, namely changing media.

  Your patch inlines a stub; no functional change.  Good.

* QCryptoCipherAlgorithm

  This:

    # @des-rfb: RFB specific variant of single DES. Do not use except in VNC.

  I guess we could compile this out if we wanted to.  I doubt we do, but
  Dan might have other ideas.

Some of this analysis should perhaps be worked into the commit message.

Overall, the schema syntax works nicely for me.  A bit on the verbose
side perhaps, but I like that the conditions are locally obvious.

Observation: we got >250 lines of VNC stuff in qapi-schema.json.  Moving
them into qapi/vnc.json would permit proper MAINTAINERS coverage.  Gerd,
what do you think?