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