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

[Victor Toso]

On Fri, Sep 07, 2018 at 04:40:16AM -0400, Frediano Ziglio wrote:
> > 
> > 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).

That would require host configuration to use socket. Neither of
SPICE_CHANNEL_STATE_RECONNECTING and
SPICE_CHANNEL_STATE_SWITCHING should happen in this scenario but
I could add a sanity check on

    spice_session_get_client_provided_socket(c->session);

I'll take your suggestion but keeping the SEGVs comment ;)
I'll send v2 later today.

Thanks for the review,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180907/cccb7d6d/attachment-0001.sig>