[Spice-devel] [PATCH spice-server] Add a comment to red_channel_client_init_send_data
Frediano Ziglio
fziglio at redhat.com
Fri Dec 2 16:17:31 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().
>
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
More information about the Spice-devel
mailing list