[Spice-devel] [PATCH 14/16] red_channel: cleanup of red_channel_client blocking methods

[Frediano Ziglio]

> 
> From: Yonit Halperin <yhalperi at redhat.com>
> 
> (1) receive timeout as a parameter.
> (2) add a return value and pass the handling
>     of failures to the calling routine.
> ---
>  server/cursor-channel.c | 1 +
>  server/dcc.h            | 1 +
>  server/red_channel.c    | 8 +++++---
>  server/red_worker.c     | 4 ++--
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 840ff30..4854bc6 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -29,6 +29,7 @@
>  #define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
>  #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
>  #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> +#define CURSOR_CLIENT_TIMEOUT 30000000000ULL //nano
>  
>  enum {
>      PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
> diff --git a/server/dcc.h b/server/dcc.h
> index 7b8a3b8..c0a6623 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -31,6 +31,7 @@
>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>  #define CLIENT_PALETTE_CACHE_SIZE 128
>  
> +#define DISPLAY_CLIENT_SHORT_TIMEOUT 15000000000ULL //nano
>  #define DISPLAY_CLIENT_TIMEOUT 30000000000ULL //nano
>  #define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT 10000000000ULL //nano, 10 sec
>  #define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro

Where are these new define used ??

> diff --git a/server/red_channel.c b/server/red_channel.c
> index 948d354..e5c6ac6 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -2337,7 +2337,7 @@ int
> red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
>          spice_warning("timeout");
>          return FALSE;
>      } else {
> -        spice_assert(red_channel_client_no_item_being_sent(rcc));
> +        spice_warn_if_fail(red_channel_client_no_item_being_sent(rcc));
>          return TRUE;
>      }
>  }

No opinion on this.

> @@ -2366,8 +2366,8 @@ int
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>      }
>      red_channel_client_push(rcc);
>  
> -    while((item_in_pipe = ring_item_is_linked(&item->link)) &&
> -          (timeout == -1 || red_get_monotonic_time() < end_time)) {
> +    while ((item_in_pipe = ring_item_is_linked(&item->link)) &&
> +           (timeout == -1 || red_get_monotonic_time() < end_time)) {
>          usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
>          red_channel_client_receive(rcc);
>          red_channel_client_send(rcc);

just space changes

> @@ -2425,4 +2425,6 @@ void
> red_channel_client_disconnect_if_pending_send(RedChannelClient *rcc)
>      } else {
>          spice_assert(red_channel_client_no_item_being_sent(rcc));
>      }
> +
> +    spice_warn_if_fail(red_channel_client_no_item_being_sent(rcc));
>  }

Does not make sense to me. If we didn't hit the assert this means
that client has been disconnected so we caring about the pipe?
Is being deleted.

I'll keep, I'd like to get some extra comments.

> diff --git a/server/red_worker.c b/server/red_worker.c
> index c65f1b2..5175839 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -955,12 +955,12 @@ static void handle_dev_stop(void *opaque, void
> *payload)
>      if (!red_channel_wait_all_sent(RED_CHANNEL(worker->display_channel),
>                                     DISPLAY_CLIENT_TIMEOUT)) {
>          red_channel_apply_clients(RED_CHANNEL(worker->display_channel),
> -
> red_channel_client_disconnect_if_pending_send);
> +
> red_channel_client_disconnect_if_pending_send);
>      }
>      if (!red_channel_wait_all_sent(RED_CHANNEL(worker->cursor_channel),
>                                     DISPLAY_CLIENT_TIMEOUT)) {
>          red_channel_apply_clients(RED_CHANNEL(worker->cursor_channel),
> -
> red_channel_client_disconnect_if_pending_send);
> +
> red_channel_client_disconnect_if_pending_send);
>      }
>  }
>  

other spaces changes

I'll remove space changes and defines.

The comment needs to be surely changed. Proposes welcome (like comments)

Frediano