[Spice-devel] [PATCH spice-server 09/10] red-channel-client: Simplify red_channel_client_wait_pipe_item_sent

[Frediano Ziglio]

> 
> On Mon, Sep 11, 2017 at 11:15:46AM +0100, Frediano Ziglio wrote:
> > Instead of inserting the marker after the item (the tail of the queue
> > is the first item to send) and then have to wait again if for
> > the specific item place the marker before the item so waiting for
> > the marker to be sent assure that we sent also the item.
> > This avoids having to call red_channel_client_wait_outgoing_item
> > and possibly the case where the item was not queued and
> > red_channel_client_wait_outgoing_item returning TRUE even if
> > the item was not sent as required.
> 
> As usual, I'd rework a bit the commit log (more punctuation/new lines, more
> accurate references to typenames/variable names, ..):
> 
> "Currently, red_channel_client_wait_pipe_item_sent() inserts a MarkerItem
> which
> will sent before the item we want to wait for: the tail of the queue is
> the first item to send, and the function uses
> red_channel_client_pipe_add_before_pos(). Then, if the marker has been

I think here should be red_channel_client_pipe_add_after_pos.

> successfully sent, the function calls red_channel_client_wait_outgoing_item
> to wait for 'item' to be sent.
> 
> Instead of doing this, we can add the MarkerItem to the queue so that
> it's sent after 'item' (ie, insert it _before_ 'item' in the queue).
> This way, when the marker is marked as having been sent, we'll also know
> that 'item' has been sent.
> 
> This avoids having to call red_channel_client_wait_outgoing_item
> and possibly the case where the item was not queued and
> red_channel_client_wait_outgoing_item returning TRUE even if
> the item was not sent as required."
> 

Other part is perfect.

> I believe the way this function works now is a result of the last hunk of
> https://cgit.freedesktop.org/spice/spice/commit/?id=c59b2884a2f7fc953fdb263085830b65e8bdcaef
> + the introduction of MarkerItem (I guess when switching to using a
> GQueue).
> 

Not sure... looks like the patch you are referring is doing something else.

> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Frediano

> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel-client.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index f0a25ecfd..d9333ba6f 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -1569,6 +1569,19 @@ void
> > red_channel_client_pipe_add_after_pos(RedChannelClient *rcc,
> >      g_queue_insert_after(&rcc->priv->pipe, pipe_item_pos, item);
> >  }
> >  
> > +static void
> > +red_channel_client_pipe_add_before_pos(RedChannelClient *rcc,
> > +                                       RedPipeItem *item,
> > +                                       GList *pipe_item_pos)
> > +{
> > +    spice_assert(pipe_item_pos);
> > +    if (!prepare_pipe_add(rcc, item)) {
> > +        return;
> > +    }
> > +
> > +    g_queue_insert_before(&rcc->priv->pipe, pipe_item_pos, item);
> > +}
> > +
> >  void red_channel_client_pipe_add_after(RedChannelClient *rcc,
> >                                         RedPipeItem *item,
> >                                         RedPipeItem *pos)
> > @@ -1779,7 +1792,7 @@ bool
> > red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
> >      red_pipe_item_init(&mark_item->base, RED_PIPE_ITEM_TYPE_MARKER);
> >      mark_item->item_in_pipe = true;
> >      red_pipe_item_ref(&mark_item->base);
> > -    red_channel_client_pipe_add_after_pos(rcc, &mark_item->base,
> > item_pos);
> > +    red_channel_client_pipe_add_before_pos(rcc, &mark_item->base,
> > item_pos);
> >  
> >      for (;;) {
> >          red_channel_client_receive(rcc);
> > @@ -1799,10 +1812,8 @@ bool
> > red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
> >          // still on the queue
> >          spice_warning("timeout");
> >          return FALSE;
> > -    } else {
> > -        return red_channel_client_wait_outgoing_item(rcc,
> > -                                                     timeout == -1 ? -1 :
> > end_time - spice_get_monotonic_time_ns());
> >      }
> > +    return TRUE;
> >  }
> >  
> >  bool red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
> > --
> > 2.13.5
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>