Re: [PATCH 7/7] spice: Call qemu spice functions indirectly

[Dr. David Alan Gilbert]

* Christophe de Dinechin (dinechin@redhat.com) wrote:
> In order to be able to move all of spice in a shared library that is
> loaded dynamically only on demand, this commit puts the functions
> called by the main binary in a QemuSpiceOps structure, and use static
> inline stubs to preserve the call sites.
> 
> With these changes, the following shared libraries are no longer
> necessary in the top-level qemu binary when building with CONFIG_MDULES:
> 
>         libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
>         libopus.so.0 => /lib64/libopus.so.0 (HEX)
>         liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
>         libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
>         libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
>         libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
>         libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
>         libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
>         liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/ui/qemu-spice.h | 74 +++++++++++++++++++----------------------
>  monitor/hmp-cmds.c      | 13 ++++++++
>  monitor/misc.c          |  2 +-
>  monitor/qmp-cmds.c      |  6 ++--
>  softmmu/vl.c            | 28 ++++++++++++++--
>  ui/spice-core.c         | 33 ++++++++++++++++--
>  6 files changed, 107 insertions(+), 49 deletions(-)
> 
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index d34cea2e0f..9721d8817e 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -25,21 +25,28 @@
>  #include <spice.h>
>  #include "qemu/config-file.h"
>  
> -extern int using_spice;
> +#define using_spice     (qemu_spice.in_use())
>  
> -void qemu_spice_init(void);
>  void qemu_spice_input_init(void);
>  void qemu_spice_audio_init(void);
> -void qemu_spice_display_init(void);
> -int qemu_spice_display_add_client(int csock, int skipauth, int tls);
>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>  bool qemu_spice_have_display_interface(QemuConsole *con);
>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
> +
> +#if !defined(CONFIG_MODULES) || defined(BUILD_DSO)
> +bool qemu_is_using_spice(void);
> +void qemu_start_using_spice(void);
> +void qemu_spice_init(void);
> +void qemu_spice_display_init(void);
> +int qemu_spice_display_add_client(int csock, int skipauth, int tls);
>  int qemu_spice_set_passwd(const char *passwd,
>                            bool fail_if_connected, bool 
> disconnect_if_connected);
>  int qemu_spice_set_pw_expire(time_t expires);
>  int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
>                              const char *subject);
> +struct SpiceInfo *qemu_spice_query(Error **errp);
> +#endif /* !CONFIG_MODULES || BUILD_DSO */
> +
>  
>  #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
>  #define SPICE_NEEDS_SET_MM_TIME 1
> @@ -47,46 +54,33 @@ int qemu_spice_migrate_info(const char *hostname, int 
> port, int tls_port,
>  #define SPICE_NEEDS_SET_MM_TIME 0
>  #endif
>  
> -#else  /* CONFIG_SPICE */
> -
> -#include "qemu/error-report.h"
> -
> -#define using_spice 0
> -#define spice_displays 0
> -static inline int qemu_spice_set_passwd(const char *passwd,
> -                                        bool fail_if_connected,
> -                                        bool disconnect_if_connected)
> -{
> -    return -1;
> -}
> -static inline int qemu_spice_set_pw_expire(time_t expires)
> -{
> -    return -1;
> -}
> -static inline int qemu_spice_migrate_info(const char *h, int p, int t,
> -                                          const char *s)
> -{
> -    return -1;
> -}
> +#else /* !CONFIG_SPICE */
>  
> -static inline int qemu_spice_display_add_client(int csock, int skipauth,
> -                                                int tls)
> -{
> -    return -1;
> -}
> +#define using_spice     0
>  
> -static inline void qemu_spice_display_init(void)
> -{
> -    /* This must never be called if CONFIG_SPICE is disabled */
> -    error_report("spice support is disabled");
> -    abort();
> -}
> +#endif /* CONFIG_SPICE */
>  
> -static inline void qemu_spice_init(void)
> +/* Module high-level inteface for SPICE */
> +struct QemuSpiceOps
>  {
> -}
> -
> -#endif /* CONFIG_SPICE */
> +    bool (*in_use)(void);
> +    void (*init)(void);
> +    void (*display_init)(void);
> +    int (*display_add_client)(int csock, int skipauth, int tls);
> +
> +    int (*set_passwd)(const char *passwd,
> +                      bool fail_if_connected,
> +                      bool disconnect_if_connected);
> +    int (*set_pw_expire)(time_t expires);
> +    int (*migrate_info)(const char *hostname,
> +                        int port, int tls_port,
> +                        const char *subject);
> +    struct SpiceInfo * (*query)(Error **errp);
> +};
> +typedef struct QemuSpiceOps QemuSpiceOps;
> +void qemu_spice_ops_register(QemuSpiceOps *ops);
> +
> +extern struct QemuSpiceOps qemu_spice;
>  
>  static inline bool qemu_using_spice(Error **errp)
>  {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ae4b6a4246..8e10af188f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -57,6 +57,7 @@
>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> +#include "ui/qemu-spice.h"
>  #endif
>  
>  void hmp_handle_error(Monitor *mon, Error *err)
> @@ -573,6 +574,14 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>  #endif
>  
>  #ifdef CONFIG_SPICE
> +SpiceInfo *qmp_query_spice(Error **errp)
> +{
> +    if (!using_spice) {
> +        return NULL;
> +    }
> +    return qemu_spice.query(errp);
> +}
> +

It's a bit weird to put the qmp function in the hmp file.

>  void hmp_info_spice(Monitor *mon, const QDict *qdict)
>  {
>      SpiceChannelList *chan;
> @@ -599,6 +608,10 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
>  
>      info = qmp_query_spice(NULL);
>  
> +    if (!info) {
> +        monitor_printf(mon, "Server: not started\n");
> +        goto out;
> +    }
>      if (!info->enabled) {
>          monitor_printf(mon, "Server: disabled\n");
>          goto out;

Dave

> diff --git a/monitor/misc.c b/monitor/misc.c
> index e847b58a8c..88f8915734 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -443,7 +443,7 @@ void qmp_client_migrate_info(const char *protocol, const 
> char *hostname,
>              return;
>          }
>  
> -        if (qemu_spice_migrate_info(hostname,
> +        if (qemu_spice.migrate_info(hostname,
>                                      has_port ? port : -1,
>                                      has_tls_port ? tls_port : -1,
>                                      cert_subject)) {
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 864cbfa32e..9f9822b3f3 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -196,7 +196,7 @@ void qmp_set_password(const char *protocol, const char 
> *password,
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
> -        rc = qemu_spice_set_passwd(password, fail_if_connected,
> +        rc = qemu_spice.set_passwd(password, fail_if_connected,
>                                     disconnect_if_connected);
>          if (rc != 0) {
>              error_setg(errp, QERR_SET_PASSWD_FAILED);
> @@ -242,7 +242,7 @@ void qmp_expire_password(const char *protocol, const char 
> *whenstr,
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
> -        rc = qemu_spice_set_pw_expire(when);
> +        rc = qemu_spice.set_pw_expire(when);
>          if (rc != 0) {
>              error_setg(errp, QERR_SET_PASSWD_FAILED);
>          }
> @@ -339,7 +339,7 @@ void qmp_add_client(const char *protocol, const char 
> *fdname,
>          }
>          skipauth = has_skipauth ? skipauth : false;
>          tls = has_tls ? tls : false;
> -        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> +        if (qemu_spice.display_add_client(fd, skipauth, tls) < 0) {
>              error_setg(errp, "spice failed to add client");
>              close(fd);
>          }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index bc0dcc4f58..8defda56c9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -173,6 +173,30 @@ int icount_align_option;
>  QemuUUID qemu_uuid;
>  bool qemu_uuid_set;
>  
> +static bool qemu_is_not_using_spice(void)
> +{
> +    return 0;
> +}
> +
> +static void qemu_spice_init_no_op(void)
> +{
> +}
> +
> +/*
> + * Only two fields are initialized, that can be used even when SPICE
> + * is not configured or not loaded. Other functions are protected by
> + * checking if using_spice.
> + */
> +QemuSpiceOps qemu_spice = {
> +    .in_use             = qemu_is_not_using_spice,
> +    .init               = qemu_spice_init_no_op,
> +};
> +
> +void qemu_spice_ops_register(QemuSpiceOps *ops)
> +{
> +    memcpy(&qemu_spice, ops, sizeof(qemu_spice));
> +}
> +
>  static NotifierList exit_notifiers =
>      NOTIFIER_LIST_INITIALIZER(exit_notifiers);
>  
> @@ -4128,7 +4152,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      /* spice needs the timers to be initialized by this point */
>      /* spice must initialize before audio as it changes the default auiodev 
> */
>      /* spice must initialize before chardevs (for spicevmc and spiceport) */
> -    qemu_spice_init();
> +    qemu_spice.init();
>  
>      qemu_opts_foreach(qemu_find_opts("object"),
>                        user_creatable_add_opts_foreach,
> @@ -4424,7 +4448,7 @@ void qemu_init(int argc, char **argv, char **envp)
>  #endif
>  
>      if (using_spice) {
> -        qemu_spice_display_init();
> +        qemu_spice.display_init();
>      }
>  
>      if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 37dd68f2ab..9d7ac70d92 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -48,7 +48,7 @@ static time_t auth_expires = TIME_MAX;
>  static int spice_migration_completed;
>  static int spice_display_is_running;
>  static int spice_have_target_host;
> -int using_spice = 0;
> +static int is_using_spice;
>  
>  static QemuThread me;
>  
> @@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
>      },
>  };
>  
> -SpiceInfo *qmp_query_spice(Error **errp)
> +SpiceInfo *qemu_spice_query(Error **errp)
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>      int port, tls_port;
> @@ -634,6 +634,16 @@ static void vm_change_state_handler(void *opaque, int 
> running,
>      }
>  }
>  
> +bool qemu_is_using_spice(void)
> +{
> +    return is_using_spice;
> +}
> +
> +void qemu_start_using_spice(void)
> +{
> +    is_using_spice = 1;
> +}
> +
>  void qemu_spice_init(void)
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> @@ -796,7 +806,7 @@ void qemu_spice_init(void)
>          error_report("failed to initialize spice server");
>          exit(1);
>      };
> -    using_spice = 1;
> +    qemu_start_using_spice();
>  
>      migration_state.notify = migration_state_notifier;
>      add_migration_state_change_notifier(&migration_state);
> @@ -1000,3 +1010,20 @@ static void spice_register_config(void)
>      qemu_add_opts(&qemu_spice_opts);
>  }
>  opts_init(spice_register_config);
> +
> +static QemuSpiceOps qemu_spice_ops = {
> +    .in_use             = qemu_is_using_spice,
> +    .init               = qemu_spice_init,
> +    .display_init       = qemu_spice_display_init,
> +    .display_add_client = qemu_spice_display_add_client,
> +    .set_passwd         = qemu_spice_set_passwd,
> +    .set_pw_expire      = qemu_spice_set_pw_expire,
> +    .migrate_info       = qemu_spice_migrate_info,
> +    .query              = qemu_spice_query,
> +};
> +
> +static void spice_register_ops(void)
> +{
> +    qemu_spice_ops_register(&qemu_spice_ops);
> +}
> +type_init(spice_register_ops);
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK