Re-arrange channel client creation to avoid exposing client lock

**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.

4 files changed, 32 insertions, 61 deletions

diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
index b7fee6fa..c937c87d 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);
 
 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 c562e8ed..195ee3ef 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;
-}
-
 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);
+    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);
         }
@@ -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 3b14fbfe..eb63b07f 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 89aab8d5..1b46c01a 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);