[Spice-devel] [PATCH spice-server v2 modified] Refactor cursor marshalling for SET, INIT

[Jonathon Jongsma]

On Fri, 2016-12-16 at 16:25 +0000, Frediano Ziglio wrote:
> 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 | 43 ++++++++++++++++++++++-----------------
> ----
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> Changes sinvce v2:
> - avoid to use AddBufInfo structure, it's confusing and
>   all fields are in CursorItem/SpiceCursor already;
>   This make cursor_fill change just a parameter, 2 is
>   confusing.
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index fded226..905a85d 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -123,26 +123,34 @@ static RedPipeItem
> *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int
>      return &item->base;
>  }
>  
> -typedef struct {
> -    void *data;
> -    uint32_t size;
> -} AddBufInfo;
> +static void marshaller_free_cursor_item(uint8_t *data, void *opaque)
> +{
> +    CursorItem *item = opaque;
> +    cursor_item_unref(item);
> +}
>  
> -static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info)
> +static void marshall_cursor_data(SpiceMarshaller *m, CursorItem
> *cursor_item,
> +                                 const SpiceCursor *red_cursor)
>  {
> -    if (info->data) {
> -        spice_marshaller_add_by_ref(m, info->data, info->size);
> +    spice_assert(red_cursor != NULL);
> +    /* the cursor data is owned by 'cursor_item', so we need to
> ensure that 'cursor_item'
> +     * remains valid until the data is sent. */
> +    if (red_cursor->data != NULL && red_cursor->data_size &&
> cursor_item != NULL) {
> +        cursor_item_ref(cursor_item);
> +        spice_marshaller_add_by_ref_full(m, red_cursor->data,
> red_cursor->data_size,
> +                                         marshaller_free_cursor_item
> , cursor_item);
>      }
>  }


I believe this is actually one of the approaches that I tried before
settling on the other approach. The reason that I didn't like it is
because it seems a bit confusing and easy to misuse. We pass two items
to the function that we assume to be related (for example, we use the
data from red_cursor, but increment the reference of cursor_item
instead of red_cursor), but there is nothing that guarantees that they
are actually related. Of course, this is just an internal helper
function, so with adequate documentation, it probably won't be misused.
And I admit that my approach was pretty ugly, so I'm ok to use this
approach after all. But I'd like to add a comment above this function
noting the assumption that cursor_item owns red_cursor.


>  
>  static void cursor_fill(CursorChannelClient *ccc, CursorItem
> *cursor,
> -                        SpiceCursor *red_cursor, AddBufInfo *addbuf)
> +                        SpiceCursor *red_cursor)
>  {
>      RedCursorCmd *cursor_cmd;
> -    addbuf->data = NULL;
>  
>      if (!cursor) {
>          red_cursor->flags = SPICE_CURSOR_FLAGS_NONE;
> +        red_cursor->data_size = 0;
> +        red_cursor->data = NULL;
>          return;
>      }
>  
> @@ -158,11 +166,6 @@ static void cursor_fill(CursorChannelClient
> *ccc, CursorItem *cursor,
>              red_cursor->flags |= SPICE_CURSOR_FLAGS_CACHE_ME;
>          }
>      }
> -
> -    if (red_cursor->data_size) {
> -        addbuf->data = red_cursor->data;
> -        addbuf->size = red_cursor->data_size;
> -    }
>  }
>  
>  void cursor_channel_disconnect(CursorChannel *cursor_channel)
> @@ -192,7 +195,6 @@ static void
> red_marshall_cursor_init(CursorChannelClient *ccc, SpiceMarshaller *
>      CursorChannel *cursor_channel;
>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
>      SpiceMsgCursorInit msg;
> -    AddBufInfo info;
>  
>      spice_assert(rcc);
>      cursor_channel =
> CURSOR_CHANNEL(red_channel_client_get_channel(rcc));
> @@ -203,9 +205,9 @@ static void
> red_marshall_cursor_init(CursorChannelClient *ccc, SpiceMarshaller *
>      msg.trail_length = cursor_channel->cursor_trail_length;
>      msg.trail_frequency = cursor_channel->cursor_trail_frequency;
>  
> -    cursor_fill(ccc, cursor_channel->item, &msg.cursor, &info);
> +    cursor_fill(ccc, cursor_channel->item, &msg.cursor);
>      spice_marshall_msg_cursor_init(base_marshaller, &msg);
> -    add_buf_from_info(base_marshaller, &info);
> +    marshall_cursor_data(base_marshaller, cursor_channel->item,
> &msg.cursor);

This demonstrates how that the relationship between the CursorItem and
the RedCursor that are passed to marshall_cursor_data() is not very
obvious. &msg.cursor is actually cursor_channel->item->red_cursor-
>u.set.shape. So the RedCursor is clearly owned by the CursorItem, but
that's hard to see since the cursor_fill() function extracts that into
a local variable for us.


>  }
>  
>  static void cursor_marshall(CursorChannelClient *ccc,
> @@ -233,15 +235,14 @@ static void cursor_marshall(CursorChannelClient
> *ccc,
>      case QXL_CURSOR_SET:
>          {
>              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);
> +            cursor_fill(ccc, item, &cursor_set.cursor);
>              spice_marshall_msg_cursor_set(m, &cursor_set);
> -            add_buf_from_info(m, &info);
> +            marshall_cursor_data(m, item, &cursor_set.cursor);
>              break;
>          }
>      case QXL_CURSOR_HIDE:


ACK with comment mentioned above