On 2020-07-22 at 11:20 CEST, Christophe de Dinechin wrote...
> On 2020-07-22 at 10:49 CEST, Gerd Hoffmann wrote...
>> Initialize spice before chardevs. That allows to register the spice
>> chardevs directly in the init function and removes the need to maintain
>> a linked list of chardevs just for registration.
>>
>> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
>
> Looks good to me, but I still need to test how this integrates with my work
> on putting SPICE in a module.
That part does not seem to work so well. It looks like with this move, the
init happens before the module is loaded:
qemu-system-x86_64 -display spice-app
Unexpected error in qemu_chr_open_spice_port() at ui/../chardev/spice.c:307:
qemu-system-x86_64: spice not enabled
I'll try to find the correct fix. Any idea how to address this?
>
> Reviewed-by: Christophe de Dinechin <dinec...@redhat.com>
>
>> ---
>> include/chardev/spice.h | 1 -
>> include/ui/qemu-spice.h | 1 -
>> chardev/spice.c | 29 ++++++-----------------------
>> softmmu/vl.c | 9 +++++----
>> ui/spice-core.c | 2 --
>> 5 files changed, 11 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/chardev/spice.h b/include/chardev/spice.h
>> index 1f7339b649dc..2013255f34fd 100644
>> --- a/include/chardev/spice.h
>> +++ b/include/chardev/spice.h
>> @@ -12,7 +12,6 @@ typedef struct SpiceChardev {
>> bool blocked;
>> const uint8_t *datapos;
>> int datalen;
>> - QLIST_ENTRY(SpiceChardev) next;
>> } SpiceChardev;
>>
>> #define TYPE_CHARDEV_SPICE "chardev-spice"
>> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
>> index 8c23dfe71797..d34cea2e0fcd 100644
>> --- a/include/ui/qemu-spice.h
>> +++ b/include/ui/qemu-spice.h
>> @@ -46,7 +46,6 @@ int qemu_spice_migrate_info(const char *hostname, int
>> port, int tls_port,
>> #else
>> #define SPICE_NEEDS_SET_MM_TIME 0
>> #endif
>> -void qemu_spice_register_ports(void);
>>
>> #else /* CONFIG_SPICE */
>>
>> diff --git a/chardev/spice.c b/chardev/spice.c
>> index bf7ea1e2940d..9733f0671699 100644
>> --- a/chardev/spice.c
>> +++ b/chardev/spice.c
>> @@ -14,9 +14,6 @@ typedef struct SpiceCharSource {
>> SpiceChardev *scd;
>> } SpiceCharSource;
>>
>> -static QLIST_HEAD(, SpiceChardev) spice_chars =
>> - QLIST_HEAD_INITIALIZER(spice_chars);
>> -
>> static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int
>> len)
>> {
>> SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
>> @@ -216,8 +213,6 @@ static void char_spice_finalize(Object *obj)
>>
>> vmc_unregister_interface(s);
>>
>> - QLIST_SAFE_REMOVE(s, next);
>> -
>> g_free((char *)s->sin.subtype);
>> g_free((char *)s->sin.portname);
>> }
>> @@ -256,8 +251,6 @@ static void chr_open(Chardev *chr, const char *subtype)
>>
>> s->active = false;
>> s->sin.subtype = g_strdup(subtype);
>> -
>> - QLIST_INSERT_HEAD(&spice_chars, s, next);
>> }
>>
>> static void qemu_chr_open_spice_vmc(Chardev *chr,
>> @@ -310,28 +303,18 @@ void qemu_chr_open_spice_port(Chardev *chr,
>> return;
>> }
>>
>> + if (!using_spice) {
>> + error_setg(errp, "spice not enabled");
>> + return;
>> + }
>> +
>> chr_open(chr, "port");
>>
>> *be_opened = false;
>> s = SPICE_CHARDEV(chr);
>> s->sin.portname = g_strdup(name);
>>
>> - if (using_spice) {
>> - /* spice server already created */
>> - vmc_register_interface(s);
>> - }
>> -}
>> -
>> -void qemu_spice_register_ports(void)
>> -{
>> - SpiceChardev *s;
>> -
>> - QLIST_FOREACH(s, &spice_chars, next) {
>> - if (s->sin.portname == NULL) {
>> - continue;
>> - }
>> - vmc_register_interface(s);
>> - }
>> + vmc_register_interface(s);
>> }
>>
>> static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend
>> *backend,
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f476ef89edb7..bc0dcc4f58bd 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -4125,6 +4125,11 @@ void qemu_init(int argc, char **argv, char **envp)
>> page_size_init();
>> socket_init();
>>
>> + /* spice needs the timers to be initialized by this point */
>> + /* spice must initialize before audio as it changes the default auiodev
>> */
>> + /* spice must initialize before chardevs (for spicevmc and spiceport) */
>> + qemu_spice_init();
>> +
>> qemu_opts_foreach(qemu_find_opts("object"),
>> user_creatable_add_opts_foreach,
>> object_create_initial, &error_fatal);
>> @@ -4139,10 +4144,6 @@ void qemu_init(int argc, char **argv, char **envp)
>> fsdev_init_func, NULL, &error_fatal);
>> #endif
>>
>> - /* spice needs the timers to be initialized by this point */
>> - /* spice must initialize before audio as it changes the default auiodev
>> */
>> - qemu_spice_init();
>> -
>> /*
>> * Note: we need to create audio and block backends before
>> * machine_set_property(), so machine properties can refer to
>> diff --git a/ui/spice-core.c b/ui/spice-core.c
>> index ecc2ec2c55c2..37dd68f2aba2 100644
>> --- a/ui/spice-core.c
>> +++ b/ui/spice-core.c
>> @@ -813,8 +813,6 @@ void qemu_spice_init(void)
>> g_free(x509_cert_file);
>> g_free(x509_cacert_file);
>>
>> - qemu_spice_register_ports();
>> -
>> #ifdef HAVE_SPICE_GL
>> if (qemu_opt_get_bool(opts, "gl", 0)) {
>> if ((port != 0) || (tls_port != 0)) {
--
Cheers,
Christophe de Dinechin (IRC c3d)
