The VNC encoding worker thread was using a single global queue shared
across all VNC displays, with no way to stop it. This made it impossible
to properly clean up resources when a VncDisplay is freed.
Move the VncJobQueue from a file-scoped global to a per-VncDisplay
member, so each display owns its worker thread and queue. Add
vnc_stop_worker_thread() to perform an orderly shutdown: signal the
thread to exit, join it, and destroy the queue. The thread is now
created as QEMU_THREAD_JOINABLE instead of QEMU_THREAD_DETACHED.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
ui/vnc-jobs.h | 3 ++-
ui/vnc.h | 2 ++
ui/vnc-jobs.c | 76 +++++++++++++++++++++++++++++++++--------------------------
ui/vnc.c | 3 ++-
4 files changed, 49 insertions(+), 35 deletions(-)
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index 59f66bcc353..e5ab55c1da6 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -37,7 +37,8 @@ void vnc_job_push(VncJob *job);
void vnc_jobs_join(VncState *vs);
void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+void vnc_start_worker_thread(VncDisplay *vd);
+void vnc_stop_worker_thread(VncDisplay *vd);
/* Locks */
static inline int vnc_trylock_display(VncDisplay *vd)
diff --git a/ui/vnc.h b/ui/vnc.h
index 110c2bd4600..9a09fcdad8b 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -62,6 +62,7 @@
typedef struct VncState VncState;
typedef struct VncJob VncJob;
+typedef struct VncJobQueue VncJobQueue;
typedef struct VncRect VncRect;
typedef struct VncRectEntry VncRectEntry;
@@ -158,6 +159,7 @@ struct VncDisplay
int ledstate;
QKbdState *kbd;
QemuMutex mutex;
+ VncJobQueue *queue;
int cursor_msize;
uint8_t *cursor_mask;
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 5b17ef54091..c809287dd3a 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -29,8 +29,6 @@
#include "qemu/osdep.h"
#include "vnc.h"
#include "vnc-jobs.h"
-#include "qemu/sockets.h"
-#include "qemu/main-loop.h"
#include "trace.h"
/*
@@ -56,17 +54,10 @@ struct VncJobQueue {
QemuCond cond;
QemuMutex mutex;
QemuThread thread;
+ bool exit;
QTAILQ_HEAD(, VncJob) jobs;
};
-typedef struct VncJobQueue VncJobQueue;
-
-/*
- * We use a single global queue, but most of the functions are
- * already reentrant, so we can easily add more than one encoding thread
- */
-static VncJobQueue *queue;
-
static void vnc_lock_queue(VncJobQueue *queue)
{
qemu_mutex_lock(&queue->mutex);
@@ -125,19 +116,22 @@ static void vnc_job_free(VncJob *job)
*/
void vnc_job_push(VncJob *job)
{
+ VncJobQueue *queue = job->vs->vd->queue;
+
assert(!QTAILQ_IN_USE(job, next));
if (QLIST_EMPTY(&job->rectangles)) {
vnc_job_free(job);
} else {
vnc_lock_queue(queue);
+ assert(!queue->exit);
QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
qemu_cond_broadcast(&queue->cond);
vnc_unlock_queue(queue);
}
}
-static bool vnc_has_job_locked(VncState *vs)
+static bool vnc_has_job_locked(VncJobQueue *queue, VncState *vs)
{
VncJob *job;
@@ -151,8 +145,10 @@ static bool vnc_has_job_locked(VncState *vs)
void vnc_jobs_join(VncState *vs)
{
+ VncJobQueue *queue = vs->vd->queue;
+
vnc_lock_queue(queue);
- while (vnc_has_job_locked(vs)) {
+ while (vnc_has_job_locked(queue, vs)) {
qemu_cond_wait(&queue->cond, &queue->mutex);
}
vnc_unlock_queue(queue);
@@ -252,9 +248,13 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
int saved_offset;
vnc_lock_queue(queue);
- while (QTAILQ_EMPTY(&queue->jobs)) {
+ while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
qemu_cond_wait(&queue->cond, &queue->mutex);
}
+ if (queue->exit) {
+ vnc_unlock_queue(queue);
+ return 1;
+ }
job = QTAILQ_FIRST(&queue->jobs);
vnc_unlock_queue(queue);
@@ -340,39 +340,49 @@ disconnected:
return 0;
}
-static VncJobQueue *vnc_queue_init(void)
-{
- VncJobQueue *queue = g_new0(VncJobQueue, 1);
-
- qemu_cond_init(&queue->cond);
- qemu_mutex_init(&queue->mutex);
- QTAILQ_INIT(&queue->jobs);
- return queue;
-}
-
static void *vnc_worker_thread(void *arg)
{
VncJobQueue *queue = arg;
while (!vnc_worker_thread_loop(queue)) ;
- g_assert_not_reached();
+
return NULL;
}
-static bool vnc_worker_thread_running(void)
+void vnc_start_worker_thread(VncDisplay *vd)
{
- return queue; /* Check global queue */
+ VncJobQueue *queue;
+
+ assert(vd->queue == NULL);
+
+ queue = g_new0(VncJobQueue, 1);
+ qemu_cond_init(&queue->cond);
+ qemu_mutex_init(&queue->mutex);
+ QTAILQ_INIT(&queue->jobs);
+ vd->queue = queue;
+
+ qemu_thread_create(&queue->thread, "vnc_worker", vnc_worker_thread, queue,
+ QEMU_THREAD_JOINABLE);
}
-void vnc_start_worker_thread(void)
+void vnc_stop_worker_thread(VncDisplay *vd)
{
- VncJobQueue *q;
+ VncJobQueue *queue = vd->queue;
- if (vnc_worker_thread_running())
+ if (!queue) {
return;
+ }
+
+ /* all VNC clients must have finished before we can stop the worker thread */
+ vnc_lock_queue(queue);
+ assert(QTAILQ_EMPTY(&queue->jobs));
+ queue->exit = true;
+ qemu_cond_broadcast(&queue->cond);
+ vnc_unlock_queue(queue);
- q = vnc_queue_init();
- qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
- QEMU_THREAD_DETACHED);
- queue = q; /* Set global queue */
+ qemu_thread_join(&queue->thread);
+ qemu_cond_destroy(&queue->cond);
+ qemu_mutex_destroy(&queue->mutex);
+ g_free(queue);
+ vd->queue = NULL;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index ba7376360e6..4e5a9ee0341 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3457,7 +3457,7 @@ void vnc_display_init(const char *id, Error **errp)
vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
vd->connections_limit = 32;
- vnc_start_worker_thread();
+ vnc_start_worker_thread(vd);
register_displaychangelistener(&vd->dcl);
vd->kbd = qkbd_state_init(vd->dcl.con);
@@ -3517,6 +3517,7 @@ static void vnc_display_free(VncDisplay *vd)
assert(QTAILQ_EMPTY(&vd->clients));
+ vnc_stop_worker_thread(vd);
vnc_display_close(vd);
unregister_displaychangelistener(&vd->dcl);
qkbd_state_free(vd->kbd);
--
2.53.0