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

[Jonathon Jongsma]

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...


> >  
> > -        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)
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel