On Mon, Sep 9, 2013 at 11:11 PM, Yonit Halperin <yhalperi@xxxxxxxxxx> wrote:
> 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);
>> }
>>
>>
>
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel