[Spice-devel,02/10] Store QXLInstance in CursorItem

Submitted by Frediano Ziglio on Nov. 2, 2015, 9:55 a.m.

Details

Message ID 1446458166-31403-3-git-send-email-fziglio@redhat.com
State New
Headers show
Series "Backported some patches from refactory branches (2nd Nov)" ( rev: 4 3 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 2, 2015, 9:55 a.m.
From: Marc-André Lureau <marcandre.lureau@gmail.com>

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@redhat.com>
---
 server/cursor-channel.c | 70 +++++++++++++++++++++++++++----------------------
 server/cursor-channel.h |  9 ++++---
 2 files changed, 44 insertions(+), 35 deletions(-)

Patch hide | download patch | download mbox

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)
+        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_ */

Comments

On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
>
> 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@redhat.com>
> ---
>  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) ...

> +        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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

ACK!
> 
> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau@gmail.com>
> >
> > 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@redhat.com>
> > ---
> >  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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ACK!
> 

Frediano
On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>
>> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> > From: Marc-André Lureau <marcandre.lureau@gmail.com>
>> >
>> > 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@redhat.com>
>> > ---
>> >  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@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> ACK!
>>
>
> Frediano
On Mon, 2015-11-02 at 11:00 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > fziglio@redhat.com> wrote:
> > > From: Marc-André Lureau <marcandre.lureau@gmail.com>
> > > 
> > > 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@redhat.com>
> > > ---
> > >  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.


Hmm, what do you mean by this? What pending issues need to be
discussed?



> 
> > > +        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@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > ACK!
> > 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> On Mon, 2015-11-02 at 11:00 -0500, Frediano Ziglio wrote:
> > > 
> > > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > > fziglio@redhat.com> wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@gmail.com>
> > > > 
> > > > 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@redhat.com>
> > > > ---
> > > >  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.
> 
> 
> Hmm, what do you mean by this? What pending issues need to be
> discussed?
> 

About the lines (spice_warning(refs > 0)) that I proposed to change
in spice_error/spice_critical. I suggested to change, you said you
prefer the warning. Nobody raise to change the "poll" (1-1).

Frediano

> 
> 
> > 
> > > > +        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@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > ACK!
> > >
Merged

Frediano

> > 
> > On Mon, 2015-11-02 at 11:00 -0500, Frediano Ziglio wrote:
> > > > 
> > > > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > > > fziglio@redhat.com> wrote:
> > > > > From: Marc-André Lureau <marcandre.lureau@gmail.com>
> > > > > 
> > > > > 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@redhat.com>
> > > > > ---
> > > > >  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.
> > 
> > 
> > Hmm, what do you mean by this? What pending issues need to be
> > discussed?
> > 
> 
> About the lines (spice_warning(refs > 0)) that I proposed to change
> in spice_error/spice_critical. I suggested to change, you said you
> prefer the warning. Nobody raise to change the "poll" (1-1).
> 
> Frediano
> 
> > 
> > 
> > > 
> > > > > +        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@lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > 
> > > > ACK!
> > > > 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Tue, Nov 3, 2015 at 11:01 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> Merged

And it got merged without both mine and yours suggestion :-\

Best Regards,
--
Fabiano Fidêncio
> 
> On Tue, Nov 3, 2015 at 11:01 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> > Merged
> 
> And it got merged without both mine and yours suggestion :-\
> 
> Best Regards,
> --
> Fabiano Fidêncio
> 

Mea culpa... yes.

I decided to post a mail on logging and style and not stop the merging.
Could you post a change for your suggestion?

Frediano
On Tue, Nov 3, 2015 at 1:38 PM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>
>> On Tue, Nov 3, 2015 at 11:01 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> > Merged
>>
>> And it got merged without both mine and yours suggestion :-\
>>
>> Best Regards,
>> --
>> Fabiano Fidêncio
>>
>
> Mea culpa... yes.
>
> I decided to post a mail on logging and style and not stop the merging.
> Could you post a change for your suggestion?

Once it's already merged I prefer to leave it as it is for now.
I would go for another series doing this kind of changes and it can be
done later.

>
> Frediano
On Tue, Nov 03, 2015 at 01:31:33PM +0100, Fabiano Fidêncio wrote:
> On Tue, Nov 3, 2015 at 11:01 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> > Merged
> 
> And it got merged without both mine and yours suggestion :-\

Yes, would have been nice to address the pending issues before
pushing...

Christophe
On Mon, Nov 02, 2015 at 05:11:04PM +0100, Fabiano Fidêncio wrote:
> On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio <fziglio@redhat.com> wrote:
> >>
> >> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> >> 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)
> 

For what it's worth, we currently have all variants, with the 'no
explicit test' variant being much more common:

$ git grep -- "--.*refs"
server/char_device.c:    if (--buf->refs == 0) {
server/char_device.c:        --buf->refs;
server/char_device.c:    if (!--char_dev->refs) {
server/cursor-channel.c:    if (--item->refs)
server/cursor-channel.c:    if (--pipe_item->refs) {
server/pixmap-cache.c:    if (--cache->refs) {
server/red_channel.c:    if (!--channel->refs) {
server/red_channel.c:    if (!--rcc->refs) {
server/red_channel.c:    if (!--client->refs) {
server/red_worker.c:    if (--monitors_config->refs > 0) {
server/red_worker.c:    if (--dpi->refs) {
server/red_worker.c:    if (!--item->refs) {
server/red_worker.c:    if (!--item->refs) {
server/red_worker.c:    if (!--surface->refs) {
server/red_worker.c:    if (--red_drawable->refs) {
server/red_worker.c:    if (!--drawable->refs) {
server/red_worker.c:    if (!--stream->refs) {
server/red_worker.c:    if (!--item->refs) {
server/red_worker.c:    if (--shared_dict->refs) {
server/reds.c:    if (!--buf->refs) {
server/smartcard.c:    if (!--item->refs) {
server/snd_worker.c:    if (!--channel->refs) {
server/spicevmc.c:    if (!--item->refs)

I personally prefer explicit tests. However, hopefully we'll manage to
get rid of much of this manual refcounting by the end of all this work,
so not really important.

Christophe
On Tue, Nov 3, 2015 at 6:00 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
> On Mon, Nov 02, 2015 at 05:11:04PM +0100, Fabiano Fidêncio wrote:
>> On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> >>
>> >> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> >> 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)
>>
>
> For what it's worth, we currently have all variants, with the 'no
> explicit test' variant being much more common:
>
> $ git grep -- "--.*refs"
> server/char_device.c:    if (--buf->refs == 0) {
> server/char_device.c:        --buf->refs;
> server/char_device.c:    if (!--char_dev->refs) {
> server/cursor-channel.c:    if (--item->refs)
> server/cursor-channel.c:    if (--pipe_item->refs) {
> server/pixmap-cache.c:    if (--cache->refs) {
> server/red_channel.c:    if (!--channel->refs) {
> server/red_channel.c:    if (!--rcc->refs) {
> server/red_channel.c:    if (!--client->refs) {
> server/red_worker.c:    if (--monitors_config->refs > 0) {
> server/red_worker.c:    if (--dpi->refs) {
> server/red_worker.c:    if (!--item->refs) {
> server/red_worker.c:    if (!--item->refs) {
> server/red_worker.c:    if (!--surface->refs) {
> server/red_worker.c:    if (--red_drawable->refs) {
> server/red_worker.c:    if (!--drawable->refs) {
> server/red_worker.c:    if (!--stream->refs) {
> server/red_worker.c:    if (!--item->refs) {
> server/red_worker.c:    if (--shared_dict->refs) {
> server/reds.c:    if (!--buf->refs) {
> server/smartcard.c:    if (!--item->refs) {
> server/snd_worker.c:    if (!--channel->refs) {
> server/spicevmc.c:    if (!--item->refs)
>
> I personally prefer explicit tests.

I also prefer explicit tests for everything that is not boolean.
It's way easier to, on a quick look, understand what is that var that
you never ever saw before ...

> However, hopefully we'll manage to
> get rid of much of this manual refcounting by the end of all this work,
> so not really important.

Yeah, I agree!

>
> Christophe
> 
> On Tue, Nov 3, 2015 at 6:00 PM, Christophe Fergeau <cfergeau@redhat.com>
> wrote:
> > On Mon, Nov 02, 2015 at 05:11:04PM +0100, Fabiano Fidêncio wrote:
> >> On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio <fziglio@redhat.com>
> >> wrote:
> >> >>
> >> >> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <fziglio@redhat.com>
> >> >> wrote:
> >> >> 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)
> >>
> >
> > For what it's worth, we currently have all variants, with the 'no
> > explicit test' variant being much more common:
> >
> > $ git grep -- "--.*refs"
> > server/char_device.c:    if (--buf->refs == 0) {
> > server/char_device.c:        --buf->refs;
> > server/char_device.c:    if (!--char_dev->refs) {
> > server/cursor-channel.c:    if (--item->refs)
> > server/cursor-channel.c:    if (--pipe_item->refs) {
> > server/pixmap-cache.c:    if (--cache->refs) {
> > server/red_channel.c:    if (!--channel->refs) {
> > server/red_channel.c:    if (!--rcc->refs) {
> > server/red_channel.c:    if (!--client->refs) {
> > server/red_worker.c:    if (--monitors_config->refs > 0) {
> > server/red_worker.c:    if (--dpi->refs) {
> > server/red_worker.c:    if (!--item->refs) {
> > server/red_worker.c:    if (!--item->refs) {
> > server/red_worker.c:    if (!--surface->refs) {
> > server/red_worker.c:    if (--red_drawable->refs) {
> > server/red_worker.c:    if (!--drawable->refs) {
> > server/red_worker.c:    if (!--stream->refs) {
> > server/red_worker.c:    if (!--item->refs) {
> > server/red_worker.c:    if (--shared_dict->refs) {
> > server/reds.c:    if (!--buf->refs) {
> > server/smartcard.c:    if (!--item->refs) {
> > server/snd_worker.c:    if (!--channel->refs) {
> > server/spicevmc.c:    if (!--item->refs)
> >
> > I personally prefer explicit tests.
> 
> I also prefer explicit tests for everything that is not boolean.
> It's way easier to, on a quick look, understand what is that var that
> you never ever saw before ...
> 

All new reference checks follow this style.

Frediano

> > However, hopefully we'll manage to
> > get rid of much of this manual refcounting by the end of all this work,
> > so not really important.
> 
> Yeah, I agree!
> 
> >
> > Christophe
>