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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jun 29, 2020 at 07:19:41PM +0200, Christophe de Dinechin wrote:
> 
> 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.

IMHO a vtable is fine for singleton objects. It is a generic way to
remove tight coupling between caller(s) and an implementation. Just
having 1 implementation doesn't invalidate the model.

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

I much prefer the code in the vtable patch version. The larger number of
lines of code doesn't bother me, because the code is really simple and
clear to read. IOW I prefer the clarity of the longer code, over the
shorter code with magic hidden behind it.

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