[Spice-devel] [spice-gtk v1 1/4] spice-channel: Properly error out if reconnect fails

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> The channel_connect() function could fail leading to a spice-channel
> existing as zombie (its coroutine return soon after).
> 
> Check if channel_connect() fails and give a proper error signal to
> user when that happens.
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1625550
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-channel.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 0a5437c..48e3265 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -2675,8 +2675,12 @@ cleanup:
>      if (c->state == SPICE_CHANNEL_STATE_RECONNECTING ||
>          c->state == SPICE_CHANNEL_STATE_SWITCHING) {
>          g_warn_if_fail(c->event == SPICE_CHANNEL_NONE);
> -        channel_connect(channel, c->tls);
> -        g_object_unref(channel);
> +        if (channel_connect(channel, c->tls)) {
> +            g_object_unref(channel);
> +        } else {
> +            c->event = SPICE_CHANNEL_ERROR_CONNECT;
> +            g_idle_add(spice_channel_delayed_unref, data);
> +        }
>      } else
>          g_idle_add(spice_channel_delayed_unref, data);
>  

Reading at all "cleanup" code is a bit confusing.
The comment "Co-routine exits now - the SpiceChannel object "...
actually happens only if you call g_object_unref, so only
after calling channel_connect but the comment is in more paths.
I would instead add a "return NULL" line after g_object_unref
so to make code clear even without the comment, remove the comment
and the else too, something like:


diff --git a/src/spice-channel.c b/src/spice-channel.c
index d1a72fc..2506431 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -2677,11 +2677,10 @@ cleanup:
         g_warn_if_fail(c->event == SPICE_CHANNEL_NONE);
         channel_connect(channel, c->tls);
         g_object_unref(channel);
-    } else
-        g_idle_add(spice_channel_delayed_unref, data);
+        return NULL;
+    }
 
-    /* Co-routine exits now - the SpiceChannel object may no longer exist,
-       so don't do anything else now unless you like SEGVs */
+    g_idle_add(spice_channel_delayed_unref, channel);
     return NULL;
 }
 

This would make this patch something like


diff --git a/src/spice-channel.c b/src/spice-channel.c
index 2506431..e56ec35 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -2675,9 +2675,11 @@ cleanup:
     if (c->state == SPICE_CHANNEL_STATE_RECONNECTING ||
         c->state == SPICE_CHANNEL_STATE_SWITCHING) {
         g_warn_if_fail(c->event == SPICE_CHANNEL_NONE);
-        channel_connect(channel, c->tls);
-        g_object_unref(channel);
-        return NULL;
+        if (channel_connect(channel, c->tls)) {
+            g_object_unref(channel);
+            return NULL;
+        }
+        c->event = SPICE_CHANNEL_ERROR_CONNECT;
     }
 
     g_idle_add(spice_channel_delayed_unref, channel);


Note also that channel_connect returns true also if client
provided the socket. In this case g_object_unref will be
called from spice_channel_coroutine (so coroutine context)
which apparently from spice_channel_delayed_unref comment
you want to avoid (unless you are sure there's another
reference that prevents real object destruction).

Frediano