[Spice-devel] [PATCH 21/23] server: move SET_ACK to red_channel
Alon Levy
alevy at redhat.com
Sat Feb 19 10:31:07 PST 2011
On Sat, Feb 12, 2011 at 08:55:24PM +0100, Marc-André Lureau wrote:
> On Fri, Feb 11, 2011 at 6:23 PM, Alon Levy <alevy at redhat.com> wrote:
> > ---
> > server/red_channel.c | 27 +++++++++++++++++++++++++++
> > server/red_channel.h | 11 +++++++++++
> > server/red_tunnel_worker.c | 22 ++--------------------
> > server/red_worker.c | 35 +++--------------------------------
> > 4 files changed, 43 insertions(+), 52 deletions(-)
> >
> > diff --git a/server/red_channel.c b/server/red_channel.c
> > index be1d9f9..dfba8b4 100644
> > --- a/server/red_channel.c
> > +++ b/server/red_channel.c
> > @@ -28,6 +28,7 @@
> > #include <errno.h>
> > #include "stat.h"
> > #include "red_channel.h"
> > +#include "generated_marshallers.h"
> >
> > static PipeItem *red_channel_pipe_get(RedChannel *channel);
> > static void red_channel_event(int fd, int event, void *data);
> > @@ -249,10 +250,34 @@ static void red_channel_reset_send_data(RedChannel *channel)
> > channel->send_data.header->serial = ++channel->send_data.serial;
> > }
> >
> > +void red_channel_push_set_ack(RedChannel *channel)
> > +{
> > + red_channel_pipe_add_type(channel, PIPE_ITEM_TYPE_SET_ACK);
> > +}
> > +
>
> I don't really see the need for a function, otherwise, we could have a
> function for all the other types.
>
I generally find it easier to have these convenience functions, see main channel.
Less error prone maybe?
> > +static void red_channel_send_set_ack(RedChannel *channel)
> > +{
> > + SpiceMsgSetAck ack;
> > +
> > + ASSERT(channel);
> > + red_channel_init_send_data(channel, SPICE_MSG_SET_ACK, NULL);
> > + ack.generation = ++channel->ack_data.generation;
> > + ack.window = channel->ack_data.client_window;
> > + channel->ack_data.messages_window = 0;
> > +
> > + spice_marshall_msg_set_ack(channel->send_data.marshaller, &ack);
> > +
> > + red_channel_begin_send_message(channel);
> > +}
> > +
> > static void red_channel_send_item(RedChannel *channel, PipeItem *item)
> > {
> > red_channel_reset_send_data(channel);
> > switch (item->type) {
> > + case PIPE_ITEM_TYPE_SET_ACK:
> > + red_channel_send_set_ack(channel);
> > + free(item);
>
> Why not call free() in release_item() instead?
>
> > + return;
> > }
> > /* only reached if not handled here */
> > channel->send_item(channel, item);
> > @@ -261,6 +286,8 @@ static void red_channel_send_item(RedChannel *channel, PipeItem *item)
> > static void red_channel_release_item(RedChannel *channel, PipeItem *item, int item_pushed)
> > {
> > switch (item->type) {
> > + case PIPE_ITEM_TYPE_SET_ACK:
> > + return;
>
> here?
>
> > }
> > /* only reached if not handled here */
> > channel->release_item(channel, item, item_pushed);
> > diff --git a/server/red_channel.h b/server/red_channel.h
> > index c662186..8367ff7 100644
> > --- a/server/red_channel.h
> > +++ b/server/red_channel.h
> > @@ -89,6 +89,16 @@ typedef struct BufDescriptor {
> > uint8_t *data;
> > } BufDescriptor;
> >
> > +/* Messages handled by red_channel
> > + * SET_ACK - sent to client on channel connection
> > + * Note that the numbers don't have to correspond to spice message types,
> > + * but we keep the 100 first allocated for base channel approach.
> > + * */
> > +enum {
> > + PIPE_ITEM_TYPE_SET_ACK=1,
> > + PIPE_ITEM_TYPE_CHANNEL_BASE=101,
> > +};
> > +
> > typedef struct PipeItem {
> > RingItem link;
> > int type;
> > @@ -226,6 +236,7 @@ void red_channel_pipe_add_type(RedChannel *channel, int pipe_item_type);
> >
> > void red_channel_ack_zero_messages_window(RedChannel *channel);
> > void red_channel_ack_set_client_window(RedChannel *channel, int client_window);
> > +void red_channel_push_set_ack(RedChannel *channel);
> >
> > // TODO: unstaticed for display/cursor channels. they do some specific pushes not through
> > // adding elements or on events. but not sure if this is actually required (only result
> > diff --git a/server/red_tunnel_worker.c b/server/red_tunnel_worker.c
> > index fdbe46e..d481a38 100644
> > --- a/server/red_tunnel_worker.c
> > +++ b/server/red_tunnel_worker.c
> > @@ -67,8 +67,7 @@
> > typedef struct TunnelWorker TunnelWorker;
> >
> > enum {
> > - PIPE_ITEM_TYPE_SET_ACK,
> > - PIPE_ITEM_TYPE_MIGRATE,
> > + PIPE_ITEM_TYPE_MIGRATE = PIPE_ITEM_TYPE_CHANNEL_BASE,
> > PIPE_ITEM_TYPE_MIGRATE_DATA,
> > PIPE_ITEM_TYPE_TUNNEL_INIT,
> > PIPE_ITEM_TYPE_SERVICE_IP_MAP,
> > @@ -2334,19 +2333,6 @@ static int tunnel_channel_handle_message(RedChannel *channel, SpiceDataHeader *h
> > /* outgoing msgs
> > ********************************/
> >
> > -static void tunnel_channel_send_set_ack(TunnelChannel *channel, PipeItem *item)
> > -{
> > - ASSERT(channel);
> > -
> > - channel->base.send_data.u.ack.generation = ++channel->base.ack_data.generation;
> > - channel->base.send_data.u.ack.window = CLIENT_ACK_WINDOW;
> > -
> > - red_channel_init_send_data(&channel->base, SPICE_MSG_SET_ACK, item);
> > - red_channel_add_buf(&channel->base, &channel->base.send_data.u.ack, sizeof(SpiceMsgSetAck));
> > -
> > - red_channel_begin_send_message(&channel->base);
> > -}
> > -
> > static void tunnel_channel_send_migrate(TunnelChannel *channel, PipeItem *item)
> > {
> > ASSERT(channel);
> > @@ -2813,9 +2799,6 @@ static void tunnel_channel_send_item(RedChannel *channel, PipeItem *item)
> > TunnelChannel *tunnel_channel = (TunnelChannel *)channel;
> >
> > switch (item->type) {
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > - tunnel_channel_send_set_ack(tunnel_channel, item);
> > - break;
> > case PIPE_ITEM_TYPE_TUNNEL_INIT:
> > tunnel_channel_send_init(tunnel_channel, item);
> > break;
> > @@ -2860,7 +2843,6 @@ static void tunnel_channel_release_pipe_item(RedChannel *channel, PipeItem *item
> > return;
> > }
> > switch (item->type) {
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > case PIPE_ITEM_TYPE_TUNNEL_INIT:
> > free(item);
> > break;
> > @@ -3409,7 +3391,7 @@ static void tunnel_channel_disconnect(RedChannel *channel)
> >
> > static void on_new_tunnel_channel(TunnelChannel *channel)
> > {
> > - red_channel_pipe_add_type(&channel->base, PIPE_ITEM_TYPE_SET_ACK);
> > + red_channel_push_set_ack(&channel->base);
> >
> > if (channel->base.migrate) {
> > channel->expect_migrate_data = TRUE;
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 473473c..1659af0 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -231,12 +231,11 @@ enum {
> > };
> >
> > enum {
> > - PIPE_ITEM_TYPE_DRAW,
> > + PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE,
> > PIPE_ITEM_TYPE_INVAL_ONE,
> > PIPE_ITEM_TYPE_CURSOR,
> > PIPE_ITEM_TYPE_MIGRATE,
> > PIPE_ITEM_TYPE_LOCAL_CURSOR,
> > - PIPE_ITEM_TYPE_SET_ACK,
> > PIPE_ITEM_TYPE_CURSOR_INIT,
> > PIPE_ITEM_TYPE_IMAGE,
> > PIPE_ITEM_TYPE_STREAM_CREATE,
> > @@ -7548,21 +7547,6 @@ static inline void send_qxl_drawable(DisplayChannel *display_channel, Drawable *
> > red_lossy_send_qxl_drawable(display_channel->common.worker, display_channel, item);
> > }
> >
> > -static void red_send_set_ack(RedChannel *channel)
> > -{
> > - SpiceMsgSetAck ack;
> > -
> > - ASSERT(channel);
> > - red_channel_init_send_data(channel, SPICE_MSG_SET_ACK, NULL);
> > - ack.generation = ++channel->ack_data.generation;
> > - ack.window = channel->ack_data.client_window;
> > - channel->ack_data.messages_window = 0;
> > -
> > - spice_marshall_msg_set_ack(channel->send_data.marshaller, &ack);
> > -
> > - red_channel_begin_send_message(channel);
> > -}
> > -
> > static inline void red_send_verb(RedChannel *channel, uint16_t verb)
> > {
> > ASSERT(channel);
> > @@ -8130,10 +8114,6 @@ static void display_channel_send_item(RedChannel *base, PipeItem *pipe_item)
> > display_channel_send_migrate_data(display_channel);
> > free(pipe_item);
> > break;
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > - red_send_set_ack((RedChannel *)display_channel);
> > - free(pipe_item);
> > - break;
> > case PIPE_ITEM_TYPE_IMAGE:
> > red_send_image(display_channel, (ImageItem *)pipe_item);
> > release_image_item((ImageItem *)pipe_item);
> > @@ -8196,10 +8176,6 @@ static void cursor_channel_send_item(RedChannel *channel, PipeItem *pipe_item)
> > cursor_channel_send_migrate(cursor_channel);
> > free(pipe_item);
> > break;
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > - red_send_set_ack(channel);
> > - free(pipe_item);
> > - break;
> > case PIPE_ITEM_TYPE_CURSOR_INIT:
> > red_reset_cursor_cache(cursor_channel);
> > red_send_cursor_init(cursor_channel);
> > @@ -8674,7 +8650,7 @@ static void on_new_display_channel(RedWorker *worker)
> > DisplayChannel *display_channel = worker->display_channel;
> > ASSERT(display_channel);
> >
> > - red_channel_pipe_add_type(&display_channel->common.base, PIPE_ITEM_TYPE_SET_ACK);
> > + red_channel_push_set_ack(&display_channel->common.base);
> >
> > if (display_channel->common.base.migrate) {
> > display_channel->expect_migrate_data = TRUE;
> > @@ -9160,8 +9136,6 @@ static void display_channel_release_item(RedChannel *channel, PipeItem *item, in
> > case PIPE_ITEM_TYPE_IMAGE:
> > release_image_item((ImageItem *)item);
> > break;
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > - break;
> > default:
> > PANIC("invalid item type");
> > }
> > @@ -9274,7 +9248,7 @@ static void on_new_cursor_channel(RedWorker *worker)
> >
> > ASSERT(channel);
> > red_channel_ack_zero_messages_window(&channel->common.base);
> > - red_channel_pipe_add_type(&channel->common.base, PIPE_ITEM_TYPE_SET_ACK);
> > + red_channel_push_set_ack(&channel->common.base);
> > if (worker->surfaces[0].context.canvas && !channel->common.base.migrate) {
> > red_channel_pipe_add_type(&worker->cursor_channel->common.base, PIPE_ITEM_TYPE_CURSOR_INIT);
> > }
> > @@ -9295,9 +9269,6 @@ static void cursor_channel_release_item(RedChannel *channel, PipeItem *item, int
> > case PIPE_ITEM_TYPE_CURSOR:
> > red_release_cursor(common->worker, SPICE_CONTAINEROF(item, CursorItem, pipe_data));
> > break;
> > - case PIPE_ITEM_TYPE_SET_ACK:
> > - free(item);
> > - break;
> > default:
> > PANIC("invalid item type");
> > }
> > --
> > 1.7.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