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

[Christophe de Dinechin]

On 2020-06-26 at 19:35 CEST, Daniel P. Berrangé wrote...
> 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.

The functions that are changed are the module interface between qemu and the
DSO. For example, qemu_spice_init is called from vl.c, i.e. the main binary,
but qemu_spice_audio_init is called from ui/spice-core.c and
ui/spice-input.c, which are both in the DSO after the commit.

The existing function-based interface in qemu-spice.h was not really
carefully designed for modularization, so this list was determined by
following the initialization path. It may not be the smallest possible cut.
It may be neat to add a patch to reorder functions based on whether they are
inside the DSO or exported from it.

As for the macro syntax, I see it as somewhat transient. I wanted to propose
a working and scalable mechanism before adding some nice syntactic sugar
tooling to it. I expect the syntax to turn into something like:

MODIFACE void  qemu_spice_display_init (void);
MODIIMPL void  qemu_spice_display_init (void) { ... }

But it feels a bit too early to do that. I prefer to experiment with a
simple macro for now.

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

My very first approach was indeed to use weak symbols, but then I learned
that ld.so no longer deals with weak symbols, apparently for security
reasons. See LD_DYNAMIC_WEAK in ld.so(8).

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

That was my second attempt (after the weak symbols). I cleaned it up a bit
and put it here: https://github.com/c3d/qemu/commits/spice-vtable.

What made me switch to the approach in this series is the following
considerations:

- A vtable is useful if there can be multiple values for a method, e.g. to
  implement inheritance, or if you have multiple instances. This is not the
  case here.

- A vtable adds one level of indirection, which does not seem to be
  particularly useful or helpful for this particular use case.

- Overloading QOM for that purpose looked more confusing than anything else.
  It looked like I was mixing unrelated concepts. Maybe that's just me.

- The required change with a vtable ends up being more extensive. Instead of
  changing a single line to put an entry point in a DSO, you need to create
  the vtable, add functions to it, add a register function, etc. I was
  looking for an easier and more scalable way.

- In particular, with a vtable, you cannot take advantage of the syntactic
  trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
  So for a vtable, you need to manually write wrappers.

This could be automated, of course, but so far I did not find any clear
benefit, and many drawbacks to using the vtable approach. As a quantitative
comparison point, the commit that does this same connection using the vtable
approach is:

 include/ui/qemu-spice.h | 104 
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 monitor/hmp-cmds.c      |   6 +++++
 softmmu/vl.c            |  10 +++++++
 ui/spice-core.c         |  38 ++++++++++++++++++++++++---
 4 files changed, 148 insertions(+), 10 deletions(-)

as opposed to what is presented in this series:

 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(-)


>
>>  #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


--
Cheers,
Christophe de Dinechin (IRC c3d)