Re: [PATCH v2 3/4] channel: add interface parameters to SpiceCoreInterfaceInternal

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

 



On Thu, Jan 07, 2016 at 05:32:16PM +0000, Frediano Ziglio wrote:
> This patch and previous ones want to solve the problem of not having a
> context in SpiceCoreInterface. SpiceCoreInterface defines a set of
> callbacks to handle events in spice-server. These callbacks allow to
> handle timers, watch for file descriptors and send channel events.
> All these callbacks does not accept a context (usually in C passed as a

do not accept

> void* parameter) so they is hard for them to differentiate the interface
> specified.

it is hard for them

> Unfortunately this structure is used even internally from different
> contextes for instance every RedWorker thread have a different context. To

contexts, for instance every RedWorker thread has a different

> solve this issue some workarounds are used. Currently for timers a variable
> depending on the current thread is used while for watches the opaque
> parameter to pass to the event callback is used as it currently points just
> to RedChannelClient structure.  This however impose some implicit

imposes

> maintanance problem in the future. What happen for instance if for some

maintainance ... What happens

> reason a timer is registered during worker initialization, run in another
> thread? What if we decide to register a file descriptor callback for
> something not a RedChannelClient?  Could be that the program will run
> without any issue till some bytes changes and weird thing could happen.

some bytes change and weird things could happen

> 
> The implementation of this solution is done implementing an internal "core"
> interface that have context specification and use it to differentiate the

that has context specific


> context instead of relying to some other, hard to maintain, detail. Then an

relying on

> adapter structure (name inpired to the adapter pattern) will provide the
> internal core interface using the external, public, definition (in the
> future this technique can be used to extend the external interface without
> breaking the ABI).

Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>

Christophe
> 
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/char-device.c       |  4 ++--
>  server/inputs-channel.c    |  2 +-
>  server/main-channel.c      |  2 +-
>  server/main-dispatcher.c   |  2 +-
>  server/red-channel.c       | 12 +++++++-----
>  server/red-common.h        |  4 ++--
>  server/red-worker.c        |  3 ++-
>  server/reds-stream.c       |  2 +-
>  server/reds.c              | 17 +++++++++--------
>  server/sound.c             |  2 +-
>  server/spice_timer_queue.c |  3 ++-
>  server/spice_timer_queue.h |  3 ++-
>  12 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index cefc14d..19e8419 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -694,7 +694,7 @@ SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance *si
>      sif = SPICE_CONTAINEROF(char_dev->sin->base.sif, SpiceCharDeviceInterface, base);
>      if (sif->base.minor_version <= 2 ||
>          !(sif->flags & SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
> -        char_dev->write_to_dev_timer = core->timer_add(spice_char_dev_write_retry, char_dev);
> +        char_dev->write_to_dev_timer = core->timer_add(core, spice_char_dev_write_retry, char_dev);
>          if (!char_dev->write_to_dev_timer) {
>              spice_error("failed creating char dev write timer");
>          }
> @@ -793,7 +793,7 @@ int spice_char_device_client_add(SpiceCharDeviceState *dev,
>      dev_client->max_send_queue_size = max_send_queue_size;
>      dev_client->do_flow_control = do_flow_control;
>      if (do_flow_control) {
> -        dev_client->wait_for_tokens_timer = core->timer_add(device_client_wait_for_tokens_timeout,
> +        dev_client->wait_for_tokens_timer = core->timer_add(core, device_client_wait_for_tokens_timeout,
>                                                              dev_client);
>          if (!dev_client->wait_for_tokens_timer) {
>              spice_error("failed to create wait for tokens timer");
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index ba97f0f..c21cab8 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -674,7 +674,7 @@ void inputs_init(void)
>      red_channel_set_cap(&g_inputs_channel->base, SPICE_INPUTS_CAP_KEY_SCANCODE);
>      reds_register_channel(&g_inputs_channel->base);
>  
> -    if (!(key_modifiers_timer = core->timer_add(key_modifiers_sender, NULL))) {
> +    if (!(key_modifiers_timer = core->timer_add(core, key_modifiers_sender, NULL))) {
>          spice_error("key modifiers timer create failed");
>      }
>  }
> diff --git a/server/main-channel.c b/server/main-channel.c
> index eb894b0..2a9f0ce 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -1087,7 +1087,7 @@ static MainChannelClient *main_channel_client_create(MainChannel *main_chan, Red
>      mcc->connection_id = connection_id;
>      mcc->bitrate_per_sec = ~0;
>  #ifdef RED_STATISTICS
> -    if (!(mcc->ping_timer = core->timer_add(ping_timer_cb, NULL))) {
> +    if (!(mcc->ping_timer = core->timer_add(core, ping_timer_cb, NULL))) {
>          spice_error("ping timer create failed");
>      }
>      mcc->ping_interval = PING_INTERVAL;
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index 77f2071..2cb53ef 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -200,7 +200,7 @@ void main_dispatcher_init(SpiceCoreInterfaceInternal *core)
>      memset(&main_dispatcher, 0, sizeof(main_dispatcher));
>      main_dispatcher.core = core;
>      dispatcher_init(&main_dispatcher.base, MAIN_DISPATCHER_NUM_MESSAGES, &main_dispatcher.base);
> -    core->watch_add(main_dispatcher.base.recv_fd, SPICE_WATCH_EVENT_READ,
> +    core->watch_add(core, main_dispatcher.base.recv_fd, SPICE_WATCH_EVENT_READ,
>                      dispatcher_handle_read, &main_dispatcher.base);
>      dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_CHANNEL_EVENT,
>                                  main_dispatcher_handle_channel_event,
> diff --git a/server/red-channel.c b/server/red-channel.c
> index f79605a..704f319 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -832,7 +832,7 @@ void red_channel_client_start_connectivity_monitoring(RedChannelClient *rcc, uin
>       */
>      if (rcc->latency_monitor.timer == NULL) {
>          rcc->latency_monitor.timer = rcc->channel->core->timer_add(
> -            red_channel_client_ping_timer, rcc);
> +            rcc->channel->core, red_channel_client_ping_timer, rcc);
>          if (!rcc->client->during_target_migrate) {
>              red_channel_client_start_ping_timer(rcc, PING_TEST_IDLE_NET_TIMEOUT_MS);
>          }
> @@ -841,7 +841,7 @@ void red_channel_client_start_connectivity_monitoring(RedChannelClient *rcc, uin
>      if (rcc->connectivity_monitor.timer == NULL) {
>          rcc->connectivity_monitor.state = CONNECTIVITY_STATE_CONNECTED;
>          rcc->connectivity_monitor.timer = rcc->channel->core->timer_add(
> -            red_channel_client_connectivity_timer, rcc);
> +            rcc->channel->core, red_channel_client_connectivity_timer, rcc);
>          rcc->connectivity_monitor.timeout = timeout_ms;
>          if (!rcc->client->during_target_migrate) {
>             rcc->channel->core->timer_start(rcc->connectivity_monitor.timer,
> @@ -906,7 +906,8 @@ RedChannelClient *red_channel_client_create(int size, RedChannel *channel, RedCl
>      ring_init(&rcc->pipe);
>      rcc->pipe_size = 0;
>  
> -    stream->watch = channel->core->watch_add(stream->socket,
> +    stream->watch = channel->core->watch_add(channel->core,
> +                                           stream->socket,
>                                             SPICE_WATCH_EVENT_READ,
>                                             red_channel_client_event, rcc);
>      rcc->id = channel->clients_num;
> @@ -917,7 +918,7 @@ RedChannelClient *red_channel_client_create(int size, RedChannel *channel, RedCl
>  
>      if (monitor_latency && reds_stream_get_family(stream) != AF_UNIX) {
>          rcc->latency_monitor.timer = channel->core->timer_add(
> -            red_channel_client_ping_timer, rcc);
> +            channel->core, red_channel_client_ping_timer, rcc);
>          if (!client->during_target_migrate) {
>              red_channel_client_start_ping_timer(rcc, PING_TEST_IDLE_NET_TIMEOUT_MS);
>          }
> @@ -1070,7 +1071,8 @@ static void dummy_watch_update_mask(SpiceWatch *watch, int event_mask)
>  {
>  }
>  
> -static SpiceWatch *dummy_watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +static SpiceWatch *dummy_watch_add(const SpiceCoreInterfaceInternal *iface,
> +                                   int fd, int event_mask, SpiceWatchFunc func, void *opaque)
>  {
>      return NULL; // apparently allowed?
>  }
> diff --git a/server/red-common.h b/server/red-common.h
> index 3c54fb8..253dc45 100644
> --- a/server/red-common.h
> +++ b/server/red-common.h
> @@ -42,12 +42,12 @@
>  typedef struct SpiceCoreInterfaceInternal SpiceCoreInterfaceInternal;
>  
>  struct SpiceCoreInterfaceInternal {
> -    SpiceTimer *(*timer_add)(SpiceTimerFunc func, void *opaque);
> +    SpiceTimer *(*timer_add)(const SpiceCoreInterfaceInternal *iface, SpiceTimerFunc func, void *opaque);
>      void (*timer_start)(SpiceTimer *timer, uint32_t ms);
>      void (*timer_cancel)(SpiceTimer *timer);
>      void (*timer_remove)(SpiceTimer *timer);
>  
> -    SpiceWatch *(*watch_add)(int fd, int event_mask, SpiceWatchFunc func, void *opaque);
> +    SpiceWatch *(*watch_add)(const SpiceCoreInterfaceInternal *iface, int fd, int event_mask, SpiceWatchFunc func, void *opaque);
>      void (*watch_update_mask)(SpiceWatch *watch, int event_mask);
>      void (*watch_remove)(SpiceWatch *watch);
>  
> diff --git a/server/red-worker.c b/server/red-worker.c
> index ce9cc56..b0b7df1 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -531,7 +531,8 @@ static void worker_watch_update_mask(SpiceWatch *watch, int event_mask)
>      }
>  }
>  
> -static SpiceWatch *worker_watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +static SpiceWatch *worker_watch_add(const SpiceCoreInterfaceInternal *iface,
> +                                    int fd, int event_mask, SpiceWatchFunc func, void *opaque)
>  {
>      /* Since we are a channel core implementation, we always get called from
>         red_channel_client_create(), so opaque always is our rcc */
> diff --git a/server/reds-stream.c b/server/reds-stream.c
> index d8e4fe4..edc25f7 100644
> --- a/server/reds-stream.c
> +++ b/server/reds-stream.c
> @@ -457,7 +457,7 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
>                  switch (errno) {
>                  case EAGAIN:
>                      if (!async->stream->watch) {
> -                        async->stream->watch = core->watch_add(async->stream->socket,
> +                        async->stream->watch = core->watch_add(core, async->stream->socket,
>                                                                 SPICE_WATCH_EVENT_READ,
>                                                                 async_read_handler, async);
>                      }
> diff --git a/server/reds.c b/server/reds.c
> index d9d0e0a..b992911 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -76,7 +76,7 @@ SpiceCoreInterfaceInternal *core = NULL;
>  
>  static SpiceCoreInterface *core_public = NULL;
>  
> -static SpiceTimer *adapter_timer_add(SpiceTimerFunc func, void *opaque)
> +static SpiceTimer *adapter_timer_add(const SpiceCoreInterfaceInternal *iface, SpiceTimerFunc func, void *opaque)
>  {
>      return core_public->timer_add(func, opaque);
>  }
> @@ -96,7 +96,8 @@ static void adapter_timer_remove(SpiceTimer *timer)
>      core_public->timer_remove(timer);
>  }
>  
> -static SpiceWatch *adapter_watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +static SpiceWatch *adapter_watch_add(const SpiceCoreInterfaceInternal *iface,
> +                                     int fd, int event_mask, SpiceWatchFunc func, void *opaque)
>  {
>      return core_public->watch_add(fd, event_mask, func, opaque);
>  }
> @@ -2358,11 +2359,11 @@ static RedLinkInfo *reds_init_client_ssl_connection(int socket)
>          case REDS_STREAM_SSL_STATUS_ERROR:
>              goto error;
>          case REDS_STREAM_SSL_STATUS_WAIT_FOR_READ:
> -            link->stream->watch = core->watch_add(link->stream->socket, SPICE_WATCH_EVENT_READ,
> +            link->stream->watch = core->watch_add(core, link->stream->socket, SPICE_WATCH_EVENT_READ,
>                                              reds_handle_ssl_accept, link);
>              break;
>          case REDS_STREAM_SSL_STATUS_WAIT_FOR_WRITE:
> -            link->stream->watch = core->watch_add(link->stream->socket, SPICE_WATCH_EVENT_WRITE,
> +            link->stream->watch = core->watch_add(core, link->stream->socket, SPICE_WATCH_EVENT_WRITE,
>                                                    reds_handle_ssl_accept, link);
>              break;
>      }
> @@ -2555,7 +2556,7 @@ static int reds_init_net(void)
>          if (-1 == reds->listen_socket) {
>              return -1;
>          }
> -        reds->listen_watch = core->watch_add(reds->listen_socket,
> +        reds->listen_watch = core->watch_add(core, reds->listen_socket,
>                                               SPICE_WATCH_EVENT_READ,
>                                               reds_accept, NULL);
>          if (reds->listen_watch == NULL) {
> @@ -2570,7 +2571,7 @@ static int reds_init_net(void)
>          if (-1 == reds->secure_listen_socket) {
>              return -1;
>          }
> -        reds->secure_listen_watch = core->watch_add(reds->secure_listen_socket,
> +        reds->secure_listen_watch = core->watch_add(core, reds->secure_listen_socket,
>                                                      SPICE_WATCH_EVENT_READ,
>                                                      reds_accept_ssl_connection, NULL);
>          if (reds->secure_listen_watch == NULL) {
> @@ -2581,7 +2582,7 @@ static int reds_init_net(void)
>  
>      if (spice_listen_socket_fd != -1 ) {
>          reds->listen_socket = spice_listen_socket_fd;
> -        reds->listen_watch = core->watch_add(reds->listen_socket,
> +        reds->listen_watch = core->watch_add(core, reds->listen_socket,
>                                               SPICE_WATCH_EVENT_READ,
>                                               reds_accept, NULL);
>          if (reds->listen_watch == NULL) {
> @@ -3343,7 +3344,7 @@ static int do_spice_init(SpiceCoreInterface *core_interface)
>      ring_init(&reds->mig_wait_disconnect_clients);
>      reds->vm_running = TRUE; /* for backward compatibility */
>  
> -    if (!(reds->mig_timer = core->timer_add(migrate_timeout, NULL))) {
> +    if (!(reds->mig_timer = core->timer_add(core, migrate_timeout, NULL))) {
>          spice_error("migration timer create failed");
>      }
>  
> diff --git a/server/sound.c b/server/sound.c
> index a8cf0b6..adb27ca 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -947,7 +947,7 @@ static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t channel_i
>      channel->receive_data.end = channel->receive_data.buf + sizeof(channel->receive_data.buf);
>      channel->send_data.marshaller = spice_marshaller_new();
>  
> -    stream->watch = core->watch_add(stream->socket, SPICE_WATCH_EVENT_READ,
> +    stream->watch = core->watch_add(core, stream->socket, SPICE_WATCH_EVENT_READ,
>                                    snd_event, channel);
>      if (stream->watch == NULL) {
>          spice_printerr("watch_add failed, %s", strerror(errno));
> diff --git a/server/spice_timer_queue.c b/server/spice_timer_queue.c
> index 12ac131..421b090 100644
> --- a/server/spice_timer_queue.c
> +++ b/server/spice_timer_queue.c
> @@ -129,7 +129,8 @@ void spice_timer_queue_destroy(void)
>      pthread_mutex_unlock(&queue_list_lock);
>  }
>  
> -SpiceTimer *spice_timer_queue_add(SpiceTimerFunc func, void *opaque)
> +SpiceTimer *spice_timer_queue_add(const SpiceCoreInterfaceInternal *iface,
> +                                  SpiceTimerFunc func, void *opaque)
>  {
>      SpiceTimer *timer = spice_new0(SpiceTimer, 1);
>      SpiceTimerQueue *queue = spice_timer_queue_find_with_lock();
> diff --git a/server/spice_timer_queue.h b/server/spice_timer_queue.h
> index a84f6cd..b17cecf 100644
> --- a/server/spice_timer_queue.h
> +++ b/server/spice_timer_queue.h
> @@ -29,7 +29,8 @@ typedef struct SpiceTimerQueue SpiceTimerQueue;
>  int spice_timer_queue_create(void);
>  void spice_timer_queue_destroy(void);
>  
> -SpiceTimer *spice_timer_queue_add(SpiceTimerFunc func, void *opaque);
> +SpiceTimer *spice_timer_queue_add(const SpiceCoreInterfaceInternal *iface,
> +                                  SpiceTimerFunc func, void *opaque);
>  void spice_timer_set(SpiceTimer *timer, uint32_t ms);
>  void spice_timer_cancel(SpiceTimer *timer);
>  void spice_timer_remove(SpiceTimer *timer);
> -- 
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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]