| Message ID | 20200626164307.3327380-10-dinechin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Move SPICE to a load module | expand |
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
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)
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
Hi, > > 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. I think this is the direction we should take. > 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. Well, we'll have two. The normal functions. And the stubs. The stubs are inline functions right now, in include/ui/qemu-spice.h, in the !CONFIG_SPICE section. We can turn them into normal functions, move them to some C file. Let QemuSpiceOpts function pointers point to the stubs initially. When spice initializes (no matter whenever modular or not) it'll set QemuSpiceOpts to the normal implementation. That way we'll also remove some spice #ifdefs as part of the spice modularization effort. Things like the "using_spice" variable which don't depend on the spice shared libraries can also be moved to the new C file with the spice stubs. I don't think we need to hide QemuSpiceOpts with inline functions like qemu_spice_migrate_info(). I would simply use ... struct QemuSpiceOps { [ ... ] int (*migrate_info)(...); [ ... ] } qemu_spice; ... then change the ... qemu_spice_migrate_info(...) .. callsites into ... qemu_spice.migrate_info(...) > - Overloading QOM for that purpose looked more confusing than anything else. > It looked like I was mixing unrelated concepts. Maybe that's just me. Hmm? Not sure what you mean. There is no need for QOM here (and I can't see anything like that in your spice-vtable branch either). > - 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. IMHO it isn't too much overhead, and I find the code is more readable that 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. See above, I don't think we need wrappers. take care, Gerd
On 2020-06-30 at 01:00 CEST, Gerd Hoffmann wrote... > Hi, > >> > 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. > > I think this is the direction we should take. > >> 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. > > Well, we'll have two. The normal functions. And the stubs. > > The stubs are inline functions right now, in include/ui/qemu-spice.h, in > the !CONFIG_SPICE section. We can turn them into normal functions, move > them to some C file. Let QemuSpiceOpts function pointers point to the > stubs initially. When spice initializes (no matter whenever modular or > not) it'll set QemuSpiceOpts to the normal implementation. Good idea. I'll do that in the next round. > > That way we'll also remove some spice #ifdefs as part of the spice > modularization effort. > > Things like the "using_spice" variable which don't depend on the spice > shared libraries can also be moved to the new C file with the spice > stubs. OK. > > I don't think we need to hide QemuSpiceOpts with inline functions like > qemu_spice_migrate_info(). I would simply use ... > > struct QemuSpiceOps { > [ ... ] > int (*migrate_info)(...); > [ ... ] > } qemu_spice; > > ... then change the ... > > qemu_spice_migrate_info(...) > > .. callsites into ... > > qemu_spice.migrate_info(...) > OK. >> - Overloading QOM for that purpose looked more confusing than anything else. >> It looked like I was mixing unrelated concepts. Maybe that's just me. > > Hmm? Not sure what you mean. There is no need for QOM here (and I > can't see anything like that in your spice-vtable branch either). I was responding to Daniels's earlier comment: > 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. > >> - 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. > > IMHO it isn't too much overhead, and I find the code is more readable > that way. There is certainly very little overhead in that case, since none of the functions is performance critical. > >> - 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. > > See above, I don't think we need wrappers. Well, so far that's two for two for the vtable approach. So unless someone else agrees with my arguments for pointer patching, that will be my next iteration of that series :-) > > take care, > Gerd -- Cheers, Christophe de Dinechin (IRC c3d)
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)); #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);
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(-)