[Spice-devel] [PATCH spice-server] Add a comment to red_channel_client_init_send_data

[Frediano Ziglio]

> On Thu, 2016-11-24 at 08:57 -0500, Frediano Ziglio wrote:
> > > 
> > > 
> > > On Thu, Nov 24, 2016 at 10:48:24AM +0000, Frediano Ziglio wrote:
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/red-channel-client.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/server/red-channel-client.h b/server/red-channel-
> > > > client.h
> > > > index 94b4f58..93d0315 100644
> > > > --- a/server/red-channel-client.h
> > > > +++ b/server/red-channel-client.h
> > > > @@ -89,6 +89,9 @@ void
> > > > red_channel_client_shutdown(RedChannelClient *rcc);
> > > >  int red_channel_client_handle_message(RedChannelClient *rcc,
> > > > uint32_t
> > > >  size,
> > > >                                        uint16_t type, void
> > > > *message);
> > > >  /* when preparing send_data: should call init and then use
> > > > marshaller */
> > > > +/* item is retained as long as the message is sent to the
> > > > client,
> > > 
> > > "The item is retained", then I don't understand when the item stops
> > > being retained. "The item is only released after the message has
> > > been
> > > sent to the client" ?
> > > 
> > > > 
> > > > + * this is used for instance to make sure an attached data
> > > > referenced
> > > 
> > > "This is used for instance to make sure attached data ..."
> > > 
> > > > 
> > > > + * by the marshaller is still valid when data are used */
> > > 
> > > and I would say "when it's used"
> > > 
> > > Maybe better to wait for Jonathon's input on the phrasing?
> > > 
> > > Christophe
> > > 
> > 
> > I'll wait, in the meantime I updated the commit log.
> > 
> > What's not clear with this API is when to pass the item and when not.
> > Basically when there is a spice_marshaller_add_ref in the code that
> > send the item potentially it's required to pass the item to
> > red_channel_client_init_send_data, otherwise passing NULL will free
> > the item when *_send_item returns (if not references elsewhere).
> > 
> > Frediano
> 
> 
> 
> Hmm, this is a weird function. I'm tempted to re-design the whole thing
> rather than just adding a comment.
> 
> I'm still a bit confused about when you're supposed to pass an item to
> this function, even after your description. As far as I can tell, it's
> only required when a marshaller function doesn't copy all of the data
> from the PipeItem but instead references some data from that PipeItem
> by pointer? In that case, the PipeItem needs to stay alive until we
> actually send the message so that it can read that data to send it over
> the network.
> 
> As far as I can tell, what you say about spice_marshaller_add_ref() is
> sort of correct, but only if the data that we're adding is owned by the
> PipeItem. (by the way, spice_marshaller_add_ref() is a weird name,
> since it doesn't seem to have anythign to do with references?? Perhaps
> spice_marshaller_add_data_buf() would be more accurate?)
> 

Personally add_ref is more clear, add_data_buf looks more a copy.

>     Here's a very rough proof-of-concept that I think is a bit nicer.
> It only tackles one single occurence of this pattern, but would perhaps
> allow us to remove this third parameter from _init_send_data()
> completely if it was done for all other cases. Note that it has
> undergone almost no testing; I post it only for discussion. A nicer
> solution would involve improving/redesigning the add_buf_from_info()
> function since the pipe_item argument appears unrelated and tacked on.
> Comments appreciated.
> 
> 
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index e421bf7..49e5229 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -128,10 +128,20 @@ typedef struct {
>      uint32_t size;
>  } AddBufInfo;
>  
> -static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info)
> +static void unref_buf(uint8_t* data, void *opaque)
> +{
> +    /* the data is owned by the pipe item, so just unref the pipe item
> */
> +    RedPipeItem *item = opaque;
> +    red_pipe_item_unref(item);
> +}
> +
> +static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info,
> RedPipeItem *item)
>  {
>      if (info->data) {
> -        spice_marshaller_add_ref(m, info->data, info->size);
> +        /* the pipe item ultimately owns the data, so keep it alive
> until the
> +         * data is no longer needed */
> +        red_pipe_item_ref(item);
> +        spice_marshaller_add_ref_full(m, info->data, info->size,
> unref_buf, item);
>      }
>  }
>  
> @@ -205,7 +215,7 @@ static void
> red_marshall_cursor_init(CursorChannelClient *ccc, SpiceMarshaller *
>  
>      cursor_fill(ccc, &msg.cursor, cursor_channel->item, &info);
>      spice_marshall_msg_cursor_init(base_marshaller, &msg);
> -    add_buf_from_info(base_marshaller, &info);
> +    add_buf_from_info(base_marshaller, &info, pipe_item);
>  }
>  
>  static void cursor_marshall(CursorChannelClient *ccc,
> @@ -235,13 +245,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, &cursor_set.cursor, item, &info);
>              spice_marshall_msg_cursor_set(m, &cursor_set);
> -            add_buf_from_info(m, &info);
> +            add_buf_from_info(m, &info, pipe_item);
>              break;
>          }
>      case QXL_CURSOR_HIDE:
> 
> 

I think to sum up, use spice_marshaller_add_ref_full instead of
spice_marshaller_add_ref so marshaller will release and remove the
parameter from spice_marshall_msg_cursor_init. Make perfectly sense.
Actually what's the expense of keeping the item always and freeing when
finished sending? This could be another option. But I prefer yours.
For instance we could keep the image and free the item.

I don't think that make any difference if items are informed when item
is dequeued or when fully sent. When we start sending an item we'll send
it all and we don't have a clear way to understand if client received it.
I was thinking that there are different code that does something
specific when the item is freed so the timing will change a bit. Still don't
think that will make any difference. There are 2 cases when this timing
can be effective. Congestion (so network buffer is full) and images (so
big messages).

Frediano