On Tue, Sep 12, 2017 at 02:03:24PM +0100, Frediano Ziglio wrote:
With some added spaces, the log is a bit easier to read imo :)
"Avoid repeating the same code twice.
red_channel_client_send sends the pending item (or a part of it). If
there are no item pending, the function does nothing (so checking for
blocked channel is useless). Also red_channel_client_send is already
called from red_channel_client_push which has a check for blocked
channels, so having calls to both red_channel_client_send() and
red_channel_client_push() is redundant.
The function on its overall tries to wait for a given item to be sent.
The call for red_channel_client_receive is mainly needed to support the
cases were to send data messages from the client should be processed
(like if "handle-acks" is requested).
Moving the loop iteration check inside the for loop instead allows to
avoid some duplication."
(I'll note that the removal of red_channel_client_push() would have made
sense in a separate commit, makes the commit log smaller and more
specific ;)
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-channel-client.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> Changes since v1:
> - removed red_channel_client_send call;
> - extend commit message.
>
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index de3ac27cb..792ad167b 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1781,18 +1781,14 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
> red_pipe_item_ref(&mark_item->base);
> red_channel_client_pipe_add_after_pos(rcc, &mark_item->base, item_pos);
>
> - if (red_channel_client_is_blocked(rcc)) {
> - red_channel_client_receive(rcc);
> - red_channel_client_send(rcc);
> - }
> - red_channel_client_push(rcc);
> -
My only comment is that we lost a if
(red_channel_client_is_blocked(rcc)) { red_channel_client_receive(rcc); }
I'd assume that it's not really important, and that it's fine to do it
unconditionally before the sleep?
Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Christophe
> - while (mark_item->item_in_pipe &&
> - (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
> - usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
> + for (;;) {
> red_channel_client_receive(rcc);
> - red_channel_client_send(rcc);
> red_channel_client_push(rcc);
> + if (!mark_item->item_in_pipe ||
> + (timeout != -1 && spice_get_monotonic_time_ns() >= end_time)) {
> + break;
> + }
> + usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
> }
>
> item_in_pipe = mark_item->item_in_pipe;
> --
> 2.13.5
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel