[virt-tools-list] [PATCH virt-viewer v3 1/2] Exit normally when canceling dialog

[Pavel Grunt]

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