[Spice-devel] [PATCH spice-gtk] channel: rely on couroutine instead of channel state
Christophe Fergeau
cfergeau at redhat.com
Sun Dec 23 09:54:35 PST 2012
Hey,
I've been trying to connect to oVirt instances using spice-gtk 0.15, and
this patch has been causing me pain (ie connection failures that go away
when I switch back to 0.14).
On Fri, Nov 30, 2012 at 09:12:03PM +0100, Marc-André Lureau wrote:
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 41ca6f8..cff047d 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1705,7 +1704,6 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
> CHANNEL_DEBUG(channel, "got channel caps %u:0x%X", i, *caps);
> }
>
> - c->state = SPICE_CHANNEL_STATE_AUTH;
> if (!spice_channel_test_common_capability(channel,
> SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION)) {
> CHANNEL_DEBUG(channel, "Server supports spice ticket auth only");
This function (spice_channel_recv_link_msg) can exit early without changing
c->state. This happens in particular when we need to switch to TLS
(c->peer_msg->error == SPICE_LINK_ERR_NEED_SECURED).
> @@ -2022,28 +2020,10 @@ static void spice_channel_iterate_write(SpiceChannel *channel)
> static void spice_channel_iterate_read(SpiceChannel *channel)
> {
> SpiceChannelPrivate *c = channel->priv;
> + g_return_if_fail(c->state != SPICE_CHANNEL_STATE_MIGRATING);
>
> - /* TODO: get rid of state, and use coroutine state */
> - switch (c->state) {
> - case SPICE_CHANNEL_STATE_LINK_HDR:
> - spice_channel_recv_link_hdr(channel);
> - break;
> - case SPICE_CHANNEL_STATE_LINK_MSG:
> - spice_channel_recv_link_msg(channel);
> - break;
> - case SPICE_CHANNEL_STATE_AUTH:
> - spice_channel_recv_auth(channel);
> - break;
> - case SPICE_CHANNEL_STATE_READY:
> - case SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE:
> - spice_channel_recv_msg(channel,
> - (handler_msg_in)SPICE_CHANNEL_GET_CLASS(channel)->handle_msg, NULL);
> - break;
> - case SPICE_CHANNEL_STATE_MIGRATING:
> - return;
> - default:
> - g_critical("unknown state %d", c->state);
> - }
My understanding is that in the TLS situation mentioned above, this code
would call spice_channel_recv_link_msg again as the state didn't change.
> @@ -2309,8 +2291,10 @@ connected:
> strerror(errno));
> }
>
> - c->state = SPICE_CHANNEL_STATE_LINK_HDR;
> spice_channel_send_link(channel);
> + spice_channel_recv_link_hdr(channel);
> + spice_channel_recv_link_msg(channel);
> + spice_channel_recv_auth(channel);
But now we always call spice_channel_recv_auth() right after calling
spice_channel_recv_link_msg(), even if the former exited early (TLS case).
The patch below helps me to get further (a connection is established with
remote-viewer, but it still thinks there's an error happening somewhere).
I'm not sure at all I'll have more time to dig into this, so sending this
to the mailing list so that this work does not get lost.
Christophe
diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index 369a0a5..346bebd 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1060,7 +1060,7 @@ static void spice_channel_send_spice_ticket(SpiceChannel *channel)
}
/* coroutine context */
-static void spice_channel_recv_auth(SpiceChannel *channel)
+static gboolean spice_channel_recv_auth(SpiceChannel *channel)
{
SpiceChannelPrivate *c = channel->priv;
uint32_t link_res;
@@ -1071,13 +1071,13 @@ static void spice_channel_recv_auth(SpiceChannel *channel)
CHANNEL_DEBUG(channel, "incomplete auth reply (%d/%" G_GSIZE_FORMAT ")",
rc, sizeof(link_res));
emit_main_context(channel, SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_LINK);
- return;
+ return FALSE;
}
if (link_res != SPICE_LINK_ERR_OK) {
CHANNEL_DEBUG(channel, "link result: reply %d", link_res);
emit_main_context(channel, SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_AUTH);
- return;
+ return FALSE;
}
c->state = SPICE_CHANNEL_STATE_READY;
@@ -1090,6 +1090,8 @@ static void spice_channel_recv_auth(SpiceChannel *channel)
if (c->state != SPICE_CHANNEL_STATE_MIGRATING)
spice_channel_up(channel);
+
+ return TRUE;
}
G_GNUC_INTERNAL
@@ -1175,7 +1177,7 @@ static void spice_channel_switch_protocol(SpiceChannel *channel, gint version)
}
/* coroutine context */
-static void spice_channel_recv_link_hdr(SpiceChannel *channel)
+static gboolean spice_channel_recv_link_hdr(SpiceChannel *channel)
{
SpiceChannelPrivate *c = channel->priv;
int rc;
@@ -1204,7 +1206,7 @@ static void spice_channel_recv_link_hdr(SpiceChannel *channel)
goto error;
}
- return;
+ return TRUE;
error:
/* Windows socket seems to give early CONNRESET errors. The server
@@ -1213,10 +1215,12 @@ error:
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);
- return;
+ return FALSE;
}
emit_main_context(channel, SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_LINK);
+
+ return FALSE;
}
#if HAVE_SASL
@@ -1652,14 +1656,16 @@ error:
#endif /* HAVE_SASL */
/* coroutine context */
-static void spice_channel_recv_link_msg(SpiceChannel *channel)
+static gboolean spice_channel_recv_link_msg(SpiceChannel *channel)
{
SpiceChannelPrivate *c;
int rc, num_caps, i;
uint32_t *caps;
+ gboolean res = FALSE;
- g_return_if_fail(channel != NULL);
- g_return_if_fail(channel->priv != NULL);
+
+ g_return_val_if_fail(channel != NULL, FALSE);
+ g_return_val_if_fail(channel->priv != NULL, FALSE);
c = channel->priv;
@@ -1670,7 +1676,7 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
g_critical("%s: %s: incomplete link reply (%d/%d)",
c->name, __FUNCTION__, rc, c->peer_hdr.size);
emit_main_context(channel, SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_LINK);
- return;
+ return res;
}
switch (c->peer_msg->error) {
case SPICE_LINK_ERR_OK:
@@ -1681,7 +1687,7 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
CHANNEL_DEBUG(channel, "switching to tls");
SPICE_CHANNEL_GET_CLASS(channel)->channel_disconnect(channel);
spice_channel_connect(channel);
- return;
+ return res;
default:
g_warning("%s: %s: unhandled error %d",
c->name, __FUNCTION__, c->peer_msg->error);
@@ -1708,6 +1714,7 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
CHANNEL_DEBUG(channel, "got channel caps %u:0x%X", i, *caps);
}
+ res = TRUE;
if (!spice_channel_test_common_capability(channel,
SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION)) {
CHANNEL_DEBUG(channel, "Server supports spice ticket auth only");
@@ -1735,11 +1742,13 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
c->use_mini_header = spice_channel_test_common_capability(channel,
SPICE_COMMON_CAP_MINI_HEADER);
CHANNEL_DEBUG(channel, "use mini header: %d", c->use_mini_header);
- return;
+ return res;
error:
SPICE_CHANNEL_GET_CLASS(channel)->channel_disconnect(channel);
emit_main_context(channel, SPICE_CHANNEL_EVENT, SPICE_CHANNEL_ERROR_LINK);
+
+ return res;
}
/* system context */
@@ -2296,9 +2305,15 @@ connected:
}
spice_channel_send_link(channel);
- spice_channel_recv_link_hdr(channel);
- spice_channel_recv_link_msg(channel);
- spice_channel_recv_auth(channel);
+ while (!spice_channel_recv_link_hdr(channel)) {
+ ;
+ }
+ while (!spice_channel_recv_link_msg(channel)) {
+ ;
+ }
+ while (!spice_channel_recv_auth(channel)) {
+ ;
+ }
while (spice_channel_iterate(channel))
;
-------------- 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/20121223/ffe8d1f5/attachment.pgp>
More information about the Spice-devel
mailing list