To be completely honest, I think these get_user_cancelled() / set_user_cancelled() functions seem overly complicated. It is basically just used as a way to differentiate between different types of error conditions, which could be done by simply returning a different error code. I know that Marc-Andre removed the VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED error code a while back, but I really think it would be much cleaner to simply add it back again (or a simpler VIRT_VIEWER_ERROR_CANCELLED) rather than adding new API to test whether the dialog was cancelled or not. There's plenty of precedence for CANCELLED to be treated as a different error type (G_IO_ERROR_CANCELLED, VIRT_ERR_AUTH_CANCELLED, etc). Further comments inline. On Tue, 2015-03-17 at 15:42 +0100, Pavel Grunt wrote: > This applies for: > session authentication dialog > libvirt authentication dialog (e.g. virt-viewer --attach guest) > oVirt authentication dialog (e.g. remote-viewer ovirt://host/guest) > 'vm choose' dialog when connecting specifying the vm name > > Related to https://bugzilla.redhat.com/show_bug.cgi?id=1201604 > --- > v2: set virt_viewer_app_set_user_cancelled() in all possible situation > squashed https://www.redhat.com/archives/virt-tools-list/2015-March/msg00101.html > --- > src/remote-viewer-main.c | 6 +++++- > src/remote-viewer.c | 2 ++ > src/virt-viewer-app.c | 15 +++++++++++++++ > src/virt-viewer-app.h | 2 ++ > src/virt-viewer-main.c | 6 +++++- > src/virt-viewer-session-spice.c | 2 ++ > src/virt-viewer-session-vnc.c | 1 + > src/virt-viewer.c | 2 ++ > 8 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c > index e8784ba..9db2a17 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)) { if we do re-introduce the CANCELLED error code, we'll obviously need to modify virt_viewer_app_start() to take a GError** output parameter. > + if (virt_viewer_app_get_user_cancelled(app)) { > + ret = 0; > + } > 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..35493f3 100644 > --- a/src/remote-viewer.c > +++ b/src/remote-viewer.c > @@ -724,6 +724,7 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED RestProxyAuth *auth, > "oVirt", > NULL, > &username, &password); > + virt_viewer_app_set_user_cancelled(VIRT_VIEWER_APP(user_data), !success); As far as I can tell, setting this status does not actually do anything. authenticate_cb() obviously happens while gtk_main() is iterating, but the only place in this patch where you actually call _get_user_cancelled() is in remote-viewer-main.c and virt-viewer-main.c, and these calls all occur before we even enter the mainloop. Or am I missing something? > if (success) { > g_object_set(G_OBJECT(proxy), > "username", username, > @@ -878,6 +879,7 @@ create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err) > &vm_name, > vms, > &error); > + virt_viewer_app_set_user_cancelled(app, vm == NULL && error == NULL); > if (vm == NULL) { > goto error; > } > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c > index 8bf728f..a50f64a 100644 > --- a/src/virt-viewer-app.c > +++ b/src/virt-viewer-app.c > @@ -119,6 +119,7 @@ struct _VirtViewerAppPrivate { > gboolean enable_accel; > gboolean authretry; > gboolean started; > + gboolean user_cancelled; > gboolean fullscreen; > gboolean attach; > gboolean quitting; > @@ -2408,6 +2409,20 @@ virt_viewer_app_get_windows(VirtViewerApp *self) > return self->priv->windows; > } > > +void > +virt_viewer_app_set_user_cancelled(VirtViewerApp *self, gboolean user_cancelled) > +{ > + g_return_if_fail(VIRT_VIEWER_IS_APP(self)); > + self->priv->user_cancelled = user_cancelled; > +} > + > +gboolean > +virt_viewer_app_get_user_cancelled(VirtViewerApp *self) > +{ > + g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), NULL); > + return self->priv->user_cancelled; > +} > + > static void > share_folder_changed(VirtViewerApp *self) > { > diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h > index d214279..de4653e 100644 > --- a/src/virt-viewer-app.h > +++ b/src/virt-viewer-app.h > @@ -101,6 +101,8 @@ gint virt_viewer_app_get_n_initial_displays(VirtViewerApp* self); > gint virt_viewer_app_get_initial_monitor_for_display(VirtViewerApp* self, gint display); > void virt_viewer_app_set_enable_accel(VirtViewerApp *app, gboolean enable); > void virt_viewer_app_show_preferences(VirtViewerApp *app, GtkWidget *parent); > +void virt_viewer_app_set_user_cancelled(VirtViewerApp *app, gboolean user_cancelled); > +gboolean virt_viewer_app_get_user_cancelled(VirtViewerApp *app); > > G_END_DECLS > > diff --git a/src/virt-viewer-main.c b/src/virt-viewer-main.c > index 3fae955..9a53799 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))) { > + if (virt_viewer_app_get_user_cancelled(VIRT_VIEWER_APP(viewer))) { > + ret = 0; > + } > goto cleanup; > + } > > gtk_main(); > > diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c > index 5eb7234..043b07f 100644 > --- a/src/virt-viewer-session-spice.c > +++ b/src/virt-viewer-session-spice.c > @@ -561,6 +561,7 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel G_GNUC_UNUSED > NULL, > username_required ? &user : NULL, > &password); > + virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), !ret); Again, I don't think this call has any effect. It happens within the mainloop. > if (!ret) { > g_signal_emit_by_name(session, "session-cancelled"); > } else { > @@ -592,6 +593,7 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel G_GNUC_UNUSED > ret = virt_viewer_auth_collect_credentials(self->priv->main_window, > "proxy", NULL, > &user, &password); > + virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), !ret); And again. > if (!ret) { > g_signal_emit_by_name(session, "session-cancelled"); > } else { > diff --git a/src/virt-viewer-session-vnc.c b/src/virt-viewer-session-vnc.c > index 5a2dd86..29bde1d 100644 > --- a/src/virt-viewer-session-vnc.c > +++ b/src/virt-viewer-session-vnc.c > @@ -303,6 +303,7 @@ virt_viewer_session_vnc_auth_credential(GtkWidget *src G_GNUC_UNUSED, > wantUsername ? &username : NULL, > wantPassword ? &password : NULL); > > + virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), !ret); and again. > if (!ret) { > vnc_display_close(self->priv->vnc); > goto cleanup; > diff --git a/src/virt-viewer.c b/src/virt-viewer.c > index 3a55057..9be3a3b 100644 > --- a/src/virt-viewer.c > +++ b/src/virt-viewer.c > @@ -730,6 +730,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > &priv->domkey, > priv->conn, > &err); > + virt_viewer_app_set_user_cancelled(app, dom == NULL && err == NULL); > if (dom == NULL) { > goto cleanup; > } > @@ -904,6 +905,7 @@ virt_viewer_connect(VirtViewerApp *app) > &auth_libvirt, > oflags); > if (!priv->conn) { > + virt_viewer_app_set_user_cancelled(app, priv->auth_cancelled); > if (!priv->auth_cancelled) { > gchar *error_message = virt_viewer_get_error_message_from_vir_error(self, virGetLastError()); > _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
