> **FIXME: introduces crash when session vdagent connects
>
> Instead of requiring the channel client to lock the client's lock,
> re-arrange things so that all of the locking can be internal to
> RedClient methods. So instead of having a pre-create validate function,
> we simply move the check to red_client_add_channel() and return an error
> if a channel already exists. This encapsulates the client implementation
> better and also reduces code duplication in RedChannelClient and
> DummyChannelClient.
> ---
>
>
> **NOTE**: The "FIXME" comment above was added at some point during the branch
> development while bisecting a crash. I don't remember any additional details
> other than what is stated in the commit log above, and so I don't know
> exactly
> what scenario triggered the crash. I can no longer reproduce this bug, but
> decided to leave the comment in for now in the hopes that it might prompt
> others to do a bit more extensive testing to see if you can reproduce the
> issue.
>
>
> server/dummy-channel-client.c | 33 ++++-----------------------------
> server/red-channel-client.c | 32 +++-----------------------------
> server/red-channel.c | 26 ++++++++++++++++++++++++--
> server/red-channel.h | 2 +-
> 4 files changed, 32 insertions(+), 61 deletions(-)
>
> diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
> index b7fee6f..c937c87 100644
> --- a/server/dummy-channel-client.c
> +++ b/server/dummy-channel-client.c
> @@ -35,48 +35,23 @@ struct DummyChannelClientPrivate
> gboolean connected;
> };
>
> -static int dummy_channel_client_pre_create_validate(RedChannel *channel,
> RedClient *client)
> -{
> - uint32_t type, id;
> - g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> - if (red_client_get_channel(client, type, id)) {
> - spice_printerr("Error client %p: duplicate channel type %d id %d",
> - client, type, id);
> - return FALSE;
> - }
> - return TRUE;
> -}
> -
> static gboolean dummy_channel_client_initable_init(GInitable *initable,
> GCancellable
> *cancellable,
> GError **error)
> {
> GError *local_error = NULL;
> - DummyChannelClient *self = DUMMY_CHANNEL_CLIENT(initable);
> - RedChannelClient *rcc = RED_CHANNEL_CLIENT(self);
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(initable);
> RedClient *client = red_channel_client_get_client(rcc);
> RedChannel *channel = red_channel_client_get_channel(rcc);
> - uint32_t type, id;
> -
> - g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> - pthread_mutex_lock(&client->lock);
> - if (!dummy_channel_client_pre_create_validate(channel,
> - client)) {
> - g_set_error(&local_error,
> - SPICE_SERVER_ERROR,
> - SPICE_SERVER_ERROR_FAILED,
> - "Client %p: duplicate channel type %d id %d",
> - client, type, id);
> - goto cleanup;
> - }
>
> rcc->incoming.header.data = rcc->incoming.header_buf;
>
> + if (!red_client_add_channel(client, rcc, &local_error))
> + goto cleanup;
> +
> red_channel_add_client(channel, rcc);
> - red_client_add_channel(client, rcc);
>
I honestly wouldn't change red_client_add_channel/red_channel_add_client order.
In theory calls to RedChannelClient from another thread can came from RedClient
as soon as red_client_add_channel returns.
> cleanup:
> - pthread_mutex_unlock(&client->lock);
> if (local_error) {
> g_warning("Failed to create channel client: %s",
> local_error->message);
> g_propagate_error(error, local_error);
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index c562e8e..195ee3e 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -851,18 +851,6 @@ static const SpiceDataHeaderOpaque mini_header_wrapper =
> {NULL, sizeof(SpiceMini
> mini_header_get_msg_type,
> mini_header_get_msg_size};
>
> -static int red_channel_client_pre_create_validate(RedChannel *channel,
> RedClient *client)
> -{
> - uint32_t type, id;
> - g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> - if (red_client_get_channel(client, type, id)) {
> - spice_printerr("Error client %p: duplicate channel type %d id %d",
> - client, type, id);
> - return FALSE;
> - }
> - return TRUE;
> -}
> -
Looks like a duplication. Definitively I should work on removing this
Dummy* virus..
> static gboolean red_channel_client_initable_init(GInitable *initable,
> GCancellable *cancellable,
> GError **error)
> @@ -870,20 +858,9 @@ static gboolean
> red_channel_client_initable_init(GInitable *initable,
> GError *local_error = NULL;
> SpiceCoreInterfaceInternal *core;
> RedChannelClient *self = RED_CHANNEL_CLIENT(initable);
> - pthread_mutex_lock(&self->priv->client->lock);
> - if (!red_channel_client_pre_create_validate(self->priv->channel,
> self->priv->client)) {
> - uint32_t id, type;
> - g_object_get(self->priv->channel,
> - "channel-type", &type,
> - "id", &id,
> - NULL);
> - g_set_error(&local_error,
> - SPICE_SERVER_ERROR,
> - SPICE_SERVER_ERROR_FAILED,
> - "Client %p: duplicate channel type %d id %d",
> - self->priv->client, type, id);
> + self->priv->id = red_channel_get_n_clients(self->priv->channel);
OT: Nobody even point out this is a bug...
Assume 2 clients connect they will have IDs 0, 1 and 2 respectively.
Now assume 0 disconnect and a new one connect, this last will receive
2 as ID so at the end there will be 3 clients with IDs 1, 2 and 2 (again!).
> + if (!red_client_add_channel(self->priv->client, self, &local_error))
> goto cleanup;
> - }
>
> core = red_channel_get_core_interface(self->priv->channel);
> if (self->priv->monitor_latency
> @@ -891,7 +868,7 @@ static gboolean
> red_channel_client_initable_init(GInitable *initable,
> self->priv->latency_monitor.timer =
> core->timer_add(core, red_channel_client_ping_timer, self);
>
> - if (!self->priv->client->during_target_migrate) {
> + if (!red_client_during_migrate_at_target(self->priv->client)) {
> red_channel_client_start_ping_timer(self,
> PING_TEST_IDLE_NET_TIMEOUT_MS);
> }
Looking at original code looks like the initialization sequence is a bit
different. Not much of a problem as RedChannel and their RedChannelClients are
executed in the same thread.
However digging more into the code looks like we should change/revert a bit,
there could be some cases leading to uninitialized fields.
> @@ -914,9 +891,7 @@ static gboolean
> red_channel_client_initable_init(GInitable *initable,
> SPICE_WATCH_EVENT_READ,
> red_channel_client_event,
> self);
> - self->priv->id = red_channel_get_n_clients(self->priv->channel);
> red_channel_add_client(self->priv->channel, self);
> - red_client_add_channel(self->priv->client, self);
>
> if (!red_channel_config_socket(self->priv->channel, self)) {
> g_set_error_literal(&local_error,
> @@ -926,7 +901,6 @@ static gboolean
> red_channel_client_initable_init(GInitable *initable,
> }
>
> cleanup:
> - pthread_mutex_unlock(&self->priv->client->lock);
> if (local_error) {
> g_warning("Failed to create channel client: %s",
> local_error->message);
> g_propagate_error(error, local_error);
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 3b14fbf..eb63b07 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -796,15 +796,37 @@ RedChannelClient *red_client_get_channel(RedClient
> *client, int type, int id)
> return ret;
> }
>
> -/* client->lock should be locked */
> -void red_client_add_channel(RedClient *client, RedChannelClient *rcc)
> +gboolean red_client_add_channel(RedClient *client, RedChannelClient *rcc,
> GError **error)
> {
> + uint32_t type, id;
> + RedChannel *channel;
> + gboolean result = TRUE;
> +
> spice_assert(rcc && client);
> + channel = red_channel_client_get_channel(rcc);
> +
> + pthread_mutex_lock(&client->lock);
> +
> + g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> + if (red_client_get_channel(client, type, id)) {
> + g_set_error(error,
> + SPICE_SERVER_ERROR,
> + SPICE_SERVER_ERROR_FAILED,
> + "Client %p: duplicate channel type %d id %d",
> + client, type, id);
> + result = FALSE;
> + goto cleanup;
> + }
> +
> client->channels = g_list_prepend(client->channels, rcc);
> if (client->during_target_migrate && client->seamless_migrate) {
> if (red_channel_client_set_migration_seamless(rcc))
> client->num_migrated_channels++;
> }
> +
> +cleanup:
> + pthread_mutex_unlock(&client->lock);
> + return result;
> }
>
> MainChannelClient *red_client_get_main(RedClient *client) {
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 89aab8d..1b46c01 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -365,7 +365,7 @@ RedClient *red_client_ref(RedClient *client);
> RedClient *red_client_unref(RedClient *client);
>
> /* client->lock should be locked */
> -void red_client_add_channel(RedClient *client, RedChannelClient *rcc);
> +gboolean red_client_add_channel(RedClient *client, RedChannelClient *rcc,
> GError **error);
> void red_client_remove_channel(RedChannelClient *rcc);
> RedChannelClient *red_client_get_channel(RedClient *client, int type, int
> id);
>
I'll post some order rollback and try to update this patch too to have
RedClient addition as last. Also I'm investigating of a possible
regression in this code leading to some leaks (not sure this patch or
a previous one... or no one).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel