On Fri, 2017-02-03 at 14:42 -0200, Eduardo Lima (Etrunko) wrote: > On 03/02/17 14:34, Christophe Fergeau wrote: > > 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? > > > > Yes, this patch depends on the fix in libgovirt. so there has to be a version check (or a bump of requirements). Pavel > > > > 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? > > > I am not sure, thinking better about it, it may be safer to cancel > on > the response signal, or maybe by the close signal. > > > > > > 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 > > Oops, slipped in. > > > > > > 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. > > Fixed. > > > > > > > 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(RemoteViewerISOLi > > > stDialog *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, > > > (GAsyncReadyCallba > > > ck) 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, > > > (GAsyncReadyC > > > allback)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. > > Also fixed, thanks for the review. V2 coming soon. _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
