[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