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

[Jonathon Jongsma]

On Fri, 2016-12-16 at 11:23 -0500, 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 | 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.

I didn't like it either. In fact, I tried 2 different approaches before
this, but wasn't happy with either of them. So I decided to just go
this route. If you can come up with a more elegant solution, I'm happy
to use your 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