[Spice-devel] [PATCH 12/18] server/red_worker: split cursor_channel_send_item

[Alon Levy]

On Tue, Feb 08, 2011 at 10:00:43PM +0100, Marc-André Lureau wrote:
> Even if the function name is explicit, I would rather see:
> 
> cursor_channel_send_item(CursorChannel *cursor_channel, ...) {
>    RedChannel *channel  =  (CursorChannel *)cursor_channel;
You meant (RedChannel *)cursor_channel? so that's what CONTAINEROF is
exactly for. CONTAINEROF(cursor_channel, RedChannel, base)

CONTAINEROF takes some getting used to, it's (pointer, type, field),
once you get the hang of it the type checking is very convenient.

> }
> 
> to avoid downcasting, perhaps it's matter of taste, but I feel better
> with upcasts.
Yes, I don't think it's taste only, it makes sense. I basically try
to keep the signatures as explicit as possible (no casts if you can
avoid them), the exceptions are when you have an interface (like RedChannel).

> 
> On Mon, Feb 7, 2011 at 7:20 PM, Alon Levy <alevy at redhat.com> wrote:
> > Split from cursor_channel_push
> > ---
> >  server/red_worker.c |   87 ++++++++++++++++++++++++++------------------------
> >  1 files changed, 45 insertions(+), 42 deletions(-)
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index e8849a7..1574b99 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -8407,53 +8407,56 @@ static void display_channel_push(RedWorker *worker)
> >     }
> >  }
> >
> > +static void cursor_channel_send_item(RedChannel *channel, PipeItem *pipe_item)
> > +{
> > +    CursorChannel *cursor_channel = (CursorChannel *)red_ref_channel(channel);
> > +    red_channel_reset_send_data(channel);
> > +    switch (pipe_item->type) {
> > +    case PIPE_ITEM_TYPE_CURSOR:
> > +        red_send_cursor(cursor_channel, (CursorItem *)pipe_item);
> > +        break;
> > +    case PIPE_ITEM_TYPE_LOCAL_CURSOR:
> > +        red_send_local_cursor(cursor_channel, (LocalCursor *)pipe_item);
> > +        break;
> > +    case PIPE_ITEM_TYPE_INVAL_ONE:
> > +        red_cursor_send_inval(cursor_channel, (CacheItem *)pipe_item);
> > +        free(pipe_item);
> > +        break;
> > +    case PIPE_ITEM_TYPE_VERB:
> > +        red_send_verb(channel, ((VerbItem*)pipe_item)->verb);
> > +        free(pipe_item);
> > +        break;
> > +    case PIPE_ITEM_TYPE_MIGRATE:
> > +        red_printf("PIPE_ITEM_TYPE_MIGRATE");
> > +        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);
> > +        free(pipe_item);
> > +        break;
> > +    case PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE:
> > +        red_reset_cursor_cache(cursor_channel);
> > +        red_send_verb(channel, SPICE_MSG_CURSOR_INVAL_ALL);
> > +        free(pipe_item);
> > +        break;
> > +    default:
> > +        red_error("invalid pipe item type");
> > +    }
> > +    red_unref_channel(channel);
> > +}
> > +
> >  static void cursor_channel_push(RedWorker *worker)
> >  {
> >     PipeItem *pipe_item;
> >
> >     while ((pipe_item = red_pipe_get((RedChannel *)worker->cursor_channel))) {
> > -        CursorChannel *cursor_channel;
> > -
> > -        cursor_channel = (CursorChannel *)red_ref_channel((RedChannel *)worker->cursor_channel);
> > -        red_channel_reset_send_data((RedChannel*)cursor_channel);
> > -        switch (pipe_item->type) {
> > -        case PIPE_ITEM_TYPE_CURSOR:
> > -            red_send_cursor(cursor_channel, (CursorItem *)pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_LOCAL_CURSOR:
> > -            red_send_local_cursor(cursor_channel, (LocalCursor *)pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_INVAL_ONE:
> > -            red_cursor_send_inval(cursor_channel, (CacheItem *)pipe_item);
> > -            free(pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_VERB:
> > -            red_send_verb((RedChannel *)cursor_channel, ((VerbItem*)pipe_item)->verb);
> > -            free(pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_MIGRATE:
> > -            red_printf("PIPE_ITEM_TYPE_MIGRATE");
> > -            cursor_channel_send_migrate(cursor_channel);
> > -            free(pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_SET_ACK:
> > -            red_send_set_ack((RedChannel *)cursor_channel);
> > -            free(pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_CURSOR_INIT:
> > -            red_reset_cursor_cache(cursor_channel);
> > -            red_send_cursor_init(cursor_channel);
> > -            free(pipe_item);
> > -            break;
> > -        case PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE:
> > -            red_reset_cursor_cache(cursor_channel);
> > -            red_send_verb((RedChannel *)cursor_channel, SPICE_MSG_CURSOR_INVAL_ALL);
> > -            free(pipe_item);
> > -            break;
> > -        default:
> > -            red_error("invalid pipe item type");
> > -        }
> > -        red_unref_channel((RedChannel *)cursor_channel);
> > +        cursor_channel_send_item((RedChannel *)worker->cursor_channel, pipe_item);
> >     }
> >  }
> >
> > --
> > 1.7.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau