[Spice-devel] [PATCH 05/12] server: inputs_channel: use red_channel

[Hans de Goede]

Hi,

See comments inline.

On 12/06/2010 11:49 AM, Alon Levy wrote:
> ---
>   server/inputs_channel.c |  406 ++++++++++++++--------------------------------
>   1 files changed, 124 insertions(+), 282 deletions(-)
>
> diff --git a/server/inputs_channel.c b/server/inputs_channel.c
> index f3e71f3..2d76d03 100644
> --- a/server/inputs_channel.c
> +++ b/server/inputs_channel.c

<snip>

> @@ -153,132 +141,22 @@ const VDAgentMouseState *inputs_get_mouse_state(void)
>       return&inputs_channel->mouse_state;
>   }
>
> -static int handle_incoming(RedsStreamContext *peer, IncomingHandler *handler)
> +static uint8_t *inputs_channel_alloc_msg_rcv_buf(RedChannel *channel, SpiceDataHeader *msg_header)
>   {

<snip lots of - lines>

> +    InputsChannel *inputs_channel = SPICE_CONTAINEROF(channel, InputsChannel, base);
> +
> +    return inputs_channel->recv_buf;
>   }
>

Hmm, does red_channel guarantee that this function will only get called once? IOW will
red_channel never call this twice and expect to get 2 different buffers then ?

Also how does red_channel now the size of the buffer? I assume the size of the buffer
is supposed to be determined by the alloc function by checking the msg_header ?

Should we then not at least verify here that the static buffer is large enough ?

<snip lots of good stuff (mostly - lines)>

> -static int marshaller_outgoing_write(SpiceMarshaller *m,
> -                                     InputsChannel *state)
> +static void inputs_channel_release_pipe_item(RedChannel *channel,
> +    PipeItem *base, int item_pushed)
>   {
> -    SpiceDataHeader *header = (SpiceDataHeader *)spice_marshaller_get_ptr(m);
> +    // All PipeItems we push are InputsPipeItem
> +    InputsPipeItem *item = (InputsPipeItem*)base;
> +
> +    if (item->data) {
> +        free(item->data);
> +    }
> +}
> +

Hmm, we are responsible for allocating the item itself too in inputs_pipe_item_new
should we not release it then (just checking) ?

> +static void inputs_channel_send_item(RedChannel *channel, PipeItem *base)
> +{
> +    InputsPipeItem *item = SPICE_CONTAINEROF(base, InputsPipeItem, base);
> +    SpiceMarshaller *m = item->m;
>       uint8_t *data;
>       size_t len;
>       int free_data;
>
> +    red_channel_reset_send_data(channel);
> +    red_channel_init_send_data(channel, base->type, base);
>       spice_marshaller_flush(m);
> -    header->size = spice_marshaller_get_total_size(m) - sizeof(SpiceDataHeader);
> -
> +    // TODO: use spice_marshaller_fill_iovec. Right now we are doing something stupid,
> +    // namely copying twice. See reds.c.
>       data = spice_marshaller_linearize(m, 0,&len,&free_data);
> -
> -    if (outgoing_write(state->peer,&state->out_handler, data, len) != OUTGOING_OK) {
> -        return FALSE;
> -    }
> -
> -    if (free_data) {
> -        free(data);
> +    item->data = (free_data&&  len>  0) ? data : NULL;
> +    if (len>  0) {
> +        red_channel_add_buf(channel, data, len);
>       }
> -
> -    return TRUE;
> +    spice_marshaller_destroy(m);
> +    red_channel_begin_send_message(channel);
>   }
>

Hmm (again), so if spice_marshaller_linearize indicates the data should be free-ed by
the caller, we do so when the item gets destroyed (iow not right now), this makes me
assume that red_channel_add_buf does not copy the data, but merely stores a reference
to it. So what if spice_marshaller_linearize() does not say the caller should free
the buffer (iow it is returning some internal buffer). Won't that buffer get destroyed
then as soon as spice_marshaller_destroy gets called, iow before the channel is done
with it.

Note if my assumption is wrong and red_channel_add_buf does copy the data, why delay
freeing the data until the item gets destroyed and not free the data immediately after
the red_channel_add_buf call ?

<snip more stuff>

>   static void inputs_migrate(Channel *channel)
>   {
> -    InputsChannel *state = (InputsChannel *)channel->data;
> -    SpiceMarshaller *m;
> +    InputsPipeItem *pipe_item = inputs_pipe_item_new(inputs_channel, PIPE_ITEM_MIGRATE);
> +    SpiceMarshaller *m = pipe_item->m;
>       SpiceMsgMigrate migrate;
>
> -    m = marshaller_new_for_outgoing(state, SPICE_MSG_MIGRATE);
> -
> +    ASSERT(inputs_channel == (InputsChannel *)channel->data);
>       migrate.flags = 0;
>       spice_marshall_msg_migrate(m,&migrate);
> -
> -    if (!marshaller_outgoing_write(m, state)) {
> -        red_printf("write failed");
> -    }
> -    spice_marshaller_destroy(m);
> +    red_channel_pipe_add(&inputs_channel->base,&pipe_item->base);
>   }
>

Question, have you tested migration with these patches ?

<snip more stuff>


Phew all done :)

I'll review the rest of your patches after my lunch break. I'll also try
to remember to start up my irc client then unlike this morning :)


Regards,

Hans