Hi, thanks for the review. > > On Tue, Mar 17, 2015 at 10:08 AM, Pavel Grunt <pgrunt@xxxxxxxxxx> > 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 > > --- > > src/remote-viewer.c | 4 ++++ > > src/virt-viewer-session-spice.c | 2 ++ > > src/virt-viewer-session-vnc.c | 1 + > > src/virt-viewer.c | 7 ++++++- > > 4 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > > index f9f4f92..c6c151b 100644 > > --- a/src/remote-viewer.c > > +++ b/src/remote-viewer.c > > @@ -876,6 +876,7 @@ create_ovirt_session(VirtViewerApp *app, const > > char *uri, GError **err) > > > > api = ovirt_proxy_fetch_api(proxy, &error); > > if (authenticate_info.dialog_cancelled) { > > + virt_viewer_app_set_user_cancelled(app, TRUE); > > g_clear_error(&error); > > goto error; > > } > > @@ -897,6 +898,9 @@ create_ovirt_session(VirtViewerApp *app, const > > char *uri, GError **err) > > vms, > > &error); > > if (vm == NULL) { > > + if (error == NULL) { > > + virt_viewer_app_set_user_cancelled(app, TRUE); > > + } > > goto error; > > } > > } > > diff --git a/src/virt-viewer-session-spice.c > > b/src/virt-viewer-session-spice.c > > index 5eb7234..93906ae 100644 > > --- a/src/virt-viewer-session-spice.c > > +++ b/src/virt-viewer-session-spice.c > > @@ -562,6 +562,7 @@ > > virt_viewer_session_spice_main_channel_event(SpiceChannel *channel > > G_GNUC_UNUSED > > username_required > > ? &user : > > NULL, > > &password); > > if (!ret) { > > + > > virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), > > TRUE); > > g_signal_emit_by_name(session, "session-cancelled"); > > } else { > > gboolean openfd; > > @@ -593,6 +594,7 @@ > > virt_viewer_session_spice_main_channel_event(SpiceChannel *channel > > G_GNUC_UNUSED > > "proxy", > > NULL, > > &user, > > &password); > > if (!ret) { > > + > > virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), > > TRUE); > > g_signal_emit_by_name(session, > > "session-cancelled"); > > } else { > > spice_uri_set_user(proxy, user); > > diff --git a/src/virt-viewer-session-vnc.c > > b/src/virt-viewer-session-vnc.c > > index 5a2dd86..68a6719 100644 > > --- a/src/virt-viewer-session-vnc.c > > +++ b/src/virt-viewer-session-vnc.c > > @@ -304,6 +304,7 @@ > > virt_viewer_session_vnc_auth_credential(GtkWidget *src > > G_GNUC_UNUSED, > > wantPassword > > ? > > &password > > : > > NULL); > > > > if (!ret) { > > + > > virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session), > > TRUE); > > vnc_display_close(self->priv->vnc); > > goto cleanup; > > } > > diff --git a/src/virt-viewer.c b/src/virt-viewer.c > > index acad6c6..65ec4e3 100644 > > --- a/src/virt-viewer.c > > +++ b/src/virt-viewer.c > > @@ -730,7 +730,10 @@ virt_viewer_initial_connect(VirtViewerApp > > *app, GError **error) > > &priv->domkey, > > priv->conn, > > &err); > > - if (dom == NULL && err != NULL) { > > + if (dom == NULL) { > > + if (err == NULL) { > > + virt_viewer_app_set_user_cancelled(app, TRUE); > > + } > > Here you changed a bit the logic and I'm not sure if it was > intentional or not. > I meant, the only way to go to clean up was: dom == NULL *and* err != > NULL. After your changes, dom == NULL is enough to go to clean up. > Is it intentional? Are you missing an else for the "err == NULL" if? > It was intentional but it should be in different patch. I will use this: if (dom == NULL && err != NULL) { goto cleanup; } if (dom == NULL && err == NULL) { virt_viewer_app_set_user_cancelled(app, TRUE); } > > goto cleanup; > > } > > } > > @@ -910,6 +913,8 @@ virt_viewer_connect(VirtViewerApp *app) > > virt_viewer_app_simple_message_dialog(app, > > error_message); > > > > g_free(error_message); > > + } else { > > + virt_viewer_app_set_user_cancelled(app, TRUE); > > } > > > > return -1; > > -- > > 2.3.2 > > > > _______________________________________________ > > virt-tools-list mailing list > > virt-tools-list@xxxxxxxxxx > > https://www.redhat.com/mailman/listinfo/virt-tools-list > > A more general comment ... I'd set > virt_viewer_app_set_user_cancelled() in all possible situations, > being > TRUE or FALSE and not only when it is TRUE. > Seems a bit more clear, at least for me :-) > sure, it makes sense. I will send v2. Thank you, Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
