ACK from me, but a couple comments below that you can ignore if you want. On Wed, 2015-03-18 at 17:49 +0100, Pavel Grunt wrote: > This applies for: > libvirt authentication dialog (e.g. virt-viewer --attach guest) > 'recent connection' dialog (e.g. remote-viewer) > 'vm choose' dialog when connecting without specifying the vm name > > This is done by using a new GError VIRT_VIEWER_ERROR_CANCELLED. > --- > v3: approach using GError instead of virt_viewer_app_set_user_cancelled() > https://www.redhat.com/archives/virt-tools-list/2015-March/msg00109.html > --- > src/remote-viewer-main.c | 6 +++++- > src/remote-viewer.c | 23 ++++++++++++-------- > src/virt-viewer-app.c | 6 +++--- > src/virt-viewer-app.h | 4 ++-- > src/virt-viewer-main.c | 6 +++++- > src/virt-viewer-util.h | 1 + > src/virt-viewer-vm-connection.c | 4 ++++ > src/virt-viewer.c | 48 ++++++++++++++++++++++++----------------- > 8 files changed, 62 insertions(+), 36 deletions(-) > > diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c > index e8784ba..b3bb053 100644 > --- a/src/remote-viewer-main.c > +++ b/src/remote-viewer-main.c > @@ -174,8 +174,12 @@ main(int argc, char **argv) > > app = VIRT_VIEWER_APP(viewer); > > - if (!virt_viewer_app_start(app)) > + if (!virt_viewer_app_start(app, &error)) { > + if (g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED)) > + ret = 0; > + g_clear_error(&error); This seems like a decent place to log an error message. Are we relying on the virt_viewer_app_start() functions to log the error instead? Unrelated to your changes, but it might be nice to move the g_clear_error() call to the cleanup section instead of calling it before both occurences of 'goto cleanup'. (same comments for virt-viewer-main.c below) > goto cleanup; > + } > > g_signal_connect(virt_viewer_app_get_session(app), "session-connected", > G_CALLBACK(connected), app); > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > index 4541515..5b67f51 100644 > --- a/src/remote-viewer.c > +++ b/src/remote-viewer.c > @@ -82,7 +82,7 @@ static OvirtVm * choose_vm(GtkWindow *main_window, > GError **error); > #endif > > -static gboolean remote_viewer_start(VirtViewerApp *self); > +static gboolean remote_viewer_start(VirtViewerApp *self, GError **error); > #ifdef HAVE_SPICE_GTK > static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error); > static void remote_viewer_window_added(VirtViewerApp *self, VirtViewerWindow *win); > @@ -177,7 +177,7 @@ remote_viewer_deactivated(VirtViewerApp *app, gboolean connect_error) > RemoteViewerPrivate *priv = self->priv; > > if (connect_error && priv->open_recent_dialog) { > - if (virt_viewer_app_start(app)) { > + if (virt_viewer_app_start(app, NULL)) { > return; > } > } > @@ -1205,7 +1205,7 @@ choose_vm(GtkWindow *main_window, > #endif > > static gboolean > -remote_viewer_start(VirtViewerApp *app) > +remote_viewer_start(VirtViewerApp *app, GError **err) > { > g_return_val_if_fail(REMOTE_VIEWER_IS(app), FALSE); > > @@ -1244,8 +1244,13 @@ remote_viewer_start(VirtViewerApp *app) > retry_dialog: > main_window = virt_viewer_app_get_main_window(app); > if (priv->open_recent_dialog) { > - if (connect_dialog(virt_viewer_window_get_window(main_window), &guri) != 0) > + if (connect_dialog(virt_viewer_window_get_window(main_window), &guri) != 0) { > + g_set_error_literal(&error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED, > + _("No connection was chosen")); > + g_propagate_error(err, error); > return FALSE; > + } > g_object_set(app, "guri", guri, NULL); > } else > g_object_get(app, "guri", &guri, NULL); > @@ -1262,7 +1267,6 @@ retry_dialog: > if (error) { > virt_viewer_app_simple_message_dialog(app, _("Invalid file %s"), guri); > g_warning("%s", error->message); > - g_clear_error(&error); > goto cleanup; > } > g_object_get(G_OBJECT(vvfile), "type", &type, NULL); > @@ -1273,12 +1277,11 @@ retry_dialog: > #ifdef HAVE_OVIRT > if (g_strcmp0(type, "ovirt") == 0) { > if (!create_ovirt_session(app, guri, &error)) { > - if (error) { > + if (error && !g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED)) { > virt_viewer_app_simple_message_dialog(app, > _("Couldn't open oVirt session: %s"), > error->message); > } > - g_clear_error(&error); > goto cleanup; > } > } else > @@ -1306,14 +1309,13 @@ retry_dialog: > _("Failed to initiate connection"); > > virt_viewer_app_simple_message_dialog(app, msg); > - g_clear_error(&error); > goto cleanup; > } > #ifdef HAVE_SPICE_GTK > } > #endif > > - ret = VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)->start(app); > + ret = VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)->start(app, &error); > > cleanup: > g_clear_object(&file); > @@ -1324,8 +1326,11 @@ cleanup: > type = NULL; > > if (!ret && priv->open_recent_dialog) { > + g_clear_error(&error); > goto retry_dialog; > } > + if (error != NULL) > + g_propagate_error(err, error); > > return ret; > } > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c > index 8bf728f..b0ee864 100644 > --- a/src/virt-viewer-app.c > +++ b/src/virt-viewer-app.c > @@ -1677,13 +1677,13 @@ virt_viewer_app_dispose (GObject *object) > } > > static gboolean > -virt_viewer_app_default_start(VirtViewerApp *self) > +virt_viewer_app_default_start(VirtViewerApp *self, GError **error G_GNUC_UNUSED) > { > virt_viewer_window_show(self->priv->main_window); > return TRUE; > } > > -gboolean virt_viewer_app_start(VirtViewerApp *self) > +gboolean virt_viewer_app_start(VirtViewerApp *self, GError **error) > { > VirtViewerAppClass *klass; > > @@ -1692,7 +1692,7 @@ gboolean virt_viewer_app_start(VirtViewerApp *self) > > g_return_val_if_fail(!self->priv->started, TRUE); > > - self->priv->started = klass->start(self); > + self->priv->started = klass->start(self, error); > return self->priv->started; > } > > diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h > index d214279..ca53603 100644 > --- a/src/virt-viewer-app.h > +++ b/src/virt-viewer-app.h > @@ -51,7 +51,7 @@ typedef struct { > void (*window_removed) (VirtViewerApp *self, VirtViewerWindow *window); > > /*< private >*/ > - gboolean (*start) (VirtViewerApp *self); > + gboolean (*start) (VirtViewerApp *self, GError **error); > gboolean (*initial_connect) (VirtViewerApp *self, GError **error); > gboolean (*activate) (VirtViewerApp *self, GError **error); > void (*deactivated) (VirtViewerApp *self, gboolean connect_error); > @@ -61,7 +61,7 @@ typedef struct { > GType virt_viewer_app_get_type (void); > > void virt_viewer_app_set_debug(gboolean debug); > -gboolean virt_viewer_app_start(VirtViewerApp *app); > +gboolean virt_viewer_app_start(VirtViewerApp *app, GError **error); > void virt_viewer_app_maybe_quit(VirtViewerApp *self, VirtViewerWindow *window); > VirtViewerWindow* virt_viewer_app_get_main_window(VirtViewerApp *self); > void virt_viewer_app_trace(VirtViewerApp *self, const char *fmt, ...); > diff --git a/src/virt-viewer-main.c b/src/virt-viewer-main.c > index 3fae955..ae880ab 100644 > --- a/src/virt-viewer-main.c > +++ b/src/virt-viewer-main.c > @@ -113,8 +113,12 @@ int main(int argc, char **argv) > if (viewer == NULL) > goto cleanup; > > - if (!virt_viewer_app_start(VIRT_VIEWER_APP(viewer))) > + if (!virt_viewer_app_start(VIRT_VIEWER_APP(viewer), &error)) { > + if (g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED)) > + ret = 0; > + g_clear_error(&error); > goto cleanup; > + } > > gtk_main(); > > diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h > index 1df81a5..2226bf5 100644 > --- a/src/virt-viewer-util.h > +++ b/src/virt-viewer-util.h > @@ -30,6 +30,7 @@ extern gboolean doDebug; > > enum { > VIRT_VIEWER_ERROR_FAILED, > + VIRT_VIEWER_ERROR_CANCELLED, > }; > > #define VIRT_VIEWER_ERROR virt_viewer_error_quark () > diff --git a/src/virt-viewer-vm-connection.c b/src/virt-viewer-vm-connection.c > index e15fd4f..35d10ff 100644 > --- a/src/virt-viewer-vm-connection.c > +++ b/src/virt-viewer-vm-connection.c > @@ -87,6 +87,10 @@ virt_viewer_vm_connection_choose_name_dialog(GtkWindow *main_window, > if (dialog_response == GTK_RESPONSE_ACCEPT && > gtk_tree_selection_get_selected(selection, &model, &iter)) { > gtk_tree_model_get(model, &iter, 0, &vm_name, -1); > + } else { > + g_set_error_literal(error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED, > + _("No virtual machine was chosen")); > } > > gtk_widget_destroy(dialog); > diff --git a/src/virt-viewer.c b/src/virt-viewer.c > index 3a55057..38f9a28 100644 > --- a/src/virt-viewer.c > +++ b/src/virt-viewer.c > @@ -69,7 +69,7 @@ G_DEFINE_TYPE (VirtViewer, virt_viewer, VIRT_VIEWER_TYPE_APP) > static gboolean virt_viewer_initial_connect(VirtViewerApp *self, GError **error); > static gboolean virt_viewer_open_connection(VirtViewerApp *self, int *fd); > static void virt_viewer_deactivated(VirtViewerApp *self, gboolean connect_error); > -static gboolean virt_viewer_start(VirtViewerApp *self); > +static gboolean virt_viewer_start(VirtViewerApp *self, GError **error); > static void virt_viewer_dispose (GObject *object); > > static void > @@ -697,7 +697,7 @@ choose_vm(GtkWindow *main_window, > return dom; > } > > -static int virt_viewer_connect(VirtViewerApp *app); > +static int virt_viewer_connect(VirtViewerApp *app, GError **error); > > static gboolean > virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > @@ -713,7 +713,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > g_debug("initial connect"); > > if (!priv->conn && > - virt_viewer_connect(app) < 0) { > + virt_viewer_connect(app, &err) < 0) { > virt_viewer_app_show_status(app, _("Waiting for libvirt to start")); > goto wait; > } > @@ -877,7 +877,7 @@ virt_viewer_get_error_message_from_vir_error(VirtViewer *self, > } > > static int > -virt_viewer_connect(VirtViewerApp *app) > +virt_viewer_connect(VirtViewerApp *app, GError **err) > { > VirtViewer *self = VIRT_VIEWER(app); > VirtViewerPrivate *priv = self->priv; > @@ -906,28 +906,36 @@ virt_viewer_connect(VirtViewerApp *app) > if (!priv->conn) { > if (!priv->auth_cancelled) { > gchar *error_message = virt_viewer_get_error_message_from_vir_error(self, virGetLastError()); > - > + g_set_error_literal(&error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED, > + error_message); > virt_viewer_app_simple_message_dialog(app, error_message); > > g_free(error_message); > + } else { > + g_set_error_literal(&error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED, > + _("Authentication was cancelled")); > } > - > + g_propagate_error(err, error); > return -1; > } > > if (!virt_viewer_app_initial_connect(app, &error)) { > if (error != NULL) { > - VirtViewerWindow *main_window = virt_viewer_app_get_main_window(app); > - > - GtkWidget *dialog = gtk_message_dialog_new(virt_viewer_window_get_window(main_window), > - GTK_DIALOG_DESTROY_WITH_PARENT, > - GTK_MESSAGE_ERROR, > - GTK_BUTTONS_CLOSE, > - "Failed to connect: %s", > - error->message); > - gtk_dialog_run(GTK_DIALOG(dialog)); > - gtk_widget_destroy(GTK_WIDGET(dialog)); > - g_clear_error(&error); > + if (!g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED)) { > + VirtViewerWindow *main_window = virt_viewer_app_get_main_window(app); > + > + GtkWidget *dialog = gtk_message_dialog_new(virt_viewer_window_get_window(main_window), > + GTK_DIALOG_DESTROY_WITH_PARENT, > + GTK_MESSAGE_ERROR, > + GTK_BUTTONS_CLOSE, > + "Failed to connect: %s", > + error->message); > + gtk_dialog_run(GTK_DIALOG(dialog)); > + gtk_widget_destroy(GTK_WIDGET(dialog)); > + } > + g_propagate_error(err, error); > } > return -1; > } > @@ -955,16 +963,16 @@ virt_viewer_connect(VirtViewerApp *app) > } > > static gboolean > -virt_viewer_start(VirtViewerApp *app) > +virt_viewer_start(VirtViewerApp *app, GError **error) > { > virt_viewer_events_register(); > > virSetErrorFunc(NULL, virt_viewer_error_func); > > - if (virt_viewer_connect(app) < 0) > + if (virt_viewer_connect(app, error) < 0) > return FALSE; > > - return VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->start(app); > + return VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->start(app, error); > } > > VirtViewer * _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
