[Spice-devel] [spice-gtk 2/2] Fix switch to old SPICE protocol

[Marc-André Lureau]

Looks good, thanks for fixing this.

ack

On Thu, Oct 10, 2013 at 5:15 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> After the previous commit, spice_channel_switch_protocol() is now
> called when needed, but it's not doing anything. What happens is
> that spice_channel_switch_protocol() triggers a channel disconnection
> and then it queues an idle to reconnect (after having changed the
> protocol version to be used).
>
> When spice_channel_recv_link_hdr() returns, we need to jump out of
> the coroutine to let the idle trigger and the new channel coroutine
> be started. But jumping out of the coroutine will call channel_disconnect()
> which calls channel_reset() which disables the idle switch_protocol()
> just queued. This causes the connection attempt to be apparently
> stuck with nothing happening.
>
> Falling back to the older SPICE protocol is not the only situation
> when we need to drop the current connection attempt and reconnect,
> we also need to do that when the remote server returns
> SPICE_LINK_ERR_NEED_SECURED to let the client know it needs to use
> a secure port for this channel. This is handled by the 'switch_tls'
> variable set in spice_channel_recv_link_msg and handled in
> spice_channel_coroutine().
>
> 'switch_tls' does the same thing as spice_channel_switch_protocol(),
> except that it calls spice_channel_connect() after channel_disconnect()
> has been called, which means the idle queued by channel_connect()
> won't get cleared.
>
> This all that commit does, remove the spice_channel_switch_protocol()
> method and use the same codepath as 'switch_tls' to handle the
> reconnection.
> ---
>  gtk/spice-channel.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index c0e7bba..08418f7 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1169,21 +1169,13 @@ static void spice_channel_send_link(SpiceChannel *channel)
>      free(buffer);
>  }
>
> -static void spice_channel_switch_protocol(SpiceChannel *channel, gint version)
> -{
> -    SpiceChannelPrivate *c = channel->priv;
> -
> -    g_object_set(c->session, "protocol", version, NULL);
> -    SPICE_CHANNEL_GET_CLASS(channel)->channel_disconnect(channel);
> -    spice_channel_connect(channel);
> -}
> -
>  /* coroutine context */
> -static gboolean spice_channel_recv_link_hdr(SpiceChannel *channel)
> +static gboolean spice_channel_recv_link_hdr(SpiceChannel *channel, gboolean *switch_protocol)
>  {
>      SpiceChannelPrivate *c = channel->priv;
>      int rc;
>
> +    *switch_protocol = FALSE;
>      rc = spice_channel_read(channel, &c->peer_hdr, sizeof(c->peer_hdr));
>      if (rc != sizeof(c->peer_hdr)) {
>          g_warning("incomplete link header (%d/%" G_GSIZE_FORMAT ")",
> @@ -1216,7 +1208,8 @@ error:
>         incompatible. Try with the oldest protocol in this case: */
>      if (c->link_hdr.major_version != 1) {
>          SPICE_DEBUG("%s: error, switching to protocol 1 (spice 0.4)", c->name);
> -        spice_channel_switch_protocol(channel, 1);
> +        *switch_protocol = TRUE;
> +        g_object_set(c->session, "protocol", 1, NULL);
>          return FALSE;
>      }
>
> @@ -2228,6 +2221,7 @@ static void *spice_channel_coroutine(void *data)
>      guint verify;
>      int rc, delay_val = 1;
>      gboolean switch_tls = FALSE;
> +    gboolean switch_protocol = FALSE;
>
>      CHANNEL_DEBUG(channel, "Started background coroutine %p", &c->coroutine);
>
> @@ -2350,7 +2344,7 @@ connected:
>      }
>
>      spice_channel_send_link(channel);
> -    if (spice_channel_recv_link_hdr(channel) == FALSE)
> +    if (spice_channel_recv_link_hdr(channel, &switch_protocol) == FALSE)
>          goto cleanup;
>      spice_channel_recv_link_msg(channel, &switch_tls);
>      if (switch_tls)
> @@ -2365,8 +2359,8 @@ cleanup:
>
>      SPICE_CHANNEL_GET_CLASS(channel)->channel_disconnect(channel);
>
> -    if (switch_tls && !c->tls) {
> -        c->tls = true;
> +    if (switch_protocol || (switch_tls && !c->tls)) {
> +        c->tls = switch_tls;
>          spice_channel_connect(channel);
>          g_object_unref(channel);
>      } else
> --
> 1.8.3.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau