The documentation of gobject signals reads:

"If you are connecting handlers to signals and using a GObject instance as your
signal handler user data, you should remember to pair calls to
g_signal_connect() with calls to g_signal_handler_disconnect() or
g_signal_handlers_disconnect_by_func(). While signal handlers are automatically
disconnected when the object emitting the signal is finalised..." [1]

This means that the signal handlers are automatically disconnected as soon as
the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`.
But this also means that it's possible that new work is tried to be scheduled
for the workerpool by the `mdevctlEventHandleCallback` (main thread context)
even if the workerpool has already been stopped by `nodeStateShutdownWait`. To
fully understand this, it's important to know that the main loop of the main
thread is still running for some time even after `nodeStateShutdownPrepare` has
been called. Let's avoid this situation by explicitly disconnect the signals
during `nodeStateShutdownPrepare`, which is called in the main thread, so that
no new work is attempted to be scheduled for the worker pool.

[1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handlers

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/node_device/node_device_udev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index a3006433e842..38740033a66e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2236,6 +2236,12 @@ nodeStateShutdownPrepare(void)
     if (!priv)
         return 0;
 
+    VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
+        GList *tmp;
+        for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next)
+            g_signal_handlers_disconnect_by_data(tmp->data, priv);
+    }
+
     VIR_WITH_OBJECT_LOCK_GUARD(priv) {
         if (priv->mdevctlTimeout != -1) {
             virEventRemoveTimeout(priv->mdevctlTimeout);
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 4/19/24 16:49, Marc Hartmayer wrote:
> The documentation of gobject signals reads:
> 
> "If you are connecting handlers to signals and using a GObject instance as your
> signal handler user data, you should remember to pair calls to
> g_signal_connect() with calls to g_signal_handler_disconnect() or
> g_signal_handlers_disconnect_by_func(). While signal handlers are automatically
> disconnected when the object emitting the signal is finalised..." [1]
> 
> This means that the signal handlers are automatically disconnected as soon as
> the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`.
> But this also means that it's possible that new work is tried to be scheduled
> for the workerpool by the `mdevctlEventHandleCallback` (main thread context)
> even if the workerpool has already been stopped by `nodeStateShutdownWait`. To
> fully understand this, it's important to know that the main loop of the main
> thread is still running for some time even after `nodeStateShutdownPrepare` has
> been called. Let's avoid this situation by explicitly disconnect the signals
> during `nodeStateShutdownPrepare`, which is called in the main thread, so that
> no new work is attempted to be scheduled for the worker pool.
> 
> [1]https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handlers
> 
> Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com>
> ---
>   src/node_device/node_device_udev.c | 6 ++++++
>   1 file changed, 6 insertions(+)

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 4/19/24 9:49 AM, Marc Hartmayer wrote:
> The documentation of gobject signals reads:
> 
> "If you are connecting handlers to signals and using a GObject instance as your
> signal handler user data, you should remember to pair calls to
> g_signal_connect() with calls to g_signal_handler_disconnect() or
> g_signal_handlers_disconnect_by_func(). While signal handlers are automatically
> disconnected when the object emitting the signal is finalised..." [1]
> 
> This means that the signal handlers are automatically disconnected as soon as
> the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`.
> But this also means that it's possible that new work is tried to be scheduled
> for the workerpool by the `mdevctlEventHandleCallback` (main thread context)
> even if the workerpool has already been stopped by `nodeStateShutdownWait`. To
> fully understand this, it's important to know that the main loop of the main
> thread is still running for some time even after `nodeStateShutdownPrepare` has
> been called. Let's avoid this situation by explicitly disconnect the signals
> during `nodeStateShutdownPrepare`, which is called in the main thread, so that
> no new work is attempted to be scheduled for the worker pool.
> 
> [1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handlers
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>   src/node_device/node_device_udev.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index a3006433e842..38740033a66e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -2236,6 +2236,12 @@ nodeStateShutdownPrepare(void)
>       if (!priv)
>           return 0;
>   
> +    VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
> +        GList *tmp;
> +        for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next)
> +            g_signal_handlers_disconnect_by_data(tmp->data, priv);
> +    }
> +
>       VIR_WITH_OBJECT_LOCK_GUARD(priv) {
>           if (priv->mdevctlTimeout != -1) {
>               virEventRemoveTimeout(priv->mdevctlTimeout);


Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org