[Spice-devel] [PATCH spice-gtk 2/2] Fix switching to TLS regression

[Christophe Fergeau]

Hey,

First thing, this patch fixes the TLS issues I was seeing with virt-viewer,
so thanks for the quick fix!

On Sun, Dec 23, 2012 at 10:35:34PM +0100, Marc-André Lureau wrote:
> The commit fcbbc248a8f885f9a9a6e7c47d7aae0c1ab3cd1b changed the way a
> channel coroutine is exiting. In particular, it was going through the
> coroutine main cleanup (finishing in main coroutine) while switching
> to TLS is recycling the channel. That part of the code is a bit
> difficult to grasp, but with this refactoring, I think it makes it
> easier to understand the reconnection.
> ---
>  gtk/spice-channel.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 369a0a5..14ced89 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1652,7 +1652,7 @@ error:
>  #endif /* HAVE_SASL */
>  
>  /* coroutine context */
> -static void spice_channel_recv_link_msg(SpiceChannel *channel)
> +static void spice_channel_recv_link_msg(SpiceChannel *channel, gboolean *switch_tls)
>  {
>      SpiceChannelPrivate *c;
>      int rc, num_caps, i;
> @@ -1677,10 +1677,8 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
>          /* nothing */
>          break;
>      case SPICE_LINK_ERR_NEED_SECURED:
> -        c->tls = true;
> +        *switch_tls = true;
>          CHANNEL_DEBUG(channel, "switching to tls");
> -        SPICE_CHANNEL_GET_CLASS(channel)->channel_disconnect(channel);
> -        spice_channel_connect(channel);
>          return;
>      default:
>          g_warning("%s: %s: unhandled error %d",
> @@ -2175,6 +2173,7 @@ static void *spice_channel_coroutine(void *data)
>      SpiceChannelPrivate *c = channel->priv;
>      guint verify;
>      int rc, delay_val = 1;
> +    gboolean switch_tls = FALSE;
>  
>      CHANNEL_DEBUG(channel, "Started background coroutine %p", &c->coroutine);
>  
> @@ -2297,28 +2296,26 @@ connected:
>  
>      spice_channel_send_link(channel);
>      spice_channel_recv_link_hdr(channel);
> -    spice_channel_recv_link_msg(channel);
> +    spice_channel_recv_link_msg(channel, &switch_tls);
> +    if (switch_tls)
> +        goto cleanup;

There are other cases when spice_channel_recv_link_msg can exit early in
error cases. It will have called emit_main_context(channel,
SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_LINK); before exiting when this
happens. Is it enough to not cause issues when spice_channel_recv_auth is
called after an early return from spice_channel_recv_link_msg?
Apart from this concern, patch seems ok.

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