On Mon, Sep 9, 2013 at 11:11 PM, Yonit Halperin <yhalp...@redhat.com> wrote: > Looks good. see comment bellow. > > On 09/08/2013 02:59 PM, Marc-André Lureau wrote: >> >> From: Marc-André Lureau <marcandre.lur...@gmail.com> >> >> 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); >> } >> >> >
-- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel