[Spice-devel] [spice-server 4/4] worker: Simplify flush_commands()

[Christophe Fergeau]

On Wed, Aug 30, 2017 at 03:10:21PM -0400, Frediano Ziglio wrote:
> > 
> > red_disconnect_display() is duplicating what red_channel_disconnect()
> > already does, so red_disconnect_display() and red_disconnect_cursor()
> > are actually identical code-wise. We can directly call
> > red_channel_disconnect() from flush_commands() rather than passing a
> > 'red_disconnect_t disconnect' argument to that function.
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > ---
> >  server/red-worker.c | 28 +++++-----------------------
> >  1 file changed, 5 insertions(+), 23 deletions(-)
> > 
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 109db63e0..fc009454c 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -284,17 +284,6 @@ static bool red_process_is_blocked(RedWorker *worker)
> >             red_channel_max_pipe_size(RED_CHANNEL(worker->display_channel)) >
> >             MAX_PIPE_SIZE;
> >  }
> >  
> > -static void red_disconnect_display(RedWorker *worker)
> > -{
> > -    spice_warning("update timeout");
> > -
> > -    // TODO: we need to record the client that actually causes the timeout.
> > So
> > -    // we need to check the locations of the various pipe heads when
> > counting,
> > -    // and disconnect only those/that.
> 
> I would personally keep this comment and put before the spice_warning below

Ok.

> 
> > -    red_channel_apply_clients(RED_CHANNEL(worker->display_channel),
> > -                              red_channel_client_disconnect);
> > -}
> > -
> >  static void red_migrate_display(DisplayChannel *display, RedChannelClient
> >  *rcc)
> >  {
> >      /* We need to stop the streams, and to send upgrade_items to the client.
> > @@ -314,10 +303,8 @@ static void red_migrate_display(DisplayChannel *display,
> > RedChannelClient *rcc)
> >  }
> >  
> >  typedef int (*red_process_t)(RedWorker *worker, int *ring_is_empty);
> > -typedef void (*red_disconnect_t)(RedWorker *worker);
> > -
> >  static void flush_commands(RedWorker *worker, RedChannel *red_channel,
> > -                           red_process_t process, red_disconnect_t
> > disconnect)
> > +                           red_process_t process)
> >  {
> >      for (;;) {
> >          uint64_t end_time;
> > @@ -346,7 +333,8 @@ static void flush_commands(RedWorker *worker, RedChannel
> > *red_channel,
> >              // TODO: MC: the whole timeout will break since it takes lowest
> >              timeout, should
> >              // do it client by client.
> >              if (spice_get_monotonic_time_ns() >= end_time) {
> > -                disconnect(worker);
> > +		spice_warning("flush timeout");
> 
> this is indented with tab. I would put the old comment before this warning.

Ah, sorry, currently working with the default vim configuration, I'll
fix that (this patch, and the config :)

> 
> > +                red_channel_disconnect(red_channel);
> >              } else {
> >                  usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
> >              }
> > @@ -357,19 +345,13 @@ static void flush_commands(RedWorker *worker,
> > RedChannel *red_channel,
> >  static void flush_display_commands(RedWorker *worker)
> >  {
> >      flush_commands(worker, RED_CHANNEL(worker->display_channel),
> > -                   red_process_display, red_disconnect_display);
> > -}
> > -
> > -static void red_disconnect_cursor(RedWorker *worker)
> > -{
> > -    spice_warning("flush cursor timeout");
> > -    red_channel_disconnect(RED_CHANNEL(worker->cursor_channel));
> > +                   red_process_display);
> >  }
> >  
> >  static void flush_cursor_commands(RedWorker *worker)
> >  {
> >      flush_commands(worker, RED_CHANNEL(worker->cursor_channel),
> > -                   red_process_cursor, red_disconnect_cursor);
> > +                   red_process_cursor);
> >  }
> >  
> >  // TODO: on timeout, don't disconnect all channels immediatly - try to
> 
> OT: typo in comment
> 
> >  disconnect the slowest ones
> 
> Beside these small comments I would ack the entire series.

Ok, I'll fix these comments, and push an additional commit with the
comment typo fix.

Thanks !

Christophe