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

Submitted by Jonathon Jongsma on Dec. 1, 2016, 5:29 p.m.

Details

Message ID 1480613394.3281.23.camel@redhat.com
State New
Headers show
Series "dcc: Remove unneeded private header inclusion" ( rev: 4 ) in Spice

Not browsing as part of any series.

Commit Message

Jonathon Jongsma Dec. 1, 2016, 5:29 p.m.
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@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.



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:

Patch hide | download patch | download mbox

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,

Comments

> 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@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
On Fri, 2016-12-02 at 10:18 -0500, Frediano Ziglio wrote:
> > 
> > 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@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.

Hmm good point. Perhaps _add_data_by_ref()? Or just keep the shorter
add_ref().


> 
> > 
> >     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.

Yeah, I thought about "stealing" the data from the item and then having
the marhsaller free the data directly, but I think that becomes
slightly more complicated. Especially since the pipe item is refcounted
and some other part of the code may be keeping the pipe item alive and
expecting the data that it holds to remain valid.

So it sounds like you agree with the general idea. Would you like me to
work on a full solution here, or were you planning to work on it?

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


I'm afraid that I don't really understand exactly what you're trying to
say in this paragraph.

Jonathon.
> 
> On Fri, 2016-12-02 at 10:18 -0500, Frediano Ziglio wrote:
> > > 
> > > 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@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.
> 
> Hmm good point. Perhaps _add_data_by_ref()? Or just keep the shorter
> add_ref().
> 

The by make stuff much clearer!

Maybe just spice_marshaller_add_by_ref ?
Providing an inline deprecated function should help the rename..
or just rename, I think it's in spice-common and all spice-common
user use submodules.

> 
> > 
> > > 
> > >     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.
> 
> Yeah, I thought about "stealing" the data from the item and then having
> the marhsaller free the data directly, but I think that becomes
> slightly more complicated. Especially since the pipe item is refcounted
> and some other part of the code may be keeping the pipe item alive and
> expecting the data that it holds to remain valid.
> 
> So it sounds like you agree with the general idea. Would you like me to
> work on a full solution here, or were you planning to work on it?
> 

I think I have stuff to do till 2018 (and it's not a typo) :-)
I didn't think about, I was just adding the comment, the idea is yours.

> > 
> > 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).
> 
> 
> I'm afraid that I don't really understand exactly what you're trying to
> say in this paragraph.

It's confusing yes, but the code is also confusing. For the memory
point of view using the marshaller is perfectly fine. But suppose
you add the item just to know when data has been queued to network,
the change would make this not possible, at least in the way you can
do it now. I don't have all the possible items list in my mind but
if this is required by some item this could be a problem.

This items part was changed quite a lot in this year. For instance
there were no reference counting in PipeItem, now there are.

Probably I'm just paranoid, it's that I learned that spice-server
code has much hidden assumptions, I'm just trying to think if there
are some related to this part of the code.

> 
> Jonathon.
> 

Frediano
On Fri, 2016-12-02 at 11:17 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Fri, 2016-12-02 at 10:18 -0500, Frediano Ziglio wrote:
> > > 
> > > > 
> > > > 
> > > > 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@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.
> > 
> > Hmm good point. Perhaps _add_data_by_ref()? Or just keep the
> > shorter
> > add_ref().
> > 
> 
> The by make stuff much clearer!
> 
> Maybe just spice_marshaller_add_by_ref ?
> Providing an inline deprecated function should help the rename..
> or just rename, I think it's in spice-common and all spice-common
> user use submodules.

OK, will look into it.

> 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > >     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.
> > 
> > Yeah, I thought about "stealing" the data from the item and then
> > having
> > the marhsaller free the data directly, but I think that becomes
> > slightly more complicated. Especially since the pipe item is
> > refcounted
> > and some other part of the code may be keeping the pipe item alive
> > and
> > expecting the data that it holds to remain valid.
> > 
> > So it sounds like you agree with the general idea. Would you like
> > me to
> > work on a full solution here, or were you planning to work on it?
> > 
> 
> I think I have stuff to do till 2018 (and it's not a typo) :-)
> I didn't think about, I was just adding the comment, the idea is
> yours.

Sure, I'll work on a patch. I think we can probably either commit this
patch in the meantime (I think Christophe's wording suggestions are
fine), or just drop it.

Jonathon

> 
> > 
> > > 
> > > 
> > > 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).
> > 
> > 
> > I'm afraid that I don't really understand exactly what you're
> > trying to
> > say in this paragraph.
> 
> It's confusing yes, but the code is also confusing. For the memory
> point of view using the marshaller is perfectly fine. But suppose
> you add the item just to know when data has been queued to network,
> the change would make this not possible, at least in the way you can
> do it now. I don't have all the possible items list in my mind but
> if this is required by some item this could be a problem.
> 
> This items part was changed quite a lot in this year. For instance
> there were no reference counting in PipeItem, now there are.
> 
> Probably I'm just paranoid, it's that I learned that spice-server
> code has much hidden assumptions, I'm just trying to think if there
> are some related to this part of the code.

> 
> > 
> > 
> > Jonathon.
> > 
> 
> Frediano