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