[Spice-devel] [PATCH spice-server v2 05/12] Re-arrange channel client creation to avoid exposing client lock
Frediano Ziglio
fziglio at redhat.com
Tue Nov 1 11:07:59 UTC 2016
From: Jonathon Jongsma <jjongsma at redhat.com>
**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
More information about the Spice-devel
mailing list