[Spice-devel] [PATCH spice-server] red_channel: replace an assert upon threads mismatch with a warning

[Yonit Halperin]

The assert:
spice_assert(pthread_equal(pthread_self(), client->thread_id))
and the assert:
spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id))
were coded in order to protect data that is accessed from the main
context (red_client and most of the channels), from
access by threads of other channels (namely, the display and cursor
channels), and vice versa.
However, some of the calls to the sound channel interface,
and also the char_device interface, can be done from the vcpu thread.
It doesn't endanger these channels internal data, since qemu use global
mutex for the vcpu and io threads.
Thus, pthread_self() can be !=  channel->thread_id, if one of them is
the vcpu thread and the other is the io-thread, and we shouldn't assert.

Future plans: A more complete and complicated solution would be to manage our own thread for
spice-channels, and push input from qemu to this thread, instead of
counting on the global mutex of qemu

rhbz#823472
---
 server/red_channel.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/server/red_channel.c b/server/red_channel.c
index 119e5e5..5662041 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -938,6 +938,8 @@ RedChannel *red_channel_create(int size,
 
     channel->out_bytes_counter = 0;
 
+    spice_debug("channel type %d id %d thread_id 0x%lx",
+                channel->type, channel->id, channel->thread_id);
     return channel;
 }
 
@@ -982,6 +984,8 @@ RedChannel *red_channel_create_dummy(int size, uint32_t type, uint32_t id)
     red_channel_set_common_cap(channel, SPICE_COMMON_CAP_MINI_HEADER);
 
     channel->thread_id = pthread_self();
+    spice_debug("channel type %d id %d thread_id 0x%lx",
+                channel->type, channel->id, channel->thread_id);
 
     channel->out_bytes_counter = 0;
 
@@ -1657,7 +1661,14 @@ void red_channel_client_ack_set_client_window(RedChannelClient *rcc, int client_
 
 static void red_channel_remove_client(RedChannelClient *rcc)
 {
-    spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id));
+    if (!pthread_equal(pthread_self(), rcc->channel->thread_id)) {
+        spice_warning("channel type %d id %d - "
+                      "channel->thread_id (0x%lx) != pthread_self (0x%lx)."
+                      "If one of the threads is != io-thread && != vcpu-thread, "
+                      "this might be a BUG",
+                      rcc->channel->type, rcc->channel->id,
+                      rcc->channel->thread_id, pthread_self());
+    }
     ring_remove(&rcc->channel_link);
     spice_assert(rcc->channel->clients_num > 0);
     rcc->channel->clients_num--;
@@ -1937,7 +1948,12 @@ void red_client_migrate(RedClient *client)
     RedChannelClient *rcc;
 
     spice_printerr("migrate client with #channels %d", client->channels_num);
-    spice_assert(pthread_equal(pthread_self(), client->thread_id));
+    if (!pthread_equal(pthread_self(), client->thread_id)) {
+        spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
+                      "If one of the threads is != io-thread && != vcpu-thread,"
+                      " this might be a BUG",
+                      client->thread_id, pthread_self());
+    }
     RING_FOREACH_SAFE(link, next, &client->channels) {
         rcc = SPICE_CONTAINEROF(link, RedChannelClient, client_link);
         if (red_channel_client_is_connected(rcc)) {
@@ -1952,7 +1968,13 @@ void red_client_destroy(RedClient *client)
     RedChannelClient *rcc;
 
     spice_printerr("destroy client with #channels %d", client->channels_num);
-    spice_assert(pthread_equal(pthread_self(), client->thread_id));
+    if (!pthread_equal(pthread_self(), client->thread_id)) {
+        spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
+                      "If one of the threads is != io-thread && != vcpu-thread,"
+                      " this might be a BUG",
+                      client->thread_id,
+                      pthread_self());
+    }
     RING_FOREACH_SAFE(link, next, &client->channels) {
         // some channels may be in other threads, so disconnection
         // is not synchronous.
-- 
1.8.1.4