[Spice-devel] [PATCH v2 7/9] worker: unify flush_cursor_commands and flush_display_commands

[Frediano Ziglio]

> 
> On Tue, 2016-01-26 at 14:51 +0100, Pavel Grunt wrote:
> > On Tue, 2016-01-26 at 09:44 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/red-worker.c | 68 ++++++++++++++++++-------------------------
> > > ----------
> > >  1 file changed, 23 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index dd20bd5..5eb54f2 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -368,20 +368,22 @@ static void red_migrate_display(DisplayChannel
> > > *display, RedChannelClient *rcc)
> > >      }
> > >  }
> > >  
> > > -static void flush_display_commands(RedWorker *worker)
> > > -{
> > > -    RedChannel *red_channel = RED_CHANNEL(worker->display_channel);
> > > +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)
> > > +{
> > >      for (;;) {
> > >          uint64_t end_time;
> > >          int ring_is_empty;
> > >  
> > > -        red_process_display(worker, &ring_is_empty);
> > > +        process(worker, &ring_is_empty);
> > >          if (ring_is_empty) {
> > >              break;
> > >          }
> > >  
> > > -        while (red_process_display(worker, &ring_is_empty)) {
> > > +        while (process(worker, &ring_is_empty)) {
> > >              red_channel_push(red_channel);
> > >          }
> > >  
> > > @@ -389,7 +391,6 @@ static void flush_display_commands(RedWorker
> > > *worker)
> > >              break;
> > >          }
> > >          end_time = spice_get_monotonic_time_ns() +
> > > COMMON_CLIENT_TIMEOUT;
> > > -        int sleep_count = 0;
> > >          for (;;) {
> > >              red_channel_push(red_channel);
> > >              if (red_channel_max_pipe_size(red_channel) <=
> > > MAX_PIPE_SIZE) {
> > > @@ -400,54 +401,30 @@ static void flush_display_commands(RedWorker
> > > *worker)
> > >              // 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) {
> > > -                spice_warning("update timeout");
> > > -                red_disconnect_all_display_TODO_remove_me(red_channe
> > > l);
> > > +                disconnect(worker);
> > >              } else {
> > > -                sleep_count++;
> > >                  usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
> > >              }
> > >          }
> > >      }
> > >  }
> > >  
> > > -static void flush_cursor_commands(RedWorker *worker)
> > > +static void red_disconnect_display(RedWorker *worker)
> > >  {
> > > -    RedChannel *red_channel = RED_CHANNEL(worker->cursor_channel);
> > > -
> > > -    for (;;) {
> > > -        uint64_t end_time;
> > > -        int ring_is_empty = FALSE;
> > > -
> > > -        red_process_cursor(worker, &ring_is_empty);
> > > -        if (ring_is_empty) {
> > > -            break;
> > > -        }
> > > +    spice_warning("update timeout");
> > > +    red_disconnect_all_display_TODO_remove_me(RED_CHANNEL(worker-
> > > > display_channel));
> > > +}
> 
> red_disconnect_all_display_TODO_remove_me() is exactly the same
> implementation
> as red_channel_disconnect(), except that it has a TODO comment in it. If we
> move
> that TODO note here (assuming we really want to keep it), we could just call
> red_channel_disconnect() here and remove the _remove_me() function...
> 
> 

>From 
http://cgit.freedesktop.org/~fziglio/spice-server/commit/?id=7e8e13593ee681cf04c349bca57dd225d7802494
looks like this _TODO_remove_me was introduced in an attempt to improve
multiple client support.
The names was red_disconnect_display (like the name I'm using for the new function).
I think would be worth having a single function.
I'll keep the comment. It make sense. Closing all clients is too much, closing
hanging ones would make more sense.

Frediano

> > >  
> > > -        while (red_process_cursor(worker, &ring_is_empty)) {
> > > -            red_channel_push(red_channel);
> > > -        }
> > > +static void flush_display_commands(RedWorker *worker)
> > > +{
> > > +    flush_commands(worker, RED_CHANNEL(worker->display_channel),
> > > +                   red_process_display, red_disconnect_display);
> > > +}
> > >  
> > > -        if (ring_is_empty) {
> > > -            break;
> > > -        }
> > > -        end_time = spice_get_monotonic_time_ns() +
> > > COMMON_CLIENT_TIMEOUT;
> > > -        int sleep_count = 0;
> > > -        for (;;) {
> > > -            red_channel_push(red_channel);
> > > -            if (red_channel_max_pipe_size(red_channel) <=
> > > MAX_PIPE_SIZE) {
> > > -                break;
> > > -            }
> > > -            red_channel_receive(red_channel);
> > > -            red_channel_send(red_channel);
> > > -            if (spice_get_monotonic_time_ns() >= end_time) {
> > > -                spice_warning("flush cursor timeout");
> > > -                cursor_channel_disconnect(worker->cursor_channel);
> > > -            } else {
> > > -                sleep_count++;
> > > -                usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
> > > -            }
> > > -        }
> > > -    }
> > > +static void red_disconnect_cursor(RedWorker *worker)
> > > +{
> > > +    spice_warning("flush cursor timeout");
> > > +    cursor_channel_disconnect(worker->cursor_channel);
> > >  }
> > >  
> > >  // TODO: on timeout, don't disconnect all channels immediatly - try
> > > to disconnect the slowest ones
> > > @@ -456,7 +433,8 @@ static void flush_cursor_commands(RedWorker
> > > *worker)
> > >  static void flush_all_qxl_commands(RedWorker *worker)
> > >  {
> > >      flush_display_commands(worker);
> > > -    flush_cursor_commands(worker);
> > > +    flush_commands(worker, RED_CHANNEL(worker->cursor_channel),
> > > +                   red_process_cursor, red_disconnect_cursor);
> > 
> > Looks good, but for symmetry I would keep flush_cursor_commands
> > 
> > Pavel
> > >  }
> > >  
> > >  static int common_channel_config_socket(RedChannelClient *rcc)