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

[Jonathon Jongsma]

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?)

    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: