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

[Frediano Ziglio]

> 
> On 10/28/2015 06:07 PM, Frediano Ziglio wrote:
> >
> >>
> >> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >>
> >> Also fix warning due to unexpected pipe item type
> >>
> >> The specific item type that was not being handled was
> >> PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor
> >> channel, but the analogous item for the display channel is
> >> PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> >>
> >> The exact warning follows:
> >>
> >>      (/usr/bin/qemu-kvm:24458): Spice-Warning **:
> >>      ../../server/dcc-send.c:2442:dcc_send_item: should not be reached
> >>      (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
> >>      ../../server/dcc.c:1595:release_item_before_push: invalid item type
> >>
> >> Author:    Marc-André Lureau <marcandre.lureau at gmail.com>
> >> Date:      Thu Feb 27 19:38:58 2014 +0200
> >> ---
> >>   server/cache_item.tmpl.c |   4 +-
> >>   server/cursor-channel.c  | 219
> >>   ++++++++++++++++++++++++++---------------------
> >>   server/cursor-channel.h  |  27 +++---
> >>   server/red_dispatcher.c  |   1 +
> >>   server/red_worker.c      | 200
> >>   +++++++++++++++++++------------------------
> >>   server/red_worker.h      |  65 +++++---------
> >>   6 files changed, 255 insertions(+), 261 deletions(-)
> >>
> >> diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
> >> index dc314c0..ad2b579 100644
> >> --- a/server/cache_item.tmpl.c
> >> +++ b/server/cache_item.tmpl.c
> >> @@ -21,6 +21,7 @@
> >>   #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
> >>   #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
> >>   #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
> >> +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
> >
> > I would prefer a PIPE_ITEM_TYPE_INVALID macro here.
> > Is not clear which part of the patch is fixing the "unexpected pipe item
> > type".
> 
> I think both are ok.
>

Was mainly style, the macro is used just here.

> >
> >>   #define FUNC_NAME(name) red_cursor_cache_##name
> >>   #define VAR_NAME(name) cursor_cache_##name
> >>   #define CHANNEL CursorChannel
> >> @@ -32,6 +33,7 @@
> >>   #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
> >>   #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
> >>   #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
> >> +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
> 
> This line fixes the "unexpected pipe item type".
> 

Oh, fine. Why was never discovered before? Is it fixing something?

> >>   #define FUNC_NAME(name) red_palette_cache_##name
> >>   #define VAR_NAME(name) palette_cache_##name
> >>   #define CHANNEL DisplayChannel
> >> @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
> >> *channel_client, CacheItem *item)
> >>       channel_client->VAR_NAME(items)--;
> >>       channel_client->VAR_NAME(available) += item->size;
> >>
> >> -    red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data,
> >> PIPE_ITEM_TYPE_INVAL_ONE);
> >> +    red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data,
> >> PIPE_ITEM_TYPE);
> >>       red_channel_client_pipe_add_tail(&channel_client->common.base,
> >>       &item->u.pipe_data); // for now
> >>   }
> >>
> >> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> >> index 7ba4a7d..adf7d78 100644
> >> --- a/server/cursor-channel.c
> >> +++ b/server/cursor-channel.c
> >> @@ -25,57 +25,64 @@
> >>   #include "cache_item.tmpl.c"
> >>   #undef CLIENT_CURSOR_CACHE
> >>
> >> -static inline CursorItem *alloc_cursor_item(void)
> >> +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t
> >> group_id)
> >>   {
> >>       CursorItem *cursor_item;
> >>
> >> +    spice_return_val_if_fail(cmd != NULL, NULL);
> >> +
> >>       cursor_item = g_slice_new0(CursorItem);
> >> +    cursor_item->qxl = qxl;
> >>       cursor_item->refs = 1;
> >> +    cursor_item->group_id = group_id;
> >> +    cursor_item->red_cursor = cmd;
> >>
> >>       return cursor_item;
> >>   }
> >>
> >> -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> >> +CursorItem *cursor_item_ref(CursorItem *item)
> >>   {
> >> -    CursorItem *cursor_item;
> >> +    spice_return_val_if_fail(item != NULL, NULL);
> >> +    spice_return_val_if_fail(item->refs > 0, NULL);
> >
> > This is detecting a memory error, perhaps a spice_error or a spice_critical
> > should be better.
> >
> >>
> >> -    spice_return_val_if_fail(cmd != NULL, NULL);
> >> -    spice_warn_if(!(cursor_item = alloc_cursor_item()));
> >> -
> >> -    cursor_item->group_id = group_id;
> >> -    cursor_item->red_cursor = cmd;
> >> +    item->refs++;
> >>
> >> -    return cursor_item;
> >> +    return item;
> >>   }
> >>
> >> -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> >> +void cursor_item_unref(CursorItem *item)
> >>   {
> >> -    if (!--cursor->refs) {
> >> -        QXLReleaseInfoExt release_info_ext;
> >> -        RedCursorCmd *cursor_cmd;
> >> -
> >> -        cursor_cmd = cursor->red_cursor;
> >> -        release_info_ext.group_id = cursor->group_id;
> >> -        release_info_ext.info = cursor_cmd->release_info;
> >> -        qxl->st->qif->release_resource(qxl, release_info_ext);
> >> -        red_put_cursor_cmd(cursor_cmd);
> >> -        free(cursor_cmd);
> >> -
> >> -        g_slice_free(CursorItem, cursor);
> >> -    }
> >> +    QXLReleaseInfoExt release_info_ext;
> >> +    RedCursorCmd *cursor_cmd;
> >> +
> >> +    spice_return_if_fail(item != NULL);
> >> +
> >> +    if (--item->refs)
> >> +        return;
> >> +
> >> +    cursor_cmd = item->red_cursor;
> >> +    release_info_ext.group_id = item->group_id;
> >> +    release_info_ext.info = cursor_cmd->release_info;
> >> +    item->qxl->st->qif->release_resource(item->qxl, release_info_ext);
> >> +    red_put_cursor_cmd(cursor_cmd);
> >> +    free(cursor_cmd);
> >> +
> >> +    g_slice_free(CursorItem, item);
> >> +
> >>   }
> >>
> >
> > There are a lot of renames cursor_channel -> cursor and cursor -> item
> > (here
> > and in following hunks). Should we remove them ?
> > Which naming should we use ?
> 
> I like cursor_channel better than cursor (or maybe even just channel).
> I like cursor_item    better than cursor (or maybe just item).
> 
> 
> I like a patch that says "move some code" to only
> move code and not change code that is moved.
> I think this patch should be split.
> 
> - Uri.
> 

This was done (see previous patch already pushed). The only think left is to change
the commit message.
I personally vote for cursor_channel too instead of just cursor (I think was like this
before this patch). About cursors I would prefer cursor_item.

Frediano