[Spice-devel] [RFC v4 21/62] server/inputs_channel: support multiple clients
Alon Levy
alevy at redhat.com
Thu May 5 00:53:35 PDT 2011
On Tue, May 03, 2011 at 01:56:43AM +0200, Marc-André Lureau wrote:
> On Tue, Apr 26, 2011 at 12:54 PM, Alon Levy <alevy at redhat.com> wrote:
> > from server events are broadcast - leds change. The rest is client
> > to server, so it is just passed on.
> > ---
> > server/inputs_channel.c | 77 +++++++++++++++++++++++------------------------
> > 1 files changed, 38 insertions(+), 39 deletions(-)
> >
> > diff --git a/server/inputs_channel.c b/server/inputs_channel.c
> > index 9fc7bca..581a35f 100644
> > --- a/server/inputs_channel.c
> > +++ b/server/inputs_channel.c
> > @@ -56,11 +56,15 @@ struct SpiceTabletState {
> > int dummy;
> > };
> >
> > +typedef struct InputsChannelClient {
> > + RedChannelClient base;
> > + uint32_t motion_count;
> > +} InputsChannelClient;
> > +
> > typedef struct InputsChannel {
> > RedChannel base;
> > uint8_t recv_buf[RECEIVE_BUF_SIZE];
> > VDAgentMouseState mouse_state;
> > - uint32_t motion_count;
> > } InputsChannel;
> >
> > enum {
> > @@ -218,33 +222,32 @@ static uint8_t kbd_get_leds(SpiceKbdInstance *sin)
> > return sif->get_leds(sin);
> > }
> >
> > -static InputsPipeItem *inputs_pipe_item_new(RedChannelClient *rcc, int type)
> > +static PipeItem *inputs_pipe_item_new(RedChannelClient *rcc, void *type, int num)
> > {
> > InputsPipeItem *item = spice_malloc(sizeof(InputsPipeItem));
> > - red_channel_pipe_item_init(rcc->channel, &item->base, type);
> > - return item;
> > + red_channel_pipe_item_init(rcc->channel, &item->base, (uint64_t)type);
> > + return &item->base;
> > }
>
>
> Why uint64_t when red_channel_pipe_item_init() takes a "int type" argument?
>
> (same comment for many similar code below)
>
dropped inputs_pipe_item_new completely, now use red_channel_pipes_add_type
> > -static KeyModifiersPipeItem *inputs_key_modifiers_item_new(
> > - InputsChannel *inputs_channel, uint8_t modifiers)
> > +static PipeItem *inputs_key_modifiers_item_new(
> > + RedChannelClient *rcc, void *data, int num)
> > {
> > KeyModifiersPipeItem *item = spice_malloc(sizeof(KeyModifiersPipeItem));
> > + uint8_t modifiers = (uint8_t)(uint64_t)data;
>
> (uint8_t)(uint64_t)data; ??
>
cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > - red_channel_pipe_item_init(&inputs_channel->base, &item->base,
> > + red_channel_pipe_item_init(rcc->channel, &item->base,
> > PIPE_ITEM_KEY_MODIFIERS);
> > item->modifiers = modifiers;
> > - return item;
> > + return &item->base;
> > }
> >
> > // Right now every PipeItem we add is an InputsPipeItem, later maybe make it simpler
> > // for type only PipeItems
> > -static void inputs_pipe_add_type(RedChannelClient *rcc, int type)
> > +static void inputs_pipe_add_type(InputsChannelClient *icc, int type)
> > {
> > - InputsPipeItem* pipe_item;
> > -
> > - pipe_item = inputs_pipe_item_new(rcc, type);
> > - red_channel_client_pipe_add_push(rcc, &pipe_item->base);
> > + red_channel_client_pipe_add_push(&icc->base,
> > + inputs_pipe_item_new(&icc->base, (void*)(uint64_t)type, 0));
> > }
> >
> > static void inputs_channel_release_pipe_item(RedChannelClient *rcc,
> > @@ -292,6 +295,7 @@ static void inputs_channel_send_item(RedChannelClient *rcc, PipeItem *base)
> > static int inputs_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint16_t type, void *message)
> > {
> > InputsChannel *inputs_channel = (InputsChannel *)rcc->channel;
> > + InputsChannelClient *icc = (InputsChannelClient *)rcc;
> > uint8_t *buf = (uint8_t *)message;
> >
> > ASSERT(g_inputs_channel == inputs_channel);
> > @@ -315,8 +319,8 @@ static int inputs_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, ui
> > case SPICE_MSGC_INPUTS_MOUSE_MOTION: {
> > SpiceMsgcMouseMotion *mouse_motion = (SpiceMsgcMouseMotion *)buf;
> >
> > - if (++inputs_channel->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0) {
> > - inputs_pipe_add_type(rcc, PIPE_ITEM_MOUSE_MOTION_ACK);
> > + if (++icc->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0) {
> > + inputs_pipe_add_type(icc, PIPE_ITEM_MOUSE_MOTION_ACK);
> > }
> > if (mouse && reds_get_mouse_mode() == SPICE_MOUSE_MODE_SERVER) {
> > SpiceMouseInterface *sif;
> > @@ -330,8 +334,8 @@ static int inputs_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, ui
> > case SPICE_MSGC_INPUTS_MOUSE_POSITION: {
> > SpiceMsgcMousePosition *pos = (SpiceMsgcMousePosition *)buf;
> >
> > - if (++inputs_channel->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0) {
> > - inputs_pipe_add_type(rcc, PIPE_ITEM_MOUSE_MOTION_ACK);
> > + if (++icc->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0) {
> > + inputs_pipe_add_type(icc, PIPE_ITEM_MOUSE_MOTION_ACK);
> > }
> > if (reds_get_mouse_mode() != SPICE_MOUSE_MODE_CLIENT) {
> > break;
> > @@ -472,12 +476,10 @@ static void inputs_shutdown(Channel *channel)
> > static void inputs_migrate(Channel *channel)
> > {
> > InputsChannel *inputs_channel = channel->data;
> > - RedChannelClient *rcc = inputs_channel->base.rcc;
> > - InputsPipeItem *item;
> >
> > ASSERT(g_inputs_channel == (InputsChannel *)channel->data);
> > - item = inputs_pipe_item_new(rcc, PIPE_ITEM_MIGRATE);
> > - red_channel_client_pipe_add_push(rcc, &item->base);
> > + red_channel_pipes_new_add_push(&inputs_channel->base,
> > + inputs_pipe_item_new, (void*)(uint64_t)PIPE_ITEM_MIGRATE);
> > }
>
> I can't resist to say that the previous code was easier to read, for a
> very similar behavior.
>
What specifically? all usage of red_channel_pipes_new_add_push that I could turn back
to red_channel_pipes_add_type were done. So specifically the one above (PIPE_ITEM_MIGRATE)
is readable imho. But some places require the factory method approach of
red_channel_pipes_new_add_push, I can change it a little I guess, but I can't think of anything
that would be much prettier, and to be honest I think this is relatively readable. Any suggestions
welcome.
> > static void inputs_pipe_add_init(RedChannelClient *rcc)
> > @@ -514,17 +516,17 @@ static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem *item)
> > {
> > }
> >
> > -static void inputs_link(Channel *channel, RedClient *client, RedsStream *stream, int migration,
> > - int num_common_caps, uint32_t *common_caps, int num_caps,
> > - uint32_t *caps)
> > +static void inputs_link(Channel *channel, RedClient *client,
> > + RedsStream *stream, int migration,
> > + int num_common_caps, uint32_t *common_caps,
> > + int num_caps, uint32_t *caps)
> > {
> > - RedChannelClient *rcc;
> > + InputsChannelClient *icc;
> >
> > ASSERT(channel->data == g_inputs_channel);
> > -
> > if (channel->data == NULL) {
> > - red_printf("input channel create");
> > - g_inputs_channel = (InputsChannel*)red_channel_create_parser(
> > + red_printf("inputs channel create");
> > + channel->data = g_inputs_channel = (InputsChannel*)red_channel_create_parser(
> > sizeof(InputsChannel), core, migration, FALSE /* handle_acks */
> > ,inputs_channel_config_socket
> > ,inputs_channel_disconnect
> > @@ -540,25 +542,22 @@ static void inputs_link(Channel *channel, RedClient *client, RedsStream *stream,
> > ,NULL
> > ,NULL
> > ,NULL);
> > + ASSERT(channel->data);
>
> In general, are we committed to be OOM-safe? If not, then we should
> abort in our allocation functions, instead of having ASSERT after
> every caller.
I think we can do that. Maybe you can send a separate email about this with a subject that will
catch the eyes of more people?
>
> > }
> > - channel->data = g_inputs_channel;
> > - ASSERT(g_inputs_channel);
> > - red_printf("input channel client create");
> > - rcc = red_channel_client_create(sizeof(RedChannelClient), &g_inputs_channel->base,
> > - client, stream);
> > - ASSERT(rcc);
> > - inputs_pipe_add_init(rcc);
> > + red_printf("inputs channel client create");
> > + icc = (InputsChannelClient*)red_channel_client_create(sizeof(InputsChannelClient),
> > + channel->data, client, stream);
> > + icc->motion_count = 0;
> > + inputs_pipe_add_init(&icc->base);
> > }
> >
> > static void inputs_push_keyboard_modifiers(uint8_t modifiers)
> > {
> > - KeyModifiersPipeItem *item;
> > -
> > if (!g_inputs_channel || !red_channel_is_connected(&g_inputs_channel->base)) {
> > return;
> > }
> > - item = inputs_key_modifiers_item_new(g_inputs_channel, modifiers);
> > - red_channel_client_pipe_add_push(g_inputs_channel->base.rcc, &item->base);
> > + red_channel_pipes_new_add_push(&g_inputs_channel->base,
> > + inputs_key_modifiers_item_new, (void*)(uint64_t)modifiers);
> > }
> >
> > void inputs_on_keyboard_leds_change(void *opaque, uint8_t leds)
> > --
> > 1.7.4.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
>
>
> --
> Marc-André Lureau
More information about the Spice-devel
mailing list