Looks good. see comment bellow.
On 09/08/2013 02:59 PM, Marc-André Lureau wrote:
From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
Use of coroutines allow to simplify spice_channel_recv_msg(), it doesn't
need to keep current reading state, it can rely on the coroutine stack
for that.
---
gtk/channel-base.c | 1 +
gtk/spice-channel-priv.h | 3 +--
gtk/spice-channel.c | 50 +++++++++++++++---------------------------------
3 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/gtk/channel-base.c b/gtk/channel-base.c
index dff3024..6bfce3d 100644
--- a/gtk/channel-base.c
+++ b/gtk/channel-base.c
@@ -187,5 +187,6 @@ void spice_channel_handle_migrate(SpiceChannel *channel, SpiceMsgIn *in)
spice_marshaller_add(out->marshaller, data->data,
spice_header_get_msg_size(data->header, c->use_mini_header));
spice_msg_out_send_internal(out);
+ /* FIXME: who unref in? */
Nice catch. Can't the migrate_data msg be unref-ed here? after
spice_msg_out_send_internal completes sending it to the dest, nobody
uses it.
}
}
diff --git a/gtk/spice-channel-priv.h b/gtk/spice-channel-priv.h
index 9e70ff2..53830a8 100644
--- a/gtk/spice-channel-priv.h
+++ b/gtk/spice-channel-priv.h
@@ -59,7 +59,7 @@ struct _SpiceMsgIn {
SpiceChannel *channel;
uint8_t header[MAX_SPICE_DATA_HEADER_SIZE];
uint8_t *data;
- int hpos,dpos;
+ int dpos;
uint8_t *parsed;
size_t psize;
message_destructor_t pfree;
@@ -122,7 +122,6 @@ struct _SpiceChannelPrivate {
SpiceLinkReply* peer_msg;
int peer_pos;
- SpiceMsgIn *msg_in;
int message_ack_window;
int message_ack_count;
diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index 4b80029..00e544b 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1770,43 +1770,26 @@ void spice_channel_recv_msg(SpiceChannel *channel,
{
SpiceChannelPrivate *c = channel->priv;
SpiceMsgIn *in;
- int header_size;
int msg_size;
int msg_type;
int sub_list_offset = 0;
- int rc;
- if (!c->msg_in) {
- c->msg_in = spice_msg_in_new(channel);
- }
- in = c->msg_in;
- header_size = spice_header_get_header_size(c->use_mini_header);
+ in = spice_msg_in_new(channel);
/* receive message */
- if (in->hpos < header_size) {
- rc = spice_channel_read(channel, in->header + in->hpos,
- header_size - in->hpos);
- if (rc < 0) {
- g_critical("recv hdr: %s", strerror(errno));
- return;
- }
- in->hpos += rc;
- if (in->hpos < header_size)
- return;
- in->data = spice_malloc(spice_header_get_msg_size(in->header, c->use_mini_header));
- }
+ spice_channel_read(channel, in->header,
+ spice_header_get_header_size(c->use_mini_header));
+ if (c->has_error)
+ goto end;
+
msg_size = spice_header_get_msg_size(in->header, c->use_mini_header);
- if (in->dpos < msg_size) {
- rc = spice_channel_read(channel, in->data + in->dpos,
- msg_size - in->dpos);
- if (rc < 0) {
- g_critical("recv msg: %s", strerror(errno));
- return;
- }
- in->dpos += rc;
- if (in->dpos < msg_size)
- return;
- }
+ /* FIXME: do not allow others to take ref on in, and use realloc here?
+ * this would avoid malloc/free on each message?
+ */
+ in->data = spice_malloc(msg_size);
+ spice_channel_read(channel, in->data, msg_size);
+ if (c->has_error)
+ goto end;
msg_type = spice_header_get_msg_type(in->header, c->use_mini_header);
sub_list_offset = spice_header_get_msg_sub_list(in->header, c->use_mini_header);
@@ -1829,7 +1812,7 @@ void spice_channel_recv_msg(SpiceChannel *channel,
if (sub_in->parsed == NULL) {
g_critical("failed to parse sub-message: %s type %d",
c->name, spice_header_get_msg_type(sub_in->header, c->use_mini_header));
- return;
+ goto end;
}
msg_handler(channel, sub_in, data);
spice_msg_in_unref(sub_in);
@@ -1851,7 +1834,7 @@ void spice_channel_recv_msg(SpiceChannel *channel,
}
/* parse message */
- in->parsed = c->parser(in->data, in->data + in->dpos, msg_type,
+ in->parsed = c->parser(in->data, in->data + msg_size, msg_type,
c->peer_hdr.minor_version, &in->psize, &in->pfree);
if (in->parsed == NULL) {
g_critical("failed to parse message: %s type %d",
@@ -1860,7 +1843,6 @@ void spice_channel_recv_msg(SpiceChannel *channel,
}
/* process message */
- c->msg_in = NULL; /* the function is reentrant, reset state */
/* spice_msg_in_hexdump(in); */
msg_handler(channel, in, data);
@@ -1869,8 +1851,6 @@ end:
* to c->in_serial (the server can sometimes skip serials) */
c->last_message_serial = spice_header_get_in_msg_serial(in);
c->in_serial++;
- /* release message */
- c->msg_in = NULL;
spice_msg_in_unref(in);
}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel