[virt-tools-list] [PATCH virt-viewer v2 2/3] Exit normally when canceling dialog
Jonathon Jongsma
jjongsma at redhat.com
Tue Mar 17 21:11:51 UTC 2015
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());
>
More information about the virt-tools-list
mailing list