>
> 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@xxxxxxxxxx>
>
> 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);
>
Sounds good.
>
>
> 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
>
Indeed you have found a pattern in the code. The program is trying to disconnect
clients which cannot flush their data in a given timeout. My only concern is
founding a name better than
red_channel_disconnect_clients_that_cannot_flush_data_in_a_given_timeout
:-)
Maybe
red_channel_disconnect_if_pending_timeout ?
OT: was looking at where this code is. Why it needs to flush all data stopping
the device? Stop does not happen just when machine is paused? Maybe something
to do with migration?
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel