Re: [PATCH 1/2] server: remove dispatching creation of worker channels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> The commit log is a bit terse and difficult to parse, but the patch
> looks OK. It's clearly an intermediate step in a refactoring, but
> that's OK.
> 
> Possible suggestion for commit log:
> 
> "Create display and cursor channels in RedWorker constructor
> 
> Instead of requiring the dispatcher to send a message to the worker to
> create the display channel and cursor channel, just create them when
> the worker is created."
> 
> ACK in either case.
> 
> 

Changed comment and merged

Frediano

> 
> On Fri, 2015-11-06 at 14:23 +0000, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > 
> > ---
> >  server/red_dispatcher.c | 74 +++++++++++++++------------------------
> > ----------
> >  server/red_dispatcher.h |  4 +--
> >  server/red_worker.c     | 68 +++++++++++++++------------------------
> > ------
> >  server/red_worker.h     |  2 ++
> >  4 files changed, 48 insertions(+), 100 deletions(-)
> > 
> > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > index 43f061d..7855776 100644
> > --- a/server/red_dispatcher.c
> > +++ b/server/red_dispatcher.c
> > @@ -1001,35 +1001,10 @@ void red_dispatcher_async_complete(struct
> > RedDispatcher *dispatcher,
> >      free(async_command);
> >  }
> >  
> > -static RedChannel
> > *red_dispatcher_display_channel_create(RedDispatcher *dispatcher)
> > -{
> > -    RedWorkerMessageDisplayChannelCreate payload;
> > -    RedChannel *display_channel;
> > -
> > -    dispatcher_send_message(&dispatcher->dispatcher,
> > -
> >  RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > -                            &payload);
> > -    receive_data(dispatcher->dispatcher.send_fd, &display_channel,
> > sizeof(RedChannel *));
> > -    return display_channel;
> > -}
> > -
> > -static RedChannel
> > *red_dispatcher_cursor_channel_create(RedDispatcher *dispatcher)
> > -{
> > -    RedWorkerMessageCursorChannelCreate payload;
> > -    RedChannel *cursor_channel;
> > -
> > -    dispatcher_send_message(&dispatcher->dispatcher,
> > -
> >  RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > -                            &payload);
> > -    receive_data(dispatcher->dispatcher.send_fd, &cursor_channel,
> > sizeof(RedChannel *));
> > -    return cursor_channel;
> > -}
> > -
> >  void red_dispatcher_init(QXLInstance *qxl)
> >  {
> >      RedDispatcher *red_dispatcher;
> > -    RedChannel *display_channel;
> > -    RedChannel *cursor_channel;
> > +    RedChannel *channel;
> >      ClientCbs client_cbs = { NULL, };
> >  
> >      spice_return_if_fail(qxl != NULL);
> > @@ -1074,34 +1049,29 @@ void red_dispatcher_init(QXLInstance *qxl)
> >  
> >      // TODO: reference and free
> >      RedWorker *worker = red_worker_new(qxl, red_dispatcher);
> > -    red_worker_run(worker);
> > -
> > -    num_active_workers = 1;
> >  
> > -    display_channel =
> > red_dispatcher_display_channel_create(red_dispatcher);
> > -
> > -    if (display_channel) {
> > -        client_cbs.connect = red_dispatcher_set_display_peer;
> > -        client_cbs.disconnect =
> > red_dispatcher_disconnect_display_peer;
> > -        client_cbs.migrate = red_dispatcher_display_migrate;
> > -        red_channel_register_client_cbs(display_channel,
> > &client_cbs);
> > -        red_channel_set_data(display_channel, red_dispatcher);
> > -        red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > -        red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> > -        red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_STREAM_REPORT);
> > -        reds_register_channel(display_channel);
> > -    }
> > -
> > -    cursor_channel =
> > red_dispatcher_cursor_channel_create(red_dispatcher);
> > +    // TODO: move to their respective channel files
> > +    channel = red_worker_get_cursor_channel(worker);
> > +    client_cbs.connect = red_dispatcher_set_cursor_peer;
> > +    client_cbs.disconnect = red_dispatcher_disconnect_cursor_peer;
> > +    client_cbs.migrate = red_dispatcher_cursor_migrate;
> > +    red_channel_register_client_cbs(channel, &client_cbs);
> > +    red_channel_set_data(channel, red_dispatcher);
> > +    reds_register_channel(channel);
> > +
> > +    channel = red_worker_get_display_channel(worker);
> > +    client_cbs.connect = red_dispatcher_set_display_peer;
> > +    client_cbs.disconnect = red_dispatcher_disconnect_display_peer;
> > +    client_cbs.migrate = red_dispatcher_display_migrate;
> > +    red_channel_register_client_cbs(channel, &client_cbs);
> > +    red_channel_set_data(channel, red_dispatcher);
> > +    red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > +    red_channel_set_cap(channel,
> > SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> > +    red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > +    reds_register_channel(channel);
> >  
> > -    if (cursor_channel) {
> > -        client_cbs.connect = red_dispatcher_set_cursor_peer;
> > -        client_cbs.disconnect =
> > red_dispatcher_disconnect_cursor_peer;
> > -        client_cbs.migrate = red_dispatcher_cursor_migrate;
> > -        red_channel_register_client_cbs(cursor_channel,
> > &client_cbs);
> > -        red_channel_set_data(cursor_channel, red_dispatcher);
> > -        reds_register_channel(cursor_channel);
> > -    }
> > +    red_worker_run(worker);
> > +    num_active_workers = 1;
> >  
> >      qxl->st->dispatcher = red_dispatcher;
> >      red_dispatcher->next = dispatchers;
> > diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> > index 242143a..fde965d 100644
> > --- a/server/red_dispatcher.h
> > +++ b/server/red_dispatcher.h
> > @@ -119,8 +119,8 @@ enum {
> >      /* suspend/windows resolution change command */
> >      RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC,
> >  
> > -    RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > -    RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > +    RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE, /* unused */
> > +    RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE, /* unused */
> >  
> >      RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
> >      RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 0c91b9f..df5be98 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -404,8 +404,6 @@ typedef struct RedWorker {
> >      clockid_t clockid;
> >      QXLInstance *qxl;
> >      RedDispatcher *red_dispatcher;
> > -
> > -    int channel;
> >      int running;
> >      struct pollfd poll_fds[MAX_EVENT_SOURCES];
> >      struct SpiceWatch watches[MAX_EVENT_SOURCES];
> > @@ -9703,23 +9701,6 @@ void
> > handle_dev_create_primary_surface_async(void *opaque, void *payload)
> >      dev_create_primary_surface(worker, msg->surface_id, msg
> > ->surface);
> >  }
> >  
> > -/* exception for Dispatcher, data going from red_worker to main
> > thread,
> > - * TODO: use a different dispatcher?
> > - * TODO: leave direct usage of channel(fd)? It's only used right
> > after the
> > - * pthread is created, since the channel duration is the lifetime of
> > the spice
> > - * server. */
> > -
> > -void handle_dev_display_channel_create(void *opaque, void *payload)
> > -{
> > -    RedWorker *worker = opaque;
> > -
> > -    RedChannel *red_channel;
> > -    // TODO: handle seemless migration. Temp, setting migrate to
> > FALSE
> > -    display_channel_create(worker, FALSE);
> > -    red_channel = &worker->display_channel->common.base;
> > -    send_data(worker->channel, &red_channel, sizeof(RedChannel *));
> > -}
> > -
> >  void handle_dev_display_connect(void *opaque, void *payload)
> >  {
> >      RedWorkerMessageDisplayConnect *msg = payload;
> > @@ -9811,21 +9792,6 @@ static void
> > handle_dev_monitors_config_async(void *opaque, void *payload)
> >  }
> >  
> >  /* TODO: special, perhaps use another dispatcher? */
> > -void handle_dev_cursor_channel_create(void *opaque, void *payload)
> > -{
> > -    RedWorker *worker = opaque;
> > -    RedChannel *red_channel;
> > -
> > -    if (!worker->cursor_channel) {
> > -        worker->cursor_channel = cursor_channel_new(worker);
> > -    } else {
> > -        spice_warning("cursor channel already created");
> > -    }
> > -
> > -    red_channel = RED_CHANNEL(worker->cursor_channel);
> > -    send_data(worker->channel, &red_channel, sizeof(RedChannel *));
> > -}
> > -
> >  void handle_dev_cursor_connect(void *opaque, void *payload)
> >  {
> >      RedWorkerMessageCursorConnect *msg = payload;
> > @@ -10162,16 +10128,6 @@ static void register_callbacks(Dispatcher
> > *dispatcher)
> >                                 
> >  sizeof(RedWorkerMessageSetMouseMode),
> >                                  DISPATCHER_NONE);
> >      dispatcher_register_handler(dispatcher,
> > -
> >  RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > -                                handle_dev_display_channel_create,
> > -
> >  sizeof(RedWorkerMessageDisplayChannelCreate),
> > -                                DISPATCHER_NONE);
> > -    dispatcher_register_handler(dispatcher,
> > -
> >  RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > -                                handle_dev_cursor_channel_create,
> > -
> >  sizeof(RedWorkerMessageCursorChannelCreate),
> > -                                DISPATCHER_NONE);
> > -    dispatcher_register_handler(dispatcher,
> >                                 
> >  RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT,
> >                                  handle_dev_destroy_surface_wait,
> >                                 
> >  sizeof(RedWorkerMessageDestroySurfaceWait),
> > @@ -10236,7 +10192,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> >  
> >      worker->red_dispatcher = red_dispatcher;
> >      worker->qxl = qxl;
> > -    worker->channel = dispatcher_get_recv_fd(dispatcher);
> >      register_callbacks(dispatcher);
> >      if (worker->record_fd) {
> >          dispatcher_register_universal_handler(dispatcher,
> > worker_dispatcher_record);
> > @@ -10265,7 +10220,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> >          worker->poll_fds[i].fd = -1;
> >      }
> >  
> > -    worker->poll_fds[0].fd = worker->channel;
> > +    worker->poll_fds[0].fd = dispatcher_get_recv_fd(dispatcher);
> >      worker->poll_fds[0].events = POLLIN;
> >      worker->watches[0].worker = worker;
> >      worker->watches[0].watch_func = handle_dev_input;
> > @@ -10290,6 +10245,10 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> >      red_init_zlib(worker);
> >      worker->event_timeout = INF_EVENT_WAIT;
> >  
> > +    worker->cursor_channel = cursor_channel_new(worker);
> > +    // TODO: handle seemless migration. Temp, setting migrate to
> > FALSE
> > +    display_channel_create(worker, FALSE);
> > +
> >      return worker;
> >  }
> >  
> > @@ -10316,6 +10275,9 @@ SPICE_GNUC_NORETURN static void
> > *red_worker_main(void *arg)
> >          spice_warning("getcpuclockid failed");
> >      }
> >  
> > +    RED_CHANNEL(worker->cursor_channel)->thread_id = pthread_self();
> > +    RED_CHANNEL(worker->display_channel)->thread_id =
> > pthread_self();
> > +
> >      for (;;) {
> >          int i, num_events;
> >          unsigned int timeout;
> > @@ -10400,3 +10362,17 @@ bool red_worker_run(RedWorker *worker)
> >  
> >      return r == 0;
> >  }
> > +
> > +RedChannel* red_worker_get_cursor_channel(RedWorker *worker)
> > +{
> > +    spice_return_val_if_fail(worker, NULL);
> > +
> > +    return RED_CHANNEL(worker->cursor_channel);
> > +}
> > +
> > +RedChannel* red_worker_get_display_channel(RedWorker *worker)
> > +{
> > +    spice_return_val_if_fail(worker, NULL);
> > +
> > +    return RED_CHANNEL(worker->display_channel);
> > +}
> > diff --git a/server/red_worker.h b/server/red_worker.h
> > index 33dd974..2995b8f 100644
> > --- a/server/red_worker.h
> > +++ b/server/red_worker.h
> > @@ -106,6 +106,8 @@ static inline void red_pipes_add_verb(RedChannel
> > *channel, uint16_t verb)
> >  RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
> > *red_dispatcher);
> >  bool       red_worker_run(RedWorker *worker);
> >  QXLInstance* red_worker_get_qxl(RedWorker *worker);
> > +RedChannel* red_worker_get_cursor_channel(RedWorker *worker);
> > +RedChannel* red_worker_get_display_channel(RedWorker *worker);
> >  
> >  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> >                                     const char *name,
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]