[Spice-devel] [PATCH v2 3/9] Refactor cursor marshalling for SET, INIT

[Frediano Ziglio]

> Use spice_marshaller_add_by_ref_full() instead of
> spice_marshaller_add_by_ref() to allow the marshaller to manage the
> lifetime of the referenced data buffer rather than having to manage it
> by passing a PipeItem to red_channel_client_init_send_data(). Since the
> data is owned by CursorItem (which is not in fact a RedPipeItem, but is
> owned by a pipe item and is itself refcounted), we take a reference on
> the CursorItem when adding the data buf to the marshaller, and then
> unreference it in the marshaller free func.
> ---
>  server/cursor-channel.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 04483af..8befa6e 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -126,13 +126,26 @@ static RedPipeItem
> *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int
>  typedef struct {
>      void *data;
>      uint32_t size;
> +    CursorItem *item;
>  } AddBufInfo;
>  

This field here does not look right... I'd honestly remove
all the AddBufInfo structure, I'll post a possible different
patch.

> -static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info)
> +static void marshaller_free_cursor_item(uint8_t *data, void *opaque)
> +{
> +    CursorItem *item = opaque;
> +    if (item)
> +        cursor_item_unref(item);
> +}
> +
> +static void marshall_cursor_data(SpiceMarshaller *m, AddBufInfo *info)
>  {
> -    if (info->data) {
> -        spice_marshaller_add_by_ref(m, info->data, info->size);
> +    spice_return_if_fail(info != NULL);
> +    /* the cursor data is owned by 'item', so we need to ensure that 'item'
> +     * remains valid until the data is sent. */
> +    if (info->item) {
> +        cursor_item_ref(info->item);
>      }
> +    spice_marshaller_add_by_ref_full(m, info->data, info->size,
> +                                     marshaller_free_cursor_item,
> info->item);
>  }
>  
>  static void cursor_fill(CursorChannelClient *ccc, CursorItem *cursor,
> @@ -140,6 +153,7 @@ static void cursor_fill(CursorChannelClient *ccc,
> CursorItem *cursor,
>  {
>      RedCursorCmd *cursor_cmd;
>      addbuf->data = NULL;
> +    addbuf->item = cursor;
>  
>      if (!cursor) {
>          red_cursor->flags = SPICE_CURSOR_FLAGS_NONE;
> @@ -205,7 +219,7 @@ static void red_marshall_cursor_init(CursorChannelClient
> *ccc, SpiceMarshaller *
>  
>      cursor_fill(ccc, cursor_channel->item, &msg.cursor, &info);
>      spice_marshall_msg_cursor_init(base_marshaller, &msg);
> -    add_buf_from_info(base_marshaller, &info);
> +    marshall_cursor_data(base_marshaller, &info);
>  }
>  
>  static void cursor_marshall(CursorChannelClient *ccc,
> @@ -215,7 +229,6 @@ static void cursor_marshall(CursorChannelClient *ccc,
>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
>      CursorChannel *cursor_channel =
>      CURSOR_CHANNEL(red_channel_client_get_channel(rcc));
>      CursorItem *item = cursor_pipe_item->cursor_item;
> -    RedPipeItem *pipe_item = &cursor_pipe_item->base;
>      RedCursorCmd *cmd;
>  
>      spice_return_if_fail(cursor_channel);
> @@ -235,13 +248,13 @@ static void cursor_marshall(CursorChannelClient *ccc,
>              SpiceMsgCursorSet cursor_set;
>              AddBufInfo info;
>  
> -            red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_SET,
> pipe_item);
> +            red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_SET,
> NULL);
>              cursor_set.position = cmd->u.set.position;
>              cursor_set.visible = cursor_channel->cursor_visible;
>  
>              cursor_fill(ccc, item, &cursor_set.cursor, &info);
>              spice_marshall_msg_cursor_set(m, &cursor_set);
> -            add_buf_from_info(m, &info);
> +            marshall_cursor_data(m, &info);
>              break;
>          }
>      case QXL_CURSOR_HIDE:

Frediano