[Spice-devel] [spice-server 1/3] channel: Call red_channel_disconnect_if_pending_send() from red_channel_wait_all_sent()

Christophe Fergeau cfergeau at redhat.com
Fri Sep 8 12:04:04 UTC 2017


On Wed, Sep 06, 2017 at 08:11:39AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 04, 2017 at 10:39:53AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Mon, Sep 04, 2017 at 09:04:03AM -0400, Frediano Ziglio wrote:
> > > > > > 
> > > > > > On Mon, Sep 04, 2017 at 06:06:57AM -0400, Frediano Ziglio wrote:
> > > > > > > > 
> > > > > > > > On Fri, Sep 01, 2017 at 06:19:28AM -0400, Frediano Ziglio wrote:
> > > > > > > > > > 
> > > > > > > > > > red_channel_disconnect_if_pending_send() and
> > > > > > > > > > red_channel_wait_all_sent()
> > > > > > > > > > are
> > > > > > > > > > always called together, we can remove one of the 2 methods.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Looks a good idea but I think that the function deserve a new
> > > > > > > > > name
> > > > > > > > 
> > > > > > > > I would not know how to change the name though :-/
> > > > > > > > 
> > > > > > > 
> > > > > > > Mumble... red_channel_goodbye_bad_guys ? :-)
> > > > > > > More serious red_channel_disconnect_slow_clients.
> > > > > > 
> > > > > > Oh wait, were you talking about
> > > > > > red_channel_disconnect_if_pending_send()?
> > > > > > I thought you wanted to rename red_channel_wait_all_sent()
> > > > > > 
> > > > > > Christophe
> > > > > > 
> > > > > 
> > > > > Yes, red_channel_wait_all_sent. The callers at the end with the
> > > > > new function will get this service, right? The wait is a "detail"
> > > > > that can be documented as the way to detect the slow ones.
> > > > 
> > > > For me the main intent of red_channel_wait_all_sent() is to try as hard
> > > > as possible to flush pending data. The disconnection is just a last
> > > > resort measure if there really is a client which is far too slow to
> > > > flush its queue. So I don't see
> > > > s/red_channel_wait_all_sent/red_channel_disconnect_slow_clients/ as a
> > > > good renaming. I definitely would do
> > > > s/red_channel_disconnect_if_pending_send/red_channel_disconnect_slow_clients/
> > > > though.
> > > > 
> > > > Christophe
> > > > 
> > > 
> > > I personally expect a wait function to not close a connection.
> > > If I call poll(2) with a timeout the poll does not close the file
> > > descriptor.
> > 
> > It does not have much to do with polling imo, it's just flushing
> > whatever is pending, with a timeout. I understand it can be odd, since
> > this function is fairly specialized already, I would say this is
> > acceptable with some additional documentation.
> > red_channel_flush_and_disconnect_slow_clients() is getting long :)
> > 
> > Christophe
> > 
> 
> I really feel the name is misleading, but if I'm the only one:
> 
> Acked-by: Frediano Ziglio <fziglio at redhat.com>

Would some documentation help a bit:

diff --git a/server/red-channel.h b/server/red-channel.h
index 8bec1f306..4cb18eb7b 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -210,9 +210,13 @@ const RedChannelCapabilities* red_channel_get_local_capabilities(RedChannel *sel
  *
  * timeout is in nano sec. -1 for no timeout.
  *
+ * This method tries for up to @timeout nanoseconds to send all the
+ * items which are currently queued. If the timeout elapses,
+ * the RedChannelClient which are too slow (those which still have pending
+ * items) will be disconnected.
+ *
  * Return: TRUE if waiting succeeded. FALSE if timeout expired.
  */
-
 bool red_channel_wait_all_sent(RedChannel *channel,
                                int64_t timeout);
 


Looking at where it's used, the function main purpose really is to flush
whatever data we have pending, not to disconnect clients.

An alternative would be to just keep the 2 function calls:

        if (!red_channel_wait_all_sent(channel,
                                       COMMON_CLIENT_TIMEOUT)) {
            red_channel_apply_clients(channel,
                                      red_channel_client_disconnect_if_pending_send);
        }

but add a helper for the disconnection:

        if (!red_channel_wait_all_sent(channel,
                                       COMMON_CLIENT_TIMEOUT)) {
            red_channel_disconnect_slow_clients(channel);
        }

Christophe


More information about the Spice-devel mailing list