[Spice-devel] [PATCH spice-server] Add a comment to red_channel_client_init_send_data
Jonathon Jongsma
jjongsma at redhat.com
Fri Dec 2 16:05:08 UTC 2016
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 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.
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.
More information about the Spice-devel
mailing list