[Spice-devel] [PATCH spice-server v2] red-channel-client: Simplify red_channel_client_wait_pipe_item_sent loop

[Christophe Fergeau]

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 at redhat.com>
> ---
>  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 at redhat.com>

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel