[Spice-devel] [PATCH spice-server v2 6/6] cursor-channel: Use a single RedCursorPipeItem to hold the cursor

[Christophe Fergeau]

On Mon, Sep 04, 2017 at 12:02:10PM +0100, Frediano Ziglio wrote:
> RedPipeItem already implements reference counting so
> this avoid duplicating code to handle a object with reference
> counting that points to another object with reference counting
> that holds a RedCursorCmd.

Acked-by: Christophe Fergeau <cfergeau at redhat.com>
Looks good to me apart from one minor comment below


> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/cursor-channel.c | 107 +++++++++++++++---------------------------------
>  1 file changed, 33 insertions(+), 74 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 2441e1ed..2efa6ef4 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -28,17 +28,17 @@
>  #include "reds.h"
>  #include "red-qxl.h"
>  
> -typedef struct CursorItem {
> +typedef struct RedCursorPipeItem {
> +    RedPipeItem base;
>      QXLInstance *qxl;
> -    int refs;
>      RedCursorCmd *red_cursor;
> -} CursorItem;
> +} RedCursorPipeItem;
>  
>  struct CursorChannel
>  {
>      CommonGraphicsChannel parent;
>  
> -    CursorItem *item;
> +    RedCursorPipeItem *item;
>      bool cursor_visible;
>      SpicePoint16 cursor_position;
>      uint16_t cursor_trail_length;
> @@ -51,83 +51,52 @@ struct CursorChannelClass
>      CommonGraphicsChannelClass parent_class;
>  };
>  
> -typedef struct RedCursorPipeItem {
> -    RedPipeItem base;
> -    CursorItem *cursor_item;
> -} RedCursorPipeItem;
> -
>  G_DEFINE_TYPE(CursorChannel, cursor_channel, TYPE_COMMON_GRAPHICS_CHANNEL)
>  
>  static void cursor_pipe_item_free(RedPipeItem *pipe_item);
>  
> -static CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd)
> +static RedCursorPipeItem *cursor_pipe_item_new(QXLInstance *qxl, RedCursorCmd *cmd)
>  {
> -    CursorItem *cursor_item;
> +    RedCursorPipeItem *item = spice_malloc0(sizeof(RedCursorPipeItem));
>  
>      spice_return_val_if_fail(cmd != NULL, NULL);
>  
> -    cursor_item = g_new0(CursorItem, 1);
> -    cursor_item->qxl = qxl;
> -    cursor_item->refs = 1;
> -    cursor_item->red_cursor = cmd;
> +    red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_CURSOR,
> +                            cursor_pipe_item_free);
> +    item->qxl = qxl;
> +    item->red_cursor = cmd;
>  
> -    return cursor_item;
> +    return item;
>  }
>  
> -static CursorItem *cursor_item_ref(CursorItem *item)
> +static RedCursorPipeItem *cursor_pipe_item_ref(RedCursorPipeItem *item)
>  {
> -    spice_return_val_if_fail(item != NULL, NULL);
> -    spice_return_val_if_fail(item->refs > 0, NULL);
> -
> -    item->refs++;
> -
> +    red_pipe_item_ref(&item->base);
>      return item;
>  }

I would not keep this cursor_pipe_item_ref() wrapper

Christophe