[Spice-devel] [PATCH spice-gtk 10/10] gtk: simplify spice_channel_recv_msg

[Marc-André Lureau]

On Mon, Sep 9, 2013 at 11:11 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> Looks good. see comment bellow.
>
> On 09/08/2013 02:59 PM, Marc-André Lureau wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau at 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