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