Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Daniel P . Berrangé]

On Fri, Jun 26, 2020 at 06:43:06PM +0200, Christophe de Dinechin wrote:
> Use the MODIFACE and MODIMPL macros to to redirect the highest-level
> qemu_spice functions into the spice-app.so load module when SPICE is
> compiled as a module.
> 
> With these changes, the following shared libraries are no longer
> necessary in the top-level qemu binary:
> 
>       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 | 24 +++++++++++++++---------
>  monitor/hmp-cmds.c      |  6 ++++++
>  softmmu/vl.c            |  1 +
>  ui/spice-core.c         | 31 +++++++++++++++++++++----------
>  ui/spice-display.c      |  2 +-
>  5 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index 8c23dfe717..0f7e139da5 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -24,22 +24,28 @@
>  
>  #include <spice.h>
>  #include "qemu/config-file.h"
> +#include "qemu/module.h"
>  
> -extern int using_spice;
> +#define using_spice     (qemu_is_using_spice())
>  
> -void qemu_spice_init(void);
> +MODIFACE(bool, qemu_is_using_spice,(void));
> +MODIFACE(void, qemu_start_using_spice, (void));
> +MODIFACE(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);
> +MODIFACE(void, qemu_spice_display_init, (void));
> +MODIFACE(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);
> -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);
> +MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
> +                                      bool fail_if_connected,
> +                                      bool disconnect_if_connected));
> +MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
> +MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
> +                                       int port, int tls_port,
> +                                       const char *subject));
> +MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));

This macro usage looks kind of unpleasant and its hard to understand
just what is going on, especially why some methods are changed but
others are not.

IIUC, the goal is to turn all these into weak symbols so they don't
need to be resolved immediately at startup, and instead have an
impl of them provided dynamically at runtime.

If so the more normal approach would be to have a struct defining
a set of callbacks, that can be registered. Or if there's a natural
fit with QOM, then a QOM interface that can then have a QOM object
impl registered as a singleton.

>  #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
>  #define SPICE_NEEDS_SET_MM_TIME 1
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 2b0b58a336..6bd9c52658 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -56,6 +56,7 @@
>  #include "migration/misc.h"
>  
>  #ifdef CONFIG_SPICE
> +#include "ui/qemu-spice.h"
>  #include <spice/enums.h>
>  #endif
>  
> @@ -573,6 +574,11 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>  #endif
>  
>  #ifdef CONFIG_SPICE
> +SpiceInfo *qmp_query_spice(Error **errp)
> +{
> +    return qemu_spice_query(errp);
> +}
> +
>  void hmp_info_spice(Monitor *mon, const QDict *qdict)
>  {
>      SpiceChannelList *chan;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 3e15ee2435..c94b4fa49b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  
> +#define MODULE_STUBS
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/units.h"
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ecc2ec2c55..dbc1886b77 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 = 0;
>  
>  static QemuThread me;
>  
> @@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
>      },
>  };
>  
> -SpiceInfo *qmp_query_spice(Error **errp)
> +MODIMPL(SpiceInfo *,qemu_spice_query,(Error **errp))
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>      int port, tls_port;
> @@ -579,8 +579,9 @@ static void migration_state_notifier(Notifier *notifier, 
> void *data)
>      }
>  }
>  
> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> -                            const char *subject)
> +MODIMPL(int, qemu_spice_migrate_info, (const char *hostname,
> +                                       int port, int tls_port,
> +                                       const char *subject))
>  {
>      int ret;
>  
> @@ -634,7 +635,17 @@ static void vm_change_state_handler(void *opaque, int 
> running,
>      }
>  }
>  
> -void qemu_spice_init(void)
> +MODIMPL(bool, qemu_is_using_spice, (void))
> +{
> +    return is_using_spice;
> +}
> +
> +MODIMPL(void, qemu_start_using_spice, (void))
> +{
> +    is_using_spice = 1;
> +}
> +
> +MODIMPL(void, qemu_spice_init, (void))
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>      const char *password, *str, *x509_dir, *addr,
> @@ -796,7 +807,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);
> @@ -945,8 +956,8 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool 
> disconnect_if_conn)
>                                     fail_if_conn, disconnect_if_conn);
>  }
>  
> -int qemu_spice_set_passwd(const char *passwd,
> -                          bool fail_if_conn, bool disconnect_if_conn)
> +MODIMPL(int, qemu_spice_set_passwd,(const char *passwd,
> +                                    bool fail_if_conn, bool 
> disconnect_if_conn))
>  {
>      if (strcmp(auth, "spice") != 0) {
>          return -1;
> @@ -957,13 +968,13 @@ int qemu_spice_set_passwd(const char *passwd,
>      return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
>  }
>  
> -int qemu_spice_set_pw_expire(time_t expires)
> +MODIMPL(int, qemu_spice_set_pw_expire, (time_t expires))
>  {
>      auth_expires = expires;
>      return qemu_spice_set_ticket(false, false);
>  }
>  
> -int qemu_spice_display_add_client(int csock, int skipauth, int tls)
> +MODIMPL(int, qemu_spice_display_add_client, (int csock, int skipauth, int 
> tls))
>  {
>      if (tls) {
>          return spice_server_add_ssl_client(spice_server, csock, skipauth);
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 19632fdf6c..90529695fe 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1164,7 +1164,7 @@ static void qemu_spice_display_init_one(QemuConsole 
> *con)
>      register_displaychangelistener(&ssd->dcl);
>  }
>  
> -void qemu_spice_display_init(void)
> +MODIMPL(void, qemu_spice_display_init,(void))
>  {
>      QemuOptsList *olist = qemu_find_opts("spice");
>      QemuOpts *opts = QTAILQ_FIRST(&olist->head);
> -- 
> 2.26.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|