[Spice-devel] [PATCH 05/11] worker: move some cursor code to cursor-channel.c

[Jonathon Jongsma]

ACK. Two minor comments below.


On Tue, 2015-10-27 at 19:19 +0000, Frediano Ziglio wrote:
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/Makefile.am      |   1 +
>  server/cursor-channel.c | 464 ++++++++++++++++++++++++++++++++++++
>  server/cursor-channel.h |  26 +++
>  server/red_worker.c     | 611 +++++---------------------------------
> ----------
>  server/red_worker.h     |  92 ++++++++
>  5 files changed, 635 insertions(+), 559 deletions(-)
>  create mode 100644 server/cursor-channel.c
> 

....

> +// TODO: share code between before/after_push since most of the
> items need the same
> +// release
> +static void
> cursor_channel_client_release_item_before_push(CursorChannelClient
> *ccc,
> +                                                           PipeItem
> *item)
> +{
> +    switch (item->type) {
> +    case PIPE_ITEM_TYPE_CURSOR: {
> +        CursorPipeItem *cursor_pipe_item = SPICE_CONTAINEROF(item,
> CursorPipeItem, base);
> +        put_cursor_pipe_item(ccc, cursor_pipe_item);
> +        break;
> +    }
> +    case PIPE_ITEM_TYPE_INVAL_ONE:
> +    case PIPE_ITEM_TYPE_VERB:
> +    case PIPE_ITEM_TYPE_CURSOR_INIT:
> +    case PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE:
> +        free(item);
> +        break;
> +    default:
> +        spice_error("invalid pipe item type");
> +    }
> +}
> +
> +static void
> cursor_channel_client_release_item_after_push(CursorChannelClient
> *ccc,
> +                                                          PipeItem
> *item)
> +{
> +    switch (item->type) {
> +        case PIPE_ITEM_TYPE_CURSOR: {
> +            CursorPipeItem *cursor_pipe_item =
> SPICE_CONTAINEROF(item, CursorPipeItem, base);
> +            put_cursor_pipe_item(ccc, cursor_pipe_item);
> +            break;
> +        }
> +        default:
> +            spice_critical("invalid item type");


This is an existing inconsistency, but here we use spice_critical(),
and just above in _before_push(), we use spice_error() for the same
situation. Would probably be good to choose one and be consistent, even
if they do behave the same with default settings...


>  
> +static inline void red_marshall_inval(RedChannelClient *rcc,
> +                                      SpiceMarshaller
> *base_marshaller, CacheItem *cach_item)
> +{
> +    SpiceMsgDisplayInvalOne inval_one;
> +
> +    red_channel_client_init_send_data(rcc, cach_item->inval_type,
> NULL);
> +    inval_one.id = *(uint64_t *)&cach_item->id;
> +
> +    spice_marshall_msg_cursor_inval_one(base_marshaller,
> &inval_one);
> +}
> +


Up above, this function was removed from red_worker.c and was added to
cursor-channel.c. Here it is being added back to red_worker.c again.
Probably this one can be removed?