[Spice-devel] [PATCHv3 spice-gtk 13/14] channel: add spice_channel_get_error()

[Marc-André Lureau]

----- Original Message -----
> Hey,
> 
> On Mon, Feb 17, 2014 at 10:35:52PM +0100, Marc-André Lureau wrote:
> > Add a function to retrieve the last GError from a channel, this may be
> > useful to provide additional error details to the client.
> > ---
> >  doc/reference/spice-gtk-sections.txt |  1 +
> >  gtk/map-file                         |  1 +
> >  gtk/spice-channel-priv.h             |  1 +
> >  gtk/spice-channel.c                  | 30 +++++++++++++++++++++++++++---
> >  gtk/spice-channel.h                  |  2 ++
> >  gtk/spice-glib-sym-file              |  1 +
> >  gtk/spice-session-priv.h             |  2 +-
> >  gtk/spice-session.c                  |  6 +++---
> >  gtk/spicy.c                          |  6 ++++++
> >  9 files changed, 43 insertions(+), 7 deletions(-)
> > 
> > diff --git a/doc/reference/spice-gtk-sections.txt
> > b/doc/reference/spice-gtk-sections.txt
> > index 411ca0e..9f0cf67 100644
> > diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> > index 1b8ec10..a9c1076 100644
> > --- a/gtk/spice-channel.c
> > +++ b/gtk/spice-channel.c
> >  static void *spice_channel_coroutine(void *data)
> > @@ -2257,15 +2280,16 @@ static void *spice_channel_coroutine(void *data)
> >  
> >  
> >  reconnect:
> > -    c->conn = spice_session_channel_open_host(c->session, channel,
> > &c->tls);
> > +    c->conn = spice_session_channel_open_host(c->session, channel,
> > &c->tls, &c->error);
> >      if (c->conn == NULL) {
> > -        if (!c->tls) {
> > +        if (!c->error && !c->tls) {
> 
> This bit...
> 
> >              CHANNEL_DEBUG(channel, "trying with TLS port");
> >              c->tls = true; /* FIXME: does that really work with provided
> >              fd */
> >              goto reconnect;
> >          } else {
> >              CHANNEL_DEBUG(channel, "Connect error");
> >              emit_main_context(channel, SPICE_CHANNEL_EVENT,
> >              SPICE_CHANNEL_ERROR_CONNECT);
> > +            g_clear_error(&c->error);
> >              goto cleanup;
> >          }
> >      }
> > diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> > index 19057bf..5e76b5f 100644
> > --- a/gtk/spice-session.c
> > +++ b/gtk/spice-session.c
> > @@ -1845,8 +1845,8 @@ GSocketConnection*
> > spice_session_channel_open_host(SpiceSession *session, SpiceC
> >  #endif
> >  
> >      if (open_host.error != NULL) {
> > -        g_warning("open host: %s", open_host.error->message);
> > -        g_clear_error(&open_host.error);
> > +        SPICE_DEBUG("open host: %s", open_host.error->message);
> > +        g_propagate_error(error, open_host.error);
> 
> ... and this bit are causing fallback to using a TLS connection to fail.
> I've tested with -spice tls-port=5911,... on qemu command line, and
> remote-viewer spice://localhost?port=5910&tls-port=5911 to start
> remote-viewer. remote-viewer first tries to connect to the non-tls port
> which fails with G_IO_ERROR_CONNECTION_REFUSED, and then we report the
> error back to the user rather than falling back to TLS.
> 
> I'd go with something like
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 632a286..56e1075 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -2276,7 +2276,17 @@ static void *spice_channel_coroutine(void *data)
>  reconnect:
>      c->conn = spice_session_channel_open_host(c->session, channel,
> &c->tls, &c->error);
>      if (c->conn == NULL) {
> -        if (!c->error && !c->tls) {
> +        gint tls_port;
> +        g_object_get(G_OBJECT(c->session),
> +                     "tls-port", &tls_port,
> +                     NULL);
> +        if (!c->tls && tls_port) {
> +            if (c->error) {
> +                CHANNEL_DEBUG(channel,
> +                              "Ignoring connection error ('%s'), retrying
> with TLS",
> +                              c->error->message);
> +                g_clear_error(&c->error);
> +            }
>              CHANNEL_DEBUG(channel, "trying with TLS port");
>              c->tls = true; /* FIXME: does that really work with provided
> fd */
>              goto reconnect;
> 
> This avoids potentially losing the non-TLS error if remote-viewer is
> started with only a non-TLS port. However, this will lose non-TLS
> connection errors when starting QEMU with just a non-TLS port and starting
> remote-viewer with both TLS and non-TLS ports.

Ok, we should let go proxy errors though... If we let go only proxy errors, I would filter that from the propagate in spice-session, not here.

> 
> Christophe
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>