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 <dinec...@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 :|
