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
_______________________________________________
Spice-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/spice-devel
