Bug 1713548

> 3.After migration completed, remote-viewer lost connection with the guest since not set target hostname in the third host;

Interesting.

> 1.Can not be reproduced with virt-viewer-5.0-13.el7.x86_64.

Difference between -13 and -14 build is the fix for bug 1505809
Let's see if Jonathon has time for it, otherwise I'll take a look later.

I may have stumbled on a related issue while playing with Qemu/virsh: in some cases where Qemu quitted unexpectedly, the virt-viewer would lost connection to the VM, but the virt-viewer window refused to quit.

> static void virt_viewer_app_quit(VirtViewerApp *self)
> {
>     ...
>
>     if (priv->session) {
>         virt_viewer_session_close(VIRT_VIEWER_SESSION(priv->session));
>         if (priv->connected) {
>             priv->quitting = TRUE;
>             return;
>         }
>     }
>
>     g_application_quit(G_APPLICATION(self));
> }

always sets the quitting flag, but nothing else happens.

This means that the disconnected/cancelled functions are never called *after* the quit request:

> static void
> virt_viewer_app_disconnected(VirtViewerSession *session G_GNUC_UNUSED, const gchar *msg, VirtViewerApp *self)
> {
>     ...
>     if (priv->quitting)
>         g_application_quit(G_APPLICATION(self));
>     ...
> }
>
> static void virt_viewer_app_cancelled(VirtViewerSession *session,
>                                       VirtViewerApp *self)
> {
>     VirtViewerAppPrivate *priv = self->priv;
>     priv->cancelled = TRUE;
>     virt_viewer_app_disconnected(session, NULL, self);
> }


so one way to solve this problem is to force-quit if the `quitting` flag has already been set (= click on 'quit' twice):

diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
index da8cfa9..47b1a46 100644
--- a/src/virt-viewer-app.c
+++ b/src/virt-viewer-app.c
@@ -298,7 +298,7 @@ virt_viewer_app_quit(VirtViewerApp *self)
 
     if (priv->session) {
         virt_viewer_session_close(VIRT_VIEWER_SESSION(priv->session));
-        if (priv->connected) {
+        if (priv->connected && !priv->quitting) {
             priv->quitting = TRUE;
             return;
         }

another way could be to check if the `cancelled` flag has been set earlier (hopefully yes but I didn't check yet), and quit if yes:

diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
index da8cfa9..47b1a46 100644
--- a/src/virt-viewer-app.c
+++ b/src/virt-viewer-app.c
@@ -298,7 +298,7 @@ virt_viewer_app_quit(VirtViewerApp *self)
 
     if (priv->session) {
         virt_viewer_session_close(VIRT_VIEWER_SESSION(priv->session));
-        if (priv->connected) {
+        if (priv->connected && !priv->cancelled) {
             priv->quitting = TRUE;
             return;
         }

I managed to reproduce once the buggy behavior (cannot close the remote-viewer window) with the HEAD I used at the time of comment #3 [0] by manually triggering a segfault in Qemu, but I cannot reproduce it again ...

My debugging investigations lead to the conclusion that if spice-gtk doesn't send the DISCONNECTED signal [1] for any reason, then the viewer cannot be closed.

The patch [2] allows a 'force-close' if the user requests twice to close the app:                                                                                       
                                                                                               
1. The first time, `virt_viewer_session_close(VIRT_VIEWER_SESSION(priv->session))` is called,
which should trigger the DISCONNECTED signal and close the app. If this fails,
2. The second time, the application is closed, no matter the internal state.


0: https://pagure.io/virt-viewer/c/898f972
1: https://gitlab.freedesktop.org/spice/spice-gtk/blob/0c52ce8937c849d8ae32ade1f22ce3a48c56c732/src/spice-session.c#L2322
2: https://www.redhat.com/archives/virt-tools-list/2019-October/msg00022.html

Reproduce with packages:
libvirt-4.5.0-23.el7_7.3.x86_64
virt-viewer-5.0-15.el7.x86_64
qemu-kvm-rhev-2.12.0-33.el7_7.4.x86_64
spice-gtk3-0.35-4.el7.x86_64

Steps:
1. Prepare migration env on HOST1 and HOST2

2. Start a vm with spice graphic on src host(HOST1)
...
    <graphics type='spice' port='5900' autoport='yes' listen='0.0.0.0'>
      <listen type='address' address='0.0.0.0'/>
      <image compression='off'/>
    </graphics>

3.Connect vm on the third machine($HOST3) with remote-viewer:
$ remote-viewer spice://$HOST3:5900

4.Do migration between HOST1 and HOST2
#virsh migrate vm1 qemu+ssh://$HOST2/system --live --verbose

5.After migration completed, remote-viewer lost connection with the guest since not set target hostname on HOST3

$ cat /etc/hosts
127.0.0.1   localhost localhost.localdomain localhost4 localhost4.localdomain4
::1         localhost localhost.localdomain localhost6 localhost6.localdomain6

6.Close remote-viewer opened in step 3.

Result: The remote-viewer windows can not be closed.
And the process is still existing.
$ pgrep remote-viewer
27293


And Kevin,
How about your plan to make the new build of virt-viewer, I hope it asap then we can have enough time to verify bug and do related functional testing.

BR,
juzhou.

Hi Kevin Pouget,
Comment 5 is tested based on rhel7.7 released version.

Then I tested with latest rhel7.8 packages, i can not reproduce bug issue.

Packages:

libvirt-4.5.0-28.el7.x86_64
virt-viewer-5.0-17.el7.x86_64
qemu-kvm-rhev-2.12.0-38.el7.x86_64
spice-gtk3-0.35-5.el7.x86_64

Steps are same with Comment 5:
After step 5, remote-viewer windows closed successfully after migration finished and the process exited.

I will attach the debug log: 5.0-17.log

Created attachment 1631349 [details]
Latest virt-viewer version testing log

the patch I proposed (https://bugzilla.redhat.com/show_bug.cgi?id=1713548#c4) was rejected upstream, so I revert the bug status to NEW.

(In reply to Kevin Pouget from comment #8)
> the patch I proposed
> (https://bugzilla.redhat.com/show_bug.cgi?id=1713548#c4) was rejected
> upstream, so I revert the bug status to NEW.

Hi Kevin,
Thanks for your quick feedback, and as you said in email, we'll close this bug as "CURRENTRELEASE" if Victor agree.

BR,
juzhou.

As per comment #6 and further discussions, closing this one.