[Spice-devel] [PATCH 1/4] Re-arrange channel client creation to avoid exposing client lock
Jonathon Jongsma
jjongsma at redhat.com
Wed Oct 26 19:47:33 UTC 2016
**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);
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;
-}
-
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 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);
--
2.7.4
More information about the Spice-devel
mailing list