>
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>
> Doing so allows us to remove the extra QXLInstance parameter from
> cursor_item_unref() and makes the code a bit cleaner.
>
> Also add cursor_item_ref().
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> server/cursor-channel.c | 70
> +++++++++++++++++++++++++++----------------------
> server/cursor-channel.h | 9 ++++---
> 2 files changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 6cc2b93..2d7afc6 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -25,55 +25,58 @@
> #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(cmd != NULL, NULL);
> - cursor_item = alloc_cursor_item();
> + spice_return_val_if_fail(item != NULL, NULL);
> + spice_return_val_if_fail(item->refs > 0, NULL);
This catch a memory corruption, should abort the program,
I suggest spice_error, spice_critical or spice_assert.
>
> - 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);
> +
> }
>
> static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
> {
> if (cursor->item)
> - cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> cursor->item);
> -
> - if (item)
> - item->refs++;
> + cursor_item_unref(cursor->item);
>
> - cursor->item = item;
> + cursor->item = item ? cursor_item_ref(item) : NULL;
> }
>
Are these tests worth doing or can we assume that a NULL pointer is expected (like
unref(NULL) and ref(NULL) do nothing) ? This would make cursor_set_item like
static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
{
cursor_item_unref(cursor->item);
cursor->item = cursor_item_ref(item);
}
On naming, sometimes is cursor_item sometimes item. I would prefer cursor_item.
> static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int
> num)
> @@ -155,7 +158,7 @@ static void put_cursor_pipe_item(CursorChannelClient
> *ccc, CursorPipeItem *pipe_
>
> spice_assert(!pipe_item_is_linked(&pipe_item->base));
>
> - cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> pipe_item->cursor_item);
> + cursor_item_unref(pipe_item->cursor_item);
> free(pipe_item);
> }
>
> @@ -411,7 +414,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
> CursorItem *cursor_item;
> int cursor_show = FALSE;
>
> - cursor_item = cursor_item_new(cursor_cmd, group_id);
> + spice_return_if_fail(cursor);
> + spice_return_if_fail(cursor_cmd);
> +
> + cursor_item = cursor_item_new(red_worker_get_qxl(cursor->common.worker),
> + cursor_cmd, group_id);
>
> switch (cursor_cmd->type) {
> case QXL_CURSOR_SET:
> @@ -439,7 +446,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
> red_channel_pipes_new_add(&cursor->common.base,
> new_cursor_pipe_item,
> (void*)cursor_item);
> }
> - cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> cursor_item);
> +
> + cursor_item_unref(cursor_item);
> }
>
> void cursor_channel_reset(CursorChannel *cursor)
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 2c19859..8b49df7 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -33,6 +33,7 @@
> #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
>
> typedef struct CursorItem {
> + QXLInstance *qxl;
> uint32_t group_id;
> int refs;
> RedCursorCmd *red_cursor;
> @@ -83,14 +84,14 @@ void cursor_channel_reset
> (CursorChannel *cursor);
> void cursor_channel_process_cmd (CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
> uint32_t group_id);
>
> -CursorItem* cursor_item_new (RedCursorCmd *cmd, uint32_t
> group_id);
> -void cursor_item_unref (QXLInstance *qxl,
> CursorItem *cursor);
> -
> -
> CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> RedClient *client, RedsStream
> *stream,
> int mig_target,
> uint32_t *common_caps, int
> num_common_caps,
> uint32_t *caps, int
> num_caps);
>
> +CursorItem* cursor_item_new (QXLInstance *qxl,
> RedCursorCmd *cmd, uint32_t group_id);
> +CursorItem* cursor_item_ref (CursorItem *cursor);
> +void cursor_item_unref (CursorItem *cursor);
> +
> #endif /* CURSOR_CHANNEL_H_ */
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel