>
> On 10/28/2015 06:07 PM, Frediano Ziglio wrote:
> >
> >>
> >> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel