On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>> > 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 eef0121..d145f86 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);
>> >
>> > - 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)
>>
>> Just a nitpick, I would prefer to have a explicit comparison here: if
>> (--items->refs > 0) ...
>>
>
> Why not
>
> if (--items->refs != 0) ??
>
> I mean, at the end the results should be the same if no errors managing
> the counters are present.
I just think the check for > 0 is the proper test we want to do,
mainly considering that refs is a int (and not a uint)
> Are you suggesting to change the test to avoid
> multiple free (hoping memory is still untouched after the first one) ?
I really didn't think about this.
>
> I start suspecting that sending the patch when there are pending issue to
> be discussed is not really a good idea.
>
>> > + 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;
>> > }
>> >
>> > static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data,
>> > int num)
>> > @@ -157,7 +160,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);
>> > }
>> >
>> > @@ -404,7 +407,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:
>> > @@ -434,7 +441,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
>> > RedCursorCmd *cursor_cmd,
>> > red_channel_pipes_new_add(&cursor->common.base,
>> > new_cursor_pipe_item, 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 293cfc1..f20001c 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(CursorChannel *cursor,
>> > 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_ */
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > Spice-devel mailing list
>> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> ACK!
>>
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel