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

[Uri Lublin]

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.

>
>>   #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".

>>   #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.