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

[Christophe Fergeau]

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140311/a6ad710a/attachment-0001.pgp>