Hey, On Thu, Jan 26, 2017 at 06:01:23PM -0200, Eduardo Lima (Etrunko) wrote: > We must take into account that users can close the dialog at anytime, > even during an operation of fetch or set ISO has not been finished. This > will cause the callbacks for those operations to be invoked with an > invalid object, crashing the application. > > To fix this issue we need to pass a GCancellable to the asynchronous > operations, so they can be cancelled if the dialog happens to get closed > before they complete. > This is going to depend on libgovirt bug https://bugzilla.gnome.org/show_bug.cgi?id=777808 being fixed, otherwise we'd be getting a deadlock, is that correct? > Signed-off-by: Eduardo Lima (Etrunko) <etrunko@xxxxxxxxxx> > --- > src/remote-viewer-iso-list-dialog.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c > index f23ddb2..a7ac98a 100644 > --- a/src/remote-viewer-iso-list-dialog.c > +++ b/src/remote-viewer-iso-list-dialog.c > @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate > GtkWidget *stack; > GtkWidget *tree_view; > OvirtForeignMenu *foreign_menu; > + GCancellable *cancellable; > }; > > enum RemoteViewerISOListDialogModel > @@ -66,10 +67,16 @@ remote_viewer_iso_list_dialog_dispose(GObject *object) > RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > + if (priv->cancellable) { > + g_cancellable_cancel(priv->cancellable); > + g_clear_object(&priv->cancellable); > + } > + g_cancellable_cancel() can be async, and at this point, the iso dialog is already dead, are you sure it's safe to cancel here? > if (priv->foreign_menu) { > g_signal_handlers_disconnect_by_data(priv->foreign_menu, object); > g_clear_object(&priv->foreign_menu); > } > + I'd drop this added blank line > G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object); > } > > @@ -157,6 +164,10 @@ fetch_iso_names_cb(OvirtForeignMenu *foreign_menu, > const gchar *msg = error ? error->message : _("Failed to fetch CD names"); > gchar *markup = g_strdup_printf("<b>%s</b>", msg); > > + g_debug("Error fetching ISO names: %s", msg); > + if (error && error->code == G_IO_ERROR_CANCELLED) > + return; > + 'error' is leaked. > gtk_label_set_markup(GTK_LABEL(priv->status), markup); > gtk_spinner_stop(GTK_SPINNER(priv->spinner)); > remote_viewer_iso_list_dialog_show_error(self, msg); > @@ -166,6 +177,7 @@ fetch_iso_names_cb(OvirtForeignMenu *foreign_menu, > return; > } > > + g_clear_object(&priv->cancellable); > g_list_foreach(iso_list, (GFunc) remote_viewer_iso_list_dialog_foreach, self); > remote_viewer_iso_list_dialog_show_files(self); > } > @@ -177,7 +189,10 @@ remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDialog *self) > RemoteViewerISOListDialogPrivate *priv = self->priv; > > gtk_list_store_clear(priv->list_store); > - ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, NULL, > + > + priv->cancellable = g_cancellable_new(); > + ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, > + priv->cancellable, > (GAsyncReadyCallback) fetch_iso_names_cb, > self); > } > @@ -223,7 +238,9 @@ remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer G_GNU > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE); > gtk_widget_set_sensitive(priv->tree_view, FALSE); > > - ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active ? NULL : name, NULL, > + priv->cancellable = g_cancellable_new(); > + ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active ? NULL : name, > + priv->cancellable, > (GAsyncReadyCallback)ovirt_foreign_menu_iso_name_changed, > self); > gtk_tree_path_free(tree_path); > @@ -308,10 +325,17 @@ ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, > * change the ISO back to the original, avoiding a possible inconsistency. > */ > if (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu, result, &error)) { > - remote_viewer_iso_list_dialog_show_error(self, error ? error->message : _("Failed to change CD")); > + gchar *msg = error ? error->message : _("Failed to change CD"); > + g_debug("Error changing ISO: %s", msg); > + > + if (error && error->code == G_IO_ERROR_CANCELLED) > + return; 'error' is leaked. > + > + remote_viewer_iso_list_dialog_show_error(self, msg); > g_clear_error(&error); > } > > + g_clear_object(&priv->cancellable); > if (!gtk_tree_model_get_iter_first(model, &iter)) > return; > > -- > 2.9.3 > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/virt-tools-list
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
