[PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting

Marc Hartmayer posted 20 patches 1 year, 9 months ago
There is a newer version of this series
[PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting
Posted by Marc Hartmayer 1 year, 9 months ago
Instead of accessing the global `driver` object pass the `udevEventData` as
parameter to the thread handler and watch callback. This has the advantage that:
1. proper refcounting
2. easier to read and test

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

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index cc8700fc4117..07cbdfa3bdeb 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
 
 /**
  * udevEventHandleThread
- * @opaque: unused
+ * @opaque: udevEventData
  *
  * Thread to handle the udevEventHandleCallback processing when udev
  * tells us there's a device change for us (add, modify, delete, etc).
@@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv,
  * would still come into play.
  */
 static void
-udevEventHandleThread(void *opaque G_GNUC_UNUSED)
+udevEventHandleThread(void *opaque)
 {
-    udevEventData *priv = driver->privateData;
+    g_autoptr(udevEventData) priv = opaque;
     struct udev_device *device = NULL;
 
     /* continue rather than break from the loop on non-fatal errors */
@@ -1943,9 +1943,9 @@ static void
 udevEventHandleCallback(int watch G_GNUC_UNUSED,
                         int fd,
                         int events G_GNUC_UNUSED,
-                        void *data G_GNUC_UNUSED)
+                        void *data)
 {
-    udevEventData *priv = driver->privateData;
+    udevEventData *priv = data;
     VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
 
     if (!udevEventMonitorSanityCheck(priv, fd))
@@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged,
 
     priv->udevThread = g_new0(virThread, 1);
     if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread,
-                            "udev-event", false, NULL) < 0) {
+                            "udev-event", false, virObjectRef(priv)) < 0) {
         virReportSystemError(errno, "%s",
                              _("failed to create udev handler thread"));
         goto unlock;
@@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged,
      * that appear while the enumeration is taking place.  */
     priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
                                     VIR_EVENT_HANDLE_READABLE,
-                                    udevEventHandleCallback, NULL, NULL);
+                                    udevEventHandleCallback, virObjectRef(priv), virObjectUnref);
     if (priv->watch == -1)
         goto unlock;
 
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting
Posted by Boris Fiuczynski 1 year, 9 months ago
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 4/19/24 16:49, Marc Hartmayer wrote:
> Instead of accessing the global `driver` object pass the `udevEventData` as
> parameter to the thread handler and watch callback. This has the advantage that:
> 1. proper refcounting
> 2. easier to read and test
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>   src/node_device/node_device_udev.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index cc8700fc4117..07cbdfa3bdeb 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
>   
>   /**
>    * udevEventHandleThread
> - * @opaque: unused
> + * @opaque: udevEventData
>    *
>    * Thread to handle the udevEventHandleCallback processing when udev
>    * tells us there's a device change for us (add, modify, delete, etc).
> @@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv,
>    * would still come into play.
>    */
>   static void
> -udevEventHandleThread(void *opaque G_GNUC_UNUSED)
> +udevEventHandleThread(void *opaque)
>   {
> -    udevEventData *priv = driver->privateData;
> +    g_autoptr(udevEventData) priv = opaque;
>       struct udev_device *device = NULL;
>   
>       /* continue rather than break from the loop on non-fatal errors */
> @@ -1943,9 +1943,9 @@ static void
>   udevEventHandleCallback(int watch G_GNUC_UNUSED,
>                           int fd,
>                           int events G_GNUC_UNUSED,
> -                        void *data G_GNUC_UNUSED)
> +                        void *data)
>   {
> -    udevEventData *priv = driver->privateData;
> +    udevEventData *priv = data;
>       VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
>   
>       if (!udevEventMonitorSanityCheck(priv, fd))
> @@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged,
>   
>       priv->udevThread = g_new0(virThread, 1);
>       if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread,
> -                            "udev-event", false, NULL) < 0) {
> +                            "udev-event", false, virObjectRef(priv)) < 0) {
>           virReportSystemError(errno, "%s",
>                                _("failed to create udev handler thread"));
>           goto unlock;
> @@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged,
>        * that appear while the enumeration is taking place.  */
>       priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
>                                       VIR_EVENT_HANDLE_READABLE,
> -                                    udevEventHandleCallback, NULL, NULL);
> +                                    udevEventHandleCallback, virObjectRef(priv), virObjectUnref);
>       if (priv->watch == -1)
>           goto unlock;
>   

-- 
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
Re: [PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting
Posted by Jonathon Jongsma 1 year, 9 months ago
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
> Instead of accessing the global `driver` object pass the `udevEventData` as
> parameter to the thread handler and watch callback. This has the advantage that:
> 1. proper refcounting
> 2. easier to read and test
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>   src/node_device/node_device_udev.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index cc8700fc4117..07cbdfa3bdeb 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
>   
>   /**
>    * udevEventHandleThread
> - * @opaque: unused
> + * @opaque: udevEventData
>    *
>    * Thread to handle the udevEventHandleCallback processing when udev
>    * tells us there's a device change for us (add, modify, delete, etc).
> @@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv,
>    * would still come into play.
>    */
>   static void
> -udevEventHandleThread(void *opaque G_GNUC_UNUSED)
> +udevEventHandleThread(void *opaque)
>   {
> -    udevEventData *priv = driver->privateData;
> +    g_autoptr(udevEventData) priv = opaque;
>       struct udev_device *device = NULL;
>   
>       /* continue rather than break from the loop on non-fatal errors */
> @@ -1943,9 +1943,9 @@ static void
>   udevEventHandleCallback(int watch G_GNUC_UNUSED,
>                           int fd,
>                           int events G_GNUC_UNUSED,
> -                        void *data G_GNUC_UNUSED)
> +                        void *data)
>   {
> -    udevEventData *priv = driver->privateData;
> +    udevEventData *priv = data;
>       VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
>   
>       if (!udevEventMonitorSanityCheck(priv, fd))
> @@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged,
>   
>       priv->udevThread = g_new0(virThread, 1);
>       if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread,
> -                            "udev-event", false, NULL) < 0) {
> +                            "udev-event", false, virObjectRef(priv)) < 0) {
>           virReportSystemError(errno, "%s",
>                                _("failed to create udev handler thread"));
>           goto unlock;
> @@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged,
>        * that appear while the enumeration is taking place.  */
>       priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
>                                       VIR_EVENT_HANDLE_READABLE,
> -                                    udevEventHandleCallback, NULL, NULL);
> +                                    udevEventHandleCallback, virObjectRef(priv), virObjectUnref);
>       if (priv->watch == -1)
>           goto unlock;
>   


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