[PATCH spice-server v2 05/12] Re-arrange channel client creation to avoid exposing client lock

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

 



From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>

**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.
---
 server/dummy-channel-client.c | 29 ++---------------------------
 server/red-channel-client.c   | 30 ++----------------------------
 server/red-channel.c          | 26 ++++++++++++++++++++++++--
 server/red-channel.h          |  2 +-
 4 files changed, 29 insertions(+), 58 deletions(-)

diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
index a242d51..e43546c 100644
--- a/server/dummy-channel-client.c
+++ b/server/dummy-channel-client.c
@@ -35,46 +35,21 @@ 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);
+    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 a28f5e6..8213f13 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -862,18 +862,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)
@@ -881,20 +869,8 @@ 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);
+    if (!red_client_add_channel(self->priv->client, self, &local_error))
         goto cleanup;
-    }
 
     if (!red_channel_config_socket(self->priv->channel, self)) {
         g_set_error_literal(&local_error,
@@ -917,7 +893,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);
         }
@@ -925,10 +901,8 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
     }
 
     red_channel_add_client(self->priv->channel, self);
-    red_client_add_channel(self->priv->client, self);
 
 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 8f9ae6c..07592db 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 c136355..6772b88 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);
 
-- 
2.7.4

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://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]