[Spice-devel] [spice-server 2/3] channel: Remove red_channel_client_disconnect_if_pending_send()

[Frediano Ziglio]

> 
> There is exactly one user in RedChannel, and this can be reimplemented
> using already public RedChannelClient API. No need for an extra
> function.
> 

Actually you are removing a function and adding another.

> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/red-channel-client.c |  9 ---------
>  server/red-channel-client.h |  1 -
>  server/red-channel.c        | 16 +++++++++++++++-
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 2612a6db4..061600df3 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1824,15 +1824,6 @@ bool
> red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
>      }
>  }
>  
> -void red_channel_client_disconnect_if_pending_send(RedChannelClient *rcc)
> -{
> -    if (red_channel_client_is_blocked(rcc) ||
> !g_queue_is_empty(&rcc->priv->pipe)) {
> -        red_channel_client_disconnect(rcc);
> -    } else {
> -        spice_assert(red_channel_client_no_item_being_sent(rcc));
> -    }
> -}
> -
>  gboolean red_channel_client_no_item_being_sent(RedChannelClient *rcc)
>  {
>      return !rcc || (rcc->priv->send_data.size == 0);
> diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> index 3665daccd..6e03656b0 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -142,7 +142,6 @@ bool
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>                                              int64_t timeout);
>  bool red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
>                                             int64_t timeout);
> -void red_channel_client_disconnect_if_pending_send(RedChannelClient *rcc);
>  
>  RedChannel* red_channel_client_get_channel(RedChannelClient *rcc);
>  
> diff --git a/server/red-channel.c b/server/red-channel.c
> index b9d7f533d..f8fb4a3d3 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -647,6 +647,20 @@ uint32_t red_channel_sum_pipes_size(RedChannel *channel)
>      return sum;
>  }
>  
> +static void red_channel_disconnect_if_pending_send(RedChannel *channel)
> +{
> +    GList *it;
> +
> +    for (it = channel->priv->clients; it != NULL; it = it->next) {

Better to reuse the macro, it can disappear during the disconnection
causing a use-after-free reading next field.

> +        RedChannelClient *rcc = RED_CHANNEL_CLIENT(it->data);
> +        if (red_channel_client_is_blocked(rcc) ||
> !red_channel_client_pipe_is_empty(rcc)) {
> +            red_channel_client_disconnect(rcc);
> +        } else {
> +            spice_assert(red_channel_client_no_item_being_sent(rcc));
> +        }
> +    }
> +}
> +

Basically you inlined red_channel_client_disconnect_if_pending_send.

So basically a RedChannel function which calls lot (4) of RedChannelClient
functions. Well... not a regression but I don't see the gain.
If you are afraid of a public function maybe an header to shared private
communication between RedChannel and RedChannelClient is an idea.
If you are afraid of object size and you want the function to be inlined
I would push for LTO or have a single module compilation.

>  bool red_channel_wait_all_sent(RedChannel *channel,
>                                 int64_t timeout)
>  {
> @@ -674,7 +688,7 @@ bool red_channel_wait_all_sent(RedChannel *channel,
>      if (max_pipe_size || blocked) {
>          spice_warning("timeout: pending out messages exist (pipe-size %u,
>          blocked %d)",
>                        max_pipe_size, blocked);
> -        red_channel_apply_clients(channel,
> red_channel_client_disconnect_if_pending_send);
> +        red_channel_disconnect_if_pending_send(channel);
>          return FALSE;
>      } else {
>          spice_assert(red_channel_no_item_being_sent(channel));

Frediano