>
> 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. Are you suggesting to change the test to avoid
multiple free (hoping memory is still untouched after the first one) ?
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