[virt-tools-list] [PATCH virt-viewer v3 1/2] Exit normally when canceling dialog
Jonathon Jongsma
jjongsma at redhat.com
Wed Mar 18 19:43:36 UTC 2015
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 *
More information about the virt-tools-list
mailing list