Hi, > > 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? > It is handled in remote_viewer_start() and virt_viewer_connect(). I agree with you it makes sense to move it to *-viewer-main.c, If you don't mind I would like to do it in another patch. > > 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'. > Sure, I will send a patch for it. Thank you, Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
