Re: [PATCH] spice: simplify chardev setup

From: Christophe de Dinechin <dinechin@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	qemu-devel@nongnu.org,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH] spice: simplify chardev setup
Date: Thu, 23 Jul 2020 10:26:59 +0200	[thread overview]
Message-ID: <7hh7ty4ox8.fsf@turbo.dinechin.lan> (raw)
In-Reply-To: <20200722111822.skfkwlmr5ikhkaqh@sirius.home.kraxel.org>

On 2020-07-22 at 13:18 CEST, Gerd Hoffmann wrote...
> On Wed, Jul 22, 2020 at 12:19:43PM +0200, Christophe de Dinechin wrote:
>>
>> 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 <kraxel@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?
>
> move chardev init from early to normal:

Works for me.

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
Tested-by: Christophe de Dinechin <dinechin@redhat.com>

>
> commit 77297a71e6be60997caf2c55c09ce01a8c492bc2
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Wed Jul 22 13:17:28 2020 +0200
>
>     [fixup] spice app
>
> diff --git a/ui/spice-app.c b/ui/spice-app.c
> index 40fb2ef57399..03d971b15f0c 100644
> --- a/ui/spice-app.c
> +++ b/ui/spice-app.c
> @@ -117,7 +117,6 @@ static void spice_app_atexit(void)
>  static void spice_app_display_early_init(DisplayOptions *opts)
>  {
>      QemuOpts *qopts;
> -    ChardevBackend *be = chr_spice_backend_new();
>      GError *err = NULL;
>
>      if (opts->has_full_screen) {
> @@ -162,6 +161,15 @@ static void spice_app_display_early_init(DisplayOptions *opts)
>      qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", &error_abort);
>      display_opengl = opts->has_gl;
>  #endif
> +}
> +
> +static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> +{
> +    ChardevBackend *be = chr_spice_backend_new();
> +    QemuOpts *qopts;
> +    GError *err = NULL;
> +    gchar *uri;
> +
>      be->u.spiceport.data->fqdn = g_strdup("org.qemu.monitor.qmp.0");
>      qemu_chardev_new("org.qemu.monitor.qmp", TYPE_CHARDEV_SPICEPORT,
>                       be, NULL, &error_abort);
> @@ -171,13 +179,6 @@ static void spice_app_display_early_init(DisplayOptions *opts)
>      qemu_opt_set(qopts, "mode", "control", &error_abort);
>
>      qapi_free_ChardevBackend(be);
> -}
> -
> -static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> -{
> -    GError *err = NULL;
> -    gchar *uri;
> -
>      uri = g_strjoin("", "spice+unix://", app_dir, "/", "spice.sock", NULL);
>      info_report("Launching display with URI: %s", uri);
>      g_app_info_launch_default_for_uri(uri, NULL, &err);

--
Cheers,
Christophe de Dinechin (IRC c3d)