[virt-tools-list] [PATCH virt-viewer] Fix close of VNC displays

Hans de Goede hdegoede at redhat.com
Tue Apr 3 19:03:54 UTC 2012


Looks good, ack.

On 04/03/2012 07:35 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> When clicking the close button on a virt-viewer window with
> a VNC session open, while the VNC session terminates, the
> window does not go away.
>
> The problem is that the virt_viewer_session_vnc_disconnected
> method never gets invoked. The close button triggers a call
> to virt_viewer_session_clear_displays which unrefs the
> VirtViewerDisplayVnc instance. This in turn triggers a call
> to gtk_container_destroy, which destroys all widgets it
> contains, ie the VncDisplay * object.
>
> With the VncDisplay object in its dispose phase, no signals
> will ever be emitted, thus the 'vnc-disconnected' signal
> never gets seen.
>
> The design issue is that VirtViewerDisplayVnc is assuming
> it owns the VncDisplay, whereas in fact the real owner is
> the VirtViewerSessionVnc object.
>
> The solution is to introduce a new virt_viewer_display_close
> method which can be used to de-parent the widget before
> VirtViewerDisplay is unref'd.
>
> The VirtViewerSessionVnc object also needs to hold a full ref
> on the VncDisplay object, not merely a floating reference
>
> * virt-viewer-display-spice.c, virt-viewer-display.c,
>    virt-viewer-display.h: Add virt_viewer_display_close
> * virt-viewer-display-vnc.c: Deparent VNC widget in
>    virt_viewer_display_close impl
> * virt-viewer-session-vnc.c: Improve logging
> * virt-viewer-session.c: Call virt_viewer_display_close
>    before unrefing display
> * virt-viewer-window.c: Improve logging
>
> Signed-off-by: Daniel P. Berrange<berrange at redhat.com>
> ---
>   src/virt-viewer-display-spice.c |    8 ++++++++
>   src/virt-viewer-display-vnc.c   |   16 +++++++++++++++-
>   src/virt-viewer-display.c       |   13 +++++++++++++
>   src/virt-viewer-display.h       |    4 ++++
>   src/virt-viewer-session-vnc.c   |    4 ++++
>   src/virt-viewer-session.c       |    6 ++++--
>   src/virt-viewer-window.c        |    1 +
>   7 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/src/virt-viewer-display-spice.c b/src/virt-viewer-display-spice.c
> index 981fb57..985e116 100644
> --- a/src/virt-viewer-display-spice.c
> +++ b/src/virt-viewer-display-spice.c
> @@ -46,6 +46,7 @@ static void virt_viewer_display_spice_send_keys(VirtViewerDisplay *display,
>                                                   int nkeyvals);
>   static GdkPixbuf *virt_viewer_display_spice_get_pixbuf(VirtViewerDisplay *display);
>   static void virt_viewer_display_spice_release_cursor(VirtViewerDisplay *display);
> +static void virt_viewer_display_spice_close(VirtViewerDisplay *display G_GNUC_UNUSED);
>
>   static void
>   virt_viewer_display_spice_finalize(GObject *obj)
> @@ -69,6 +70,7 @@ virt_viewer_display_spice_class_init(VirtViewerDisplaySpiceClass *klass)
>       dclass->send_keys = virt_viewer_display_spice_send_keys;
>       dclass->get_pixbuf = virt_viewer_display_spice_get_pixbuf;
>       dclass->release_cursor = virt_viewer_display_spice_release_cursor;
> +    dclass->close = virt_viewer_display_spice_close;
>
>       g_type_class_add_private(klass, sizeof(VirtViewerDisplaySpicePrivate));
>   }
> @@ -257,6 +259,12 @@ virt_viewer_display_spice_release_cursor(VirtViewerDisplay *display)
>   }
>
>
> +static void
> +virt_viewer_display_spice_close(VirtViewerDisplay *display G_GNUC_UNUSED)
> +{
> +}
> +
> +
>   /*
>    * Local variables:
>    *  c-indent-level: 4
> diff --git a/src/virt-viewer-display-vnc.c b/src/virt-viewer-display-vnc.c
> index 878740f..c0bcf13 100644
> --- a/src/virt-viewer-display-vnc.c
> +++ b/src/virt-viewer-display-vnc.c
> @@ -39,6 +39,7 @@ struct _VirtViewerDisplayVncPrivate {
>
>   static void virt_viewer_display_vnc_send_keys(VirtViewerDisplay* display, const guint *keyvals, int nkeyvals);
>   static GdkPixbuf *virt_viewer_display_vnc_get_pixbuf(VirtViewerDisplay* display);
> +static void virt_viewer_display_vnc_close(VirtViewerDisplay *display);
>
>   static void
>   virt_viewer_display_vnc_finalize(GObject *obj)
> @@ -61,6 +62,7 @@ virt_viewer_display_vnc_class_init(VirtViewerDisplayVncClass *klass)
>
>       dclass->send_keys = virt_viewer_display_vnc_send_keys;
>       dclass->get_pixbuf = virt_viewer_display_vnc_get_pixbuf;
> +    dclass->close = virt_viewer_display_vnc_close;
>
>       g_type_class_add_private(klass, sizeof(VirtViewerDisplayVncPrivate));
>   }
> @@ -153,7 +155,6 @@ virt_viewer_display_vnc_new(VncDisplay *vnc)
>       display = g_object_new(VIRT_VIEWER_TYPE_DISPLAY_VNC, NULL);
>
>       g_object_ref(vnc);
> -    g_object_ref(vnc); /* Because gtk_container_add steals the first ref */
>       display->priv->vnc = vnc;
>
>       gtk_container_add(GTK_CONTAINER(display), GTK_WIDGET(display->priv->vnc));
> @@ -188,6 +189,19 @@ virt_viewer_display_vnc_new(VncDisplay *vnc)
>       return GTK_WIDGET(display);
>   }
>
> +
> +static void
> +virt_viewer_display_vnc_close(VirtViewerDisplay *display)
> +{
> +    VirtViewerDisplayVnc *vnc = VIRT_VIEWER_DISPLAY_VNC(display);
> +
> +    /* We're not the real owner, so we shouldn't be letting the container
> +     * destroy the widget. There are still signals that need to be
> +     * propagated to the VirtViewerSession
> +     */
> +    gtk_container_remove(GTK_CONTAINER(display), GTK_WIDGET(vnc->priv->vnc));
> +}
> +
>   /*
>    * Local variables:
>    *  c-indent-level: 4
> diff --git a/src/virt-viewer-display.c b/src/virt-viewer-display.c
> index 40d23ad..49abf1c 100644
> --- a/src/virt-viewer-display.c
> +++ b/src/virt-viewer-display.c
> @@ -579,6 +579,19 @@ void virt_viewer_display_release_cursor(VirtViewerDisplay *self)
>       klass->release_cursor(self);
>   }
>
> +
> +void virt_viewer_display_close(VirtViewerDisplay *self)
> +{
> +    VirtViewerDisplayClass *klass;
> +
> +    g_return_if_fail(VIRT_VIEWER_IS_DISPLAY(self));
> +
> +    klass = VIRT_VIEWER_DISPLAY_GET_CLASS(self);
> +    g_return_if_fail(klass->close != NULL);
> +
> +    klass->close(self);
> +}
> +
>   /*
>    * Local variables:
>    *  c-indent-level: 4
> diff --git a/src/virt-viewer-display.h b/src/virt-viewer-display.h
> index 07db5fd..ffbaf0e 100644
> --- a/src/virt-viewer-display.h
> +++ b/src/virt-viewer-display.h
> @@ -75,6 +75,8 @@ struct _VirtViewerDisplayClass {
>       GdkPixbuf *(*get_pixbuf)(VirtViewerDisplay *display);
>       void (*release_cursor)(VirtViewerDisplay *display);
>
> +    void (*close)(VirtViewerDisplay *display);
> +
>       /* signals */
>       void (*display_pointer_grab)(VirtViewerDisplay *display);
>       void (*display_pointer_ungrab)(VirtViewerDisplay *display);
> @@ -112,6 +114,8 @@ void virt_viewer_display_set_auto_resize(VirtViewerDisplay *display, gboolean au
>   gboolean virt_viewer_display_get_auto_resize(VirtViewerDisplay *display);
>   void virt_viewer_display_release_cursor(VirtViewerDisplay *display);
>
> +void virt_viewer_display_close(VirtViewerDisplay *display);
> +
>   G_END_DECLS
>
>   #endif /* _VIRT_VIEWER_DISPLAY_H */
> diff --git a/src/virt-viewer-session-vnc.c b/src/virt-viewer-session-vnc.c
> index 075b1ce..3e27566 100644
> --- a/src/virt-viewer-session-vnc.c
> +++ b/src/virt-viewer-session-vnc.c
> @@ -104,6 +104,7 @@ static void
>   virt_viewer_session_vnc_disconnected(VncDisplay *vnc G_GNUC_UNUSED,
>                                        VirtViewerSessionVnc *session)
>   {
> +    DEBUG_LOG("Disconnected");
>       g_signal_emit_by_name(session, "session-disconnected");
>       /* TODO perhaps? */
>       /* virt_viewer_display_set_show_hint(VIRT_VIEWER_DISPLAY(session->priv->vnc), */
> @@ -234,6 +235,7 @@ virt_viewer_session_vnc_close(VirtViewerSession* session)
>
>       g_return_if_fail(self != NULL);
>
> +    DEBUG_LOG("close vnc=%p", self->priv->vnc);
>       if (self->priv->vnc != NULL) {
>           virt_viewer_session_clear_displays(session);
>           vnc_display_close(self->priv->vnc);
> @@ -241,6 +243,7 @@ virt_viewer_session_vnc_close(VirtViewerSession* session)
>       }
>
>       self->priv->vnc = VNC_DISPLAY(vnc_display_new());
> +    g_object_ref_sink(self->priv->vnc);
>
>       g_signal_connect(self->priv->vnc, "vnc-connected",
>                        G_CALLBACK(virt_viewer_session_vnc_connected), session);
> @@ -271,6 +274,7 @@ virt_viewer_session_vnc_new(GtkWindow *main_window)
>       session = g_object_new(VIRT_VIEWER_TYPE_SESSION_VNC, NULL);
>
>       session->priv->vnc = VNC_DISPLAY(vnc_display_new());
> +    g_object_ref_sink(session->priv->vnc);
>       session->priv->main_window = g_object_ref(main_window);
>
>       g_signal_connect(session->priv->vnc, "vnc-connected",
> diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> index 51a8a5a..18b6922 100644
> --- a/src/virt-viewer-session.c
> +++ b/src/virt-viewer-session.c
> @@ -304,8 +304,10 @@ void virt_viewer_session_clear_displays(VirtViewerSession *session)
>       GList *tmp = session->priv->displays;
>
>       while (tmp) {
> -        g_signal_emit_by_name(session, "session-display-removed", tmp->data);
> -        g_object_unref(tmp->data);
> +        VirtViewerDisplay *display = VIRT_VIEWER_DISPLAY(tmp->data);
> +        g_signal_emit_by_name(session, "session-display-removed", display);
> +        virt_viewer_display_close(display);
> +        g_object_unref(display);
>           tmp = tmp->next;
>       }
>       g_list_free(session->priv->displays);
> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> index b17499b..771a8b9 100644
> --- a/src/virt-viewer-window.c
> +++ b/src/virt-viewer-window.c
> @@ -651,6 +651,7 @@ virt_viewer_window_delete(GtkWidget *src G_GNUC_UNUSED,
>                             void *dummy G_GNUC_UNUSED,
>                             VirtViewerWindow *self)
>   {
> +    DEBUG_LOG("Window closed");
>       virt_viewer_app_window_set_visible(self->priv->app, self, FALSE);
>       return TRUE;
>   }




More information about the virt-tools-list mailing list