diff mbox series

spice: delay starting until display are initialized

Message ID 20210128111319.329755-1-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show
Series spice: delay starting until display are initialized | expand

Commit Message

Marc-André Lureau Jan. 28, 2021, 11:13 a.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

QEMU used to run qemu_spice.display_init() before vm_start(), and
QXL/display interfaces where started then. Now, vm_start() happens
before QXL/display interfaces are added and Spice server doesn't
automatically start them in this case (fixed in spice git)

Fixes Spice regression introduced after 5.2, with refactoring commits
b4e1a34211 ("vl: remove separate preconfig main_loop") and
facf7c60ee ("vl: initialize displays _after_ exiting preconfiguration"),
probably others.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/runstate.h | 1 +
 include/ui/qemu-spice.h   | 1 +
 softmmu/runstate.c        | 5 +++++
 ui/spice-core.c           | 9 ++++++++-
 ui/spice-display.c        | 2 ++
 5 files changed, 17 insertions(+), 1 deletion(-)

Comments

Gerd Hoffmann Jan. 28, 2021, 11:43 a.m. UTC | #1 
Hi,

> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 0178d5766d..3d3e3bcb22 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
>          }
>          qemu_spice_display_init_one(con);
>      }

       if (runstate_is_running()) {
            qemu_spice_display_start();
       }

Isn't that enough?

take care,
  Gerd
Marc-André Lureau Jan. 28, 2021, 11:57 a.m. UTC | #2 
Hi

On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 0178d5766d..3d3e3bcb22 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> >          }
> >          qemu_spice_display_init_one(con);
> >      }
>
>        if (runstate_is_running()) {
>             qemu_spice_display_start();
>        }
>
> Isn't that enough?
>

That should be fine too, I'll update the patch. thanks
Marc-André Lureau Jan. 28, 2021, noon UTC | #3 
Hi

On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > diff --git a/ui/spice-display.c b/ui/spice-display.c
>> > index 0178d5766d..3d3e3bcb22 100644
>> > --- a/ui/spice-display.c
>> > +++ b/ui/spice-display.c
>> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
>> >          }
>> >          qemu_spice_display_init_one(con);
>> >      }
>>
>>        if (runstate_is_running()) {
>>             qemu_spice_display_start();
>>        }
>>
>> Isn't that enough?
>>
>
> That should be fine too, I'll update the patch. thanks
>

Actually no, we still need to prevent the initial
qemu_spice_display_start(), and do a single call when everything is ready
(since spice server doesn't handle adding interfaces dynamically when
running).
Gerd Hoffmann Jan. 28, 2021, 2:26 p.m. UTC | #4 
On Thu, Jan 28, 2021 at 04:00:20PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> > Hi
> >
> > On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >>   Hi,
> >>
> >> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> >> > index 0178d5766d..3d3e3bcb22 100644
> >> > --- a/ui/spice-display.c
> >> > +++ b/ui/spice-display.c
> >> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> >> >          }
> >> >          qemu_spice_display_init_one(con);
> >> >      }
> >>
> >>        if (runstate_is_running()) {
> >>             qemu_spice_display_start();
> >>        }
> >>
> >> Isn't that enough?
> >>
> >
> > That should be fine too, I'll update the patch. thanks
> >
> 
> Actually no, we still need to prevent the initial
> qemu_spice_display_start(), and do a single call when everything is ready
> (since spice server doesn't handle adding interfaces dynamically when
> running).

I still think that moving these three lines to the correct place is
enough.  Maybe even just qemu_spice_display_start() as it keeps track
of the state and you can safely call this twice.

take care,
  Gerd
Marc-André Lureau Jan. 28, 2021, 2:35 p.m. UTC | #5 
Hi

On Thu, Jan 28, 2021 at 6:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Jan 28, 2021 at 04:00:20PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
> > marcandre.lureau@redhat.com> wrote:
> >
> > > Hi
> > >
> > > On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >>   Hi,
> > >>
> > >> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > >> > index 0178d5766d..3d3e3bcb22 100644
> > >> > --- a/ui/spice-display.c
> > >> > +++ b/ui/spice-display.c
> > >> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> > >> >          }
> > >> >          qemu_spice_display_init_one(con);
> > >> >      }
> > >>
> > >>        if (runstate_is_running()) {
> > >>             qemu_spice_display_start();
> > >>        }
> > >>
> > >> Isn't that enough?
> > >>
> > >
> > > That should be fine too, I'll update the patch. thanks
> > >
> >
> > Actually no, we still need to prevent the initial
> > qemu_spice_display_start(), and do a single call when everything is ready
> > (since spice server doesn't handle adding interfaces dynamically when
> > running).
>
> I still think that moving these three lines to the correct place is
> enough.  Maybe even just qemu_spice_display_start() as it keeps track
> of the state and you can safely call this twice.
>

It's not enough, since the first time qemu_spice_display_start() is
called (on vm_start) the display interfaces aren't yet registered. And
spice server doesn't automatically start the newly added interfaces.
Gerd Hoffmann Jan. 28, 2021, 2:42 p.m. UTC | #6 
> > I still think that moving these three lines to the correct place is
> > enough.  Maybe even just qemu_spice_display_start() as it keeps track
> > of the state and you can safely call this twice.
> 
> It's not enough, since the first time qemu_spice_display_start() is
> called (on vm_start) the display interfaces aren't yet registered. And
> spice server doesn't automatically start the newly added interfaces.

So move the vmstate handler registration call too?
I'd prefer to not add more state variables if we can avoid it ...

take care,
  Gerd
Marc-André Lureau Jan. 28, 2021, 3:05 p.m. UTC | #7 
Hi

On Thu, Jan 28, 2021 at 6:42 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > I still think that moving these three lines to the correct place is
> > > enough.  Maybe even just qemu_spice_display_start() as it keeps track
> > > of the state and you can safely call this twice.
> >
> > It's not enough, since the first time qemu_spice_display_start() is
> > called (on vm_start) the display interfaces aren't yet registered. And
> > spice server doesn't automatically start the newly added interfaces.
>
> So move the vmstate handler registration call too?
> I'd prefer to not add more state variables if we can avoid it ...
>

Does that seem right to you?

diff --git a/ui/spice-core.c b/ui/spice-core.c
index b621dd86b6..f592331ce4 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -46,7 +46,6 @@ static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
-static int spice_display_init_done;
 static int spice_display_is_running;
 static int spice_have_target_host;

@@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
*name, const char *value,
 static void vm_change_state_handler(void *opaque, int running,
                                     RunState state)
 {
-    if (running && spice_display_init_done) {
+    if (running) {
         qemu_spice_display_start();
     } else if (state != RUN_STATE_PAUSED) {
         qemu_spice_display_stop();
@@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
int running,

 void qemu_spice_display_init_done(void)
 {
-    spice_display_init_done = true;
+    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
     vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
 }

@@ -810,7 +809,6 @@ static void qemu_spice_init(void)

     qemu_spice_input_init();

-    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
     qemu_spice_display_stop();

     g_free(x509_key_file);
Gerd Hoffmann Jan. 28, 2021, 4:34 p.m. UTC | #8 
Hi,

> > So move the vmstate handler registration call too?
> > I'd prefer to not add more state variables if we can avoid it ...
> 
> Does that seem right to you?

> @@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
> *name, const char *value,
>  static void vm_change_state_handler(void *opaque, int running,
>                                      RunState state)
>  {
> -    if (running && spice_display_init_done) {
> +    if (running) {
>          qemu_spice_display_start();
>      } else if (state != RUN_STATE_PAUSED) {
>          qemu_spice_display_stop();
> @@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
> int running,
> 
>  void qemu_spice_display_init_done(void)
>  {
> -    spice_display_init_done = true;
> +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
>      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());

I'd just call qemu_spice_display_start() directly here, the need for
runstate_get() goes away then.  Otherwise looks good to me.

take care,
  Gerd
Marc-André Lureau Jan. 28, 2021, 7:28 p.m. UTC | #9 
On Thu, Jan 28, 2021 at 8:34 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > So move the vmstate handler registration call too?
> > > I'd prefer to not add more state variables if we can avoid it ...
> >
> > Does that seem right to you?
>
> > @@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
> > *name, const char *value,
> >  static void vm_change_state_handler(void *opaque, int running,
> >                                      RunState state)
> >  {
> > -    if (running && spice_display_init_done) {
> > +    if (running) {
> >          qemu_spice_display_start();
> >      } else if (state != RUN_STATE_PAUSED) {
> >          qemu_spice_display_stop();
> > @@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
> > int running,
> >
> >  void qemu_spice_display_init_done(void)
> >  {
> > -    spice_display_init_done = true;
> > +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
> >      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
>
> I'd just call qemu_spice_display_start() directly here, the need for
> runstate_get() goes away then.  Otherwise looks good to me.

Hmm, that could work, but it will behave differently as we will start
spice even if the VM is not running then.
Gerd Hoffmann Jan. 29, 2021, 2:22 p.m. UTC | #10 
> > >  void qemu_spice_display_init_done(void)
> > >  {
> > > -    spice_display_init_done = true;
> > > +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
> > >      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
> >
> > I'd just call qemu_spice_display_start() directly here, the need for
> > runstate_get() goes away then.  Otherwise looks good to me.
> 
> Hmm, that could work, but it will behave differently as we will start
> spice even if the VM is not running then.

if (runstate_is_running())
    qemu_spice_display_start()

take care,
  Gerd
diff mbox series

Patch

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index e557f470d4..40b3083008 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -6,6 +6,7 @@ 
 
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
+RunState runstate_get(void);
 int runstate_is_running(void);
 bool runstate_needs_reset(void);
 bool runstate_store(char *str, size_t size);
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 2beb792972..71ecd6cfd1 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -28,6 +28,7 @@ 
 
 void qemu_spice_input_init(void);
 void qemu_spice_display_init(void);
+void qemu_spice_display_init_done(void);
 bool qemu_spice_have_display_interface(QemuConsole *con);
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index beee050815..92ef7444d0 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -195,6 +195,11 @@  static void runstate_init(void)
     qemu_mutex_init(&vmstop_lock);
 }
 
+RunState runstate_get(void)
+{
+    return current_run_state;
+}
+
 /* This function will abort() on invalid state transitions */
 void runstate_set(RunState new_state)
 {
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5746d0aae7..b621dd86b6 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -46,6 +46,7 @@  static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
+static int spice_display_init_done;
 static int spice_display_is_running;
 static int spice_have_target_host;
 
@@ -625,13 +626,19 @@  static int add_channel(void *opaque, const char *name, const char *value,
 static void vm_change_state_handler(void *opaque, int running,
                                     RunState state)
 {
-    if (running) {
+    if (running && spice_display_init_done) {
         qemu_spice_display_start();
     } else if (state != RUN_STATE_PAUSED) {
         qemu_spice_display_stop();
     }
 }
 
+void qemu_spice_display_init_done(void)
+{
+    spice_display_init_done = true;
+    vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
+}
+
 static void qemu_spice_init(void)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 0178d5766d..3d3e3bcb22 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1188,4 +1188,6 @@  void qemu_spice_display_init(void)
         }
         qemu_spice_display_init_one(con);
     }
+
+    qemu_spice_display_init_done();
 }