[libvirt PATCH] src: ensure GSource background unref happens in correct event loop

Daniel P. Berrangé posted 1 patch 3 years, 1 month ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210316163032.148029-1-berrange@redhat.com
meson.build                            |  3 ---
src/libvirt_glib_crash_workaround.syms | 11 -----------
src/libvirt_private.syms               |  1 +
src/meson.build                        |  7 -------
src/qemu/qemu_agent.c                  |  2 +-
src/qemu/qemu_monitor.c                |  2 +-
src/util/glibcompat.c                  | 20 ++++++++++++++++++--
src/util/glibcompat.h                  | 11 +----------
src/util/vireventglib.c                | 12 ++++++------
9 files changed, 28 insertions(+), 41 deletions(-)
delete mode 100644 src/libvirt_glib_crash_workaround.syms
[libvirt PATCH] src: ensure GSource background unref happens in correct event loop
Posted by Daniel P. Berrangé 3 years, 1 month ago
The g_idle_add function	adds a callback	to the primary GMainContext.

To workaround the GSource unref	bugs, we need to add our callbacks
to the GMainContext that is associated with the	GSource	being
unref'd. Thus code using the per-VM virEventThread must use its
private	GMainContext.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build                            |  3 ---
 src/libvirt_glib_crash_workaround.syms | 11 -----------
 src/libvirt_private.syms               |  1 +
 src/meson.build                        |  7 -------
 src/qemu/qemu_agent.c                  |  2 +-
 src/qemu/qemu_monitor.c                |  2 +-
 src/util/glibcompat.c                  | 20 ++++++++++++++++++--
 src/util/glibcompat.h                  | 11 +----------
 src/util/vireventglib.c                | 12 ++++++------
 9 files changed, 28 insertions(+), 41 deletions(-)
 delete mode 100644 src/libvirt_glib_crash_workaround.syms

diff --git a/meson.build b/meson.build
index 69a7b4c88e..ea93a2a8ec 100644
--- a/meson.build
+++ b/meson.build
@@ -1049,9 +1049,6 @@ if host_machine.system() == 'windows'
 else
   gio_dep = dependency('gio-unix-2.0', version: '>=' + glib_version)
 endif
-# GLib event loop race workaround in glibcompat.h, remove when minimum required
-# glib is >= 2.64.0
-glib_crash_workaround = glib_dep.version().version_compare('<2.64.0')
 glib_dep = declare_dependency(
   dependencies: [ glib_dep, gobject_dep, gio_dep ],
 )
diff --git a/src/libvirt_glib_crash_workaround.syms b/src/libvirt_glib_crash_workaround.syms
deleted file mode 100644
index 249058b65b..0000000000
--- a/src/libvirt_glib_crash_workaround.syms
+++ /dev/null
@@ -1,11 +0,0 @@
-#
-# Private symbols specific for pre-2.64.0 GLib workaround
-#
-
-# util/glibcompat.h
-virEventGLibSourceUnrefIdle;
-
-# Let emacs know we want case-insensitive sorting
-# Local Variables:
-# sort-fold-case: t
-# End:
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 767d665613..526dcee11a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1733,6 +1733,7 @@ virStorageSourceUpdatePhysicalSize;
 # util/glibcompat.h
 vir_g_canonicalize_filename;
 vir_g_fsync;
+vir_g_source_unref;
 vir_g_strdup_printf;
 vir_g_strdup_vprintf;
 
diff --git a/src/meson.build b/src/meson.build
index 70a5a83eea..c7ff9e978c 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -124,13 +124,6 @@ else
   sym_files += 'libvirt_libssh2.syms'
 endif
 
-if glib_crash_workaround
-  used_sym_files += 'libvirt_glib_crash_workaround.syms'
-else
-  sym_files += 'libvirt_glib_crash_workaround.syms'
-endif
-
-
 # variables filled by subdirectories
 
 libvirt_libs = []
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 01ba2c78c1..787a7bb41c 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -533,7 +533,7 @@ qemuAgentUnregister(qemuAgentPtr agent)
 {
     if (agent->watch) {
         g_source_destroy(agent->watch);
-        g_vir_source_unref_safe(agent->watch);
+        vir_g_source_unref(agent->watch, agent->context);
         agent->watch = NULL;
     }
 }
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b4f2641504..f6cd9d9eda 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -862,7 +862,7 @@ qemuMonitorUnregister(qemuMonitorPtr mon)
 {
     if (mon->watch) {
         g_source_destroy(mon->watch);
-        g_vir_source_unref_safe(mon->watch);
+        vir_g_source_unref(mon->watch, mon->context);
         mon->watch = NULL;
     }
 }
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c
index fe19ffa87b..eb6dcc0111 100644
--- a/src/util/glibcompat.c
+++ b/src/util/glibcompat.c
@@ -231,9 +231,14 @@ vir_g_strdup_vprintf(const char *msg, va_list args)
  *
  * Drop when min glib >= 2.64.0
  */
-#if GLIB_CHECK_VERSION(2, 64, 0) != TRUE
+#if GLIB_CHECK_VERSION(2, 64, 0)
+void vir_g_source_unref(GSource *src, GMainContext *ctx G_GNUC_UNUSED)
+{
+    g_source_unref(src);
+}
+#else
 
-gboolean
+static gboolean
 virEventGLibSourceUnrefIdle(gpointer data)
 {
     GSource *src = data;
@@ -243,4 +248,15 @@ virEventGLibSourceUnrefIdle(gpointer data)
     return FALSE;
 }
 
+void vir_g_source_unref(GSource *src, GMainContext *ctx)
+{
+    GSource *idle = g_idle_source_new();
+
+    g_source_set_callback(idle, virEventGLibSourceUnrefIdle, src, NULL);
+
+    g_source_attach(idle, ctx);
+
+    g_source_unref(idle);
+}
+
 #endif
diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h
index 9c52843274..697687b967 100644
--- a/src/util/glibcompat.h
+++ b/src/util/glibcompat.h
@@ -85,13 +85,4 @@ char *vir_g_strdup_vprintf(const char *msg, va_list args)
 #undef g_fsync
 #define g_fsync vir_g_fsync
 
-/* Drop when min glib >= 2.64.0 */
-#if GLIB_CHECK_VERSION(2, 64, 0)
-# define g_vir_source_unref_safe(source) g_source_unref(source)
-#else
-# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source)
-
-gboolean
-virEventGLibSourceUnrefIdle(gpointer data);
-
-#endif
+void vir_g_source_unref(GSource *src, GMainContext *ctx);
diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 88e3ec6d5d..f3e5a344b0 100644
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -214,7 +214,7 @@ virEventGLibHandleUpdate(int watch,
         if (data->source != NULL) {
             VIR_DEBUG("Removed old handle source=%p", data->source);
             g_source_destroy(data->source);
-            g_vir_source_unref_safe(data->source);
+            vir_g_source_unref(data->source, NULL);
         }
 
         data->source = virEventGLibAddSocketWatch(
@@ -228,7 +228,7 @@ virEventGLibHandleUpdate(int watch,
 
         VIR_DEBUG("Removed old handle source=%p", data->source);
         g_source_destroy(data->source);
-        g_vir_source_unref_safe(data->source);
+        vir_g_source_unref(data->source, NULL);
         data->source = NULL;
         data->events = 0;
     }
@@ -277,7 +277,7 @@ virEventGLibHandleRemove(int watch)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_vir_source_unref_safe(data->source);
+        vir_g_source_unref(data->source, NULL);
         data->source = NULL;
         data->events = 0;
     }
@@ -410,7 +410,7 @@ virEventGLibTimeoutUpdate(int timer,
     if (interval >= 0) {
         if (data->source != NULL) {
             g_source_destroy(data->source);
-            g_vir_source_unref_safe(data->source);
+            vir_g_source_unref(data->source, NULL);
         }
 
         data->interval = interval;
@@ -420,7 +420,7 @@ virEventGLibTimeoutUpdate(int timer,
             goto cleanup;
 
         g_source_destroy(data->source);
-        g_vir_source_unref_safe(data->source);
+        vir_g_source_unref(data->source, NULL);
         data->source = NULL;
     }
 
@@ -469,7 +469,7 @@ virEventGLibTimeoutRemove(int timer)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_vir_source_unref_safe(data->source);
+        vir_g_source_unref(data->source, NULL);
         data->source = NULL;
     }
 
-- 
2.30.2