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

[Alon Levy]

On Mon, Dec 06, 2010 at 12:36:57PM +0100, Hans de Goede wrote:
> 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 ?

The only user for this buffer is in red_channel.c:red_peer_handle_incoming, it is called
once per parsed message, and used only between the calls. That is:

repeat:
 msg = alloc_msg_rcv_buf
 <use msg>
 release_msg_rcv_buf

It is never 

> 
> 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 ?
Yes, but I don't check it.

> 
> Should we then not at least verify here that the static buffer is large enough ?
Yes, I'll add that.

> 
> <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) ?

Will fold in later patch as requested.

> 
> >+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.

You're correct, the destroy is premature, since linearize can return with free_data==False,
in which case data is pointing to marshaller memory, which becomes invalid after destroy.

Do note that later patches change this to do away with the linearize (which is just left
atm to make the patches easier to read, later all writes become through the marshaller using
the automatically generated marshallers). v2 will fix.

> 
> 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 ?

So you are correct that I can release immediately  in that case. Basically I should if
on the free_data and destroy if it is true, otherwise destroy only on release. But that requires
adding another variable to keep track of length 0 and free_data==True, so I just punt
spice_marshaller_destroy to inputs_channel_release_pipe_item.

> 
> <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>
> 
> 

I'll do a test, though I'm afraid of what I find :/

> 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