Since there's going to be a worker thread which needs to have some data
protected by a lock, the whole code would just simply get unnecessary
complex, since two sets of locks would be necessary, driver lock (for
udev monitor and event handle) and a mutex protecting thread-local data.
Given the future thread will need to access the udev monitor socket as
well, why not protect everything with a single lock, even better, by
converting the driver's private data to a lockable object, we get the
automatic object disposal feature for free.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 140 ++++++++++++++++++++++++-------------
 src/node_device/node_device_udev.h |   3 -
 2 files changed, 93 insertions(+), 50 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index a9a4c9b6b..bb9787fdb 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -53,12 +53,78 @@ VIR_LOG_INIT("node_device.node_device_udev");
 # define TYPE_RAID 12
 #endif
 
-struct _udevPrivate {
+typedef struct _udevEventData udevEventData;
+typedef udevEventData *udevEventDataPtr;
+
+struct _udevEventData {
+    virObjectLockable parent;
+
     struct udev_monitor *udev_monitor;
     int watch;
     bool privileged;
 };
 
+static virClassPtr udevEventDataClass;
+
+static void
+udevEventDataDispose(void *obj)
+{
+    struct udev *udev = NULL;
+    udevEventDataPtr data = obj;
+
+    if (data->watch != -1)
+        virEventRemoveHandle(data->watch);
+
+    if (data->udev_monitor)
+        udev = udev_monitor_get_udev(data->udev_monitor);
+
+    udev_unref(udev);
+    udev_monitor_unref(data->udev_monitor);
+}
+
+
+static int
+udevEventDataOnceInit(void)
+{
+    if (!(udevEventDataClass = virClassNew(virClassForObjectLockable(),
+                                           "udevEventData",
+                                           sizeof(udevEventData),
+                                           udevEventDataDispose)))
+        return -1;
+
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(udevEventData)
+
+static udevEventDataPtr
+udevEventDataNew(void)
+{
+    udevEventDataPtr ret = NULL;
+
+    if (udevEventDataInitialize() < 0)
+        return NULL;
+
+    if (!(ret = virObjectLockableNew(udevEventDataClass)))
+        return NULL;
+
+    ret->watch = -1;
+    return ret;
+}
+
+
+static bool
+udevEventDataIsPrivileged(udevEventDataPtr data)
+{
+    bool privileged;
+
+    virObjectLock(data);
+    privileged = data->privileged;
+    virObjectUnlock(data);
+
+    return privileged;
+}
+
 
 static bool
 udevHasDeviceProperty(struct udev_device *dev,
@@ -447,7 +513,7 @@ udevProcessPCI(struct udev_device *device,
     virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev;
     virPCIEDeviceInfoPtr pci_express = NULL;
     virPCIDevicePtr pciDev = NULL;
-    udevPrivate *priv = driver->privateData;
+    udevEventDataPtr priv = driver->privateData;
     int ret = -1;
     char *p;
 
@@ -498,7 +564,7 @@ udevProcessPCI(struct udev_device *device,
         goto cleanup;
 
     /* We need to be root to read PCI device configs */
-    if (priv->privileged) {
+    if (udevEventDataIsPrivileged(priv)) {
         if (virPCIGetHeaderType(pciDev, &pci_dev->hdrType) < 0)
             goto cleanup;
 
@@ -1559,39 +1625,18 @@ udevPCITranslateDeinit(void)
 static int
 nodeStateCleanup(void)
 {
-    udevPrivate *priv = NULL;
-    struct udev_monitor *udev_monitor = NULL;
-    struct udev *udev = NULL;
-
     if (!driver)
         return -1;
 
     nodeDeviceLock();
 
+    virObjectUnref(driver->privateData);
     virObjectUnref(driver->nodeDeviceEventState);
 
-    priv = driver->privateData;
-
-    if (priv) {
-        if (priv->watch != -1)
-            virEventRemoveHandle(priv->watch);
-
-        udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
-
-        if (udev_monitor != NULL) {
-            udev = udev_monitor_get_udev(udev_monitor);
-            udev_monitor_unref(udev_monitor);
-        }
-    }
-
-    if (udev != NULL)
-        udev_unref(udev);
-
     virNodeDeviceObjListFree(driver->devs);
     nodeDeviceUnlock();
     virMutexDestroy(&driver->lock);
     VIR_FREE(driver);
-    VIR_FREE(priv);
 
     udevPCITranslateDeinit();
     return 0;
@@ -1615,16 +1660,17 @@ udevHandleOneDevice(struct udev_device *device)
 }
 
 
+/* the caller must be holding the udevEventData object lock prior to calling
+ * this function
+ */
 static bool
-udevEventMonitorSanityCheck(struct udev_monitor *udev_monitor,
+udevEventMonitorSanityCheck(udevEventDataPtr priv,
                             int fd)
 {
     int rc = -1;
 
-    rc = udev_monitor_get_fd(udev_monitor);
+    rc = udev_monitor_get_fd(priv->udev_monitor);
     if (fd != rc) {
-        udevPrivate *priv = driver->privateData;
-
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("File descriptor returned by udev %d does not "
                          "match node device file descriptor %d"),
@@ -1650,15 +1696,19 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
                         int events ATTRIBUTE_UNUSED,
                         void *data ATTRIBUTE_UNUSED)
 {
+    udevEventDataPtr priv = driver->privateData;
     struct udev_device *device = NULL;
-    struct udev_monitor *udev_monitor = NULL;
 
-    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+    virObjectLock(priv);
 
-    if (!udevEventMonitorSanityCheck(udev_monitor, fd))
+    if (!udevEventMonitorSanityCheck(priv, fd)) {
+        virObjectUnlock(priv);
         return;
+    }
+
+    device = udev_monitor_receive_device(priv->udev_monitor);
+    virObjectUnlock(priv);
 
-    device = udev_monitor_receive_device(udev_monitor);
     if (device == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("udev_monitor_receive_device returned NULL"));
@@ -1675,12 +1725,13 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
 static void
 udevGetDMIData(virNodeDevCapSystemPtr syscap)
 {
+    udevEventDataPtr priv = driver->privateData;
     struct udev *udev = NULL;
     struct udev_device *device = NULL;
     virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
     virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
 
-    udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driver));
+    udev = udev_monitor_get_udev(priv->udev_monitor);
 
     device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
     if (device == NULL) {
@@ -1791,15 +1842,9 @@ nodeStateInitialize(bool privileged,
                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
                     void *opaque ATTRIBUTE_UNUSED)
 {
-    udevPrivate *priv = NULL;
+    udevEventDataPtr priv = NULL;
     struct udev *udev = NULL;
 
-    if (VIR_ALLOC(priv) < 0)
-        return -1;
-
-    priv->watch = -1;
-    priv->privileged = privileged;
-
     if (VIR_ALLOC(driver) < 0) {
         VIR_FREE(priv);
         return -1;
@@ -1813,12 +1858,13 @@ nodeStateInitialize(bool privileged,
         return -1;
     }
 
+    nodeDeviceLock();
+
+    if (!(driver->devs = virNodeDeviceObjListNew()) ||
+        !(priv = udevEventDataNew()))
+        goto unlock;
+
     driver->privateData = priv;
-    nodeDeviceLock();
-
-    if (!(driver->devs = virNodeDeviceObjListNew()))
-        goto unlock;
-
     driver->nodeDeviceEventState = virObjectEventStateNew();
 
     if (udevPCITranslateInit(privileged) < 0)
@@ -1836,7 +1882,7 @@ nodeStateInitialize(bool privileged,
 #endif
 
     priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
-    if (priv->udev_monitor == NULL) {
+    if (!priv->udev_monitor) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("udev_monitor_new_from_netlink returned NULL"));
         goto unlock;
diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h
index 9a07ab77e..f15e5204c 100644
--- a/src/node_device/node_device_udev.h
+++ b/src/node_device/node_device_udev.h
@@ -23,9 +23,6 @@
 #include <libudev.h>
 #include <stdint.h>
 
-typedef struct _udevPrivate udevPrivate;
-
 #define SYSFS_DATA_SIZE 4096
-#define DRV_STATE_UDEV_MONITOR(ds) (((udevPrivate *)((ds)->privateData))->udev_monitor)
 #define DMI_DEVPATH "/sys/devices/virtual/dmi/id"
 #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id"
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/11/2017 10:52 AM, Erik Skultety wrote:
> Since there's going to be a worker thread which needs to have some data
> protected by a lock, the whole code would just simply get unnecessary
> complex, since two sets of locks would be necessary, driver lock (for
> udev monitor and event handle) and a mutex protecting thread-local data.
> Given the future thread will need to access the udev monitor socket as
> well, why not protect everything with a single lock, even better, by
> converting the driver's private data to a lockable object, we get the
> automatic object disposal feature for free.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 140 ++++++++++++++++++++++++-------------
>  src/node_device/node_device_udev.h |   3 -
>  2 files changed, 93 insertions(+), 50 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index a9a4c9b6b..bb9787fdb 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -53,12 +53,78 @@ VIR_LOG_INIT("node_device.node_device_udev");
>  # define TYPE_RAID 12
>  #endif
>  
> -struct _udevPrivate {
> +typedef struct _udevEventData udevEventData;
> +typedef udevEventData *udevEventDataPtr;
> +
> +struct _udevEventData {
> +    virObjectLockable parent;
> +
>      struct udev_monitor *udev_monitor;
>      int watch;
>      bool privileged;
>  };

Mental note - maybe the driver->privateData should change to
driver->udevEventData in _virNodeDeviceDriverState

You interchange using @priv and @data throughout which right now could
make sense, but in the future may make less sense. I have a preference
for consistency, so calling @data in some places and @priv in others
leads to inconsistency. I believe pick one and be consistent.

>  
> +static virClassPtr udevEventDataClass;
> +
> +static void
> +udevEventDataDispose(void *obj)
> +{
> +    struct udev *udev = NULL;
> +    udevEventDataPtr data = obj;
> +
> +    if (data->watch != -1)
> +        virEventRemoveHandle(data->watch);
> +
> +    if (data->udev_monitor)
> +        udev = udev_monitor_get_udev(data->udev_monitor);
> +
> +    udev_unref(udev);
> +    udev_monitor_unref(data->udev_monitor);

This is a different order than previous cleanup - perhaps should be:

    if (data->udev_monitor) {
        udev = udev_monitor_get_udev(data->udev_monitor);
        udev_monitor_unref(data->udev_monitor);
        udev_unref(udev);
    }


or

    if (!data->udev_monitor)
        return;

    udev = udev_monitor_get_udev(data->udev_monitor);
    udev_monitor_unref(data->udev_monitor);
    udev_unref(udev);

Just not clear what happens to udev if you unref udev before unref
udev_monitor.  Better safe than sorry and go in the opposite order of
Initialization.

> +}
> +
> +
> +static int
> +udevEventDataOnceInit(void)
> +{
> +    if (!(udevEventDataClass = virClassNew(virClassForObjectLockable(),
> +                                           "udevEventData",
> +                                           sizeof(udevEventData),
> +                                           udevEventDataDispose)))
> +        return -1;
> +
> +    return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(udevEventData)
> +
> +static udevEventDataPtr
> +udevEventDataNew(void)
> +{
> +    udevEventDataPtr ret = NULL;
> +
> +    if (udevEventDataInitialize() < 0)
> +        return NULL;
> +
> +    if (!(ret = virObjectLockableNew(udevEventDataClass)))
> +        return NULL;
> +
> +    ret->watch = -1;
> +    return ret;
> +}
> +
> +
> +static bool
> +udevEventDataIsPrivileged(udevEventDataPtr data)
> +{
> +    bool privileged;

Hmmm... is @data privileged or is the driver that needs the privilege
check... IOW: should privileged by a DriverState value (even though it'd
be unused in _hal).  It comes in as a virDrvStateInitialize value. That
way it's just a driver->privileged check regardless of whether there is
an event thread running.

I know - should have thought of this sooner, but sometimes things are
more obvious at odd points in review time. Moving privileged would be a
patch 1.5 and thus make this helper unnecessary. It'd be a simple move
from private to NodeDeviceDriverState and then store/read from there
instead of @priv.  Then udevGetDMIData wouldn't need to get @priv.

> +
> +    virObjectLock(data);
> +    privileged = data->privileged;
> +    virObjectUnlock(data);
> +
> +    return privileged;
> +}
> +
>  
>  static bool
>  udevHasDeviceProperty(struct udev_device *dev,
> @@ -447,7 +513,7 @@ udevProcessPCI(struct udev_device *device,
>      virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev;
>      virPCIEDeviceInfoPtr pci_express = NULL;
>      virPCIDevicePtr pciDev = NULL;
> -    udevPrivate *priv = driver->privateData;
> +    udevEventDataPtr priv = driver->privateData;
>      int ret = -1;
>      char *p;
>  
> @@ -498,7 +564,7 @@ udevProcessPCI(struct udev_device *device,
>          goto cleanup;
>  
>      /* We need to be root to read PCI device configs */
> -    if (priv->privileged) {
> +    if (udevEventDataIsPrivileged(priv)) {
>          if (virPCIGetHeaderType(pciDev, &pci_dev->hdrType) < 0)
>              goto cleanup;
>  
> @@ -1559,39 +1625,18 @@ udevPCITranslateDeinit(void)
>  static int
>  nodeStateCleanup(void)
>  {
> -    udevPrivate *priv = NULL;
> -    struct udev_monitor *udev_monitor = NULL;
> -    struct udev *udev = NULL;
> -
>      if (!driver)
>          return -1;
>  
>      nodeDeviceLock();
>  
> +    virObjectUnref(driver->privateData);
>      virObjectUnref(driver->nodeDeviceEventState);
>  
> -    priv = driver->privateData;
> -
> -    if (priv) {

FWIW: It's this that got me to thinking privileged should never have
been part of @priv...

> -        if (priv->watch != -1)
> -            virEventRemoveHandle(priv->watch);
> -
> -        udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> -
> -        if (udev_monitor != NULL) {
> -            udev = udev_monitor_get_udev(udev_monitor);
> -            udev_monitor_unref(udev_monitor);
> -        }
> -    }
> -
> -    if (udev != NULL)
> -        udev_unref(udev);
> -
>      virNodeDeviceObjListFree(driver->devs);
>      nodeDeviceUnlock();
>      virMutexDestroy(&driver->lock);
>      VIR_FREE(driver);
> -    VIR_FREE(priv);
>  
>      udevPCITranslateDeinit();
>      return 0;
> @@ -1615,16 +1660,17 @@ udevHandleOneDevice(struct udev_device *device)
>  }
>  
>  
> +/* the caller must be holding the udevEventData object lock prior to calling
> + * this function
> + */
>  static bool
> -udevEventMonitorSanityCheck(struct udev_monitor *udev_monitor,
> +udevEventMonitorSanityCheck(udevEventDataPtr priv,
>                              int fd)
>  {
>      int rc = -1;
>  
> -    rc = udev_monitor_get_fd(udev_monitor);
> +    rc = udev_monitor_get_fd(priv->udev_monitor);
>      if (fd != rc) {
> -        udevPrivate *priv = driver->privateData;
> -
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("File descriptor returned by udev %d does not "
>                           "match node device file descriptor %d"),
> @@ -1650,15 +1696,19 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int events ATTRIBUTE_UNUSED,
>                          void *data ATTRIBUTE_UNUSED)
>  {
> +    udevEventDataPtr priv = driver->privateData;
>      struct udev_device *device = NULL;
> -    struct udev_monitor *udev_monitor = NULL;
>  
> -    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);

Should we add a "if (!priv) return" before this?

> +    virObjectLock(priv);
>  
> -    if (!udevEventMonitorSanityCheck(udev_monitor, fd))
> +    if (!udevEventMonitorSanityCheck(priv, fd)) {
> +        virObjectUnlock(priv);
>          return;
> +    }
> +
> +    device = udev_monitor_receive_device(priv->udev_monitor);
> +    virObjectUnlock(priv);
>  
> -    device = udev_monitor_receive_device(udev_monitor);
>      if (device == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("udev_monitor_receive_device returned NULL"));
> @@ -1675,12 +1725,13 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>  static void
>  udevGetDMIData(virNodeDevCapSystemPtr syscap)
>  {
> +    udevEventDataPtr priv = driver->privateData;
>      struct udev *udev = NULL;
>      struct udev_device *device = NULL;
>      virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
>      virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
>  
> -    udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driver));
> +    udev = udev_monitor_get_udev(priv->udev_monitor);

There's no lock/unlock around priv-> access here (although there is
earlier).

>  
>      device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
>      if (device == NULL) {
> @@ -1791,15 +1842,9 @@ nodeStateInitialize(bool privileged,
>                      virStateInhibitCallback callback ATTRIBUTE_UNUSED,
>                      void *opaque ATTRIBUTE_UNUSED)
>  {
> -    udevPrivate *priv = NULL;
> +    udevEventDataPtr priv = NULL;
>      struct udev *udev = NULL;
>  
> -    if (VIR_ALLOC(priv) < 0)
> -        return -1;
> -
> -    priv->watch = -1;
> -    priv->privileged = privileged;

FWIW: You lost priv->privileged setting in this patch... So it'd always
be false

> -
>      if (VIR_ALLOC(driver) < 0) {
>          VIR_FREE(priv);

^^ won't be necessary and then of course the { } won't be either...

Same a few lines lower where there's VIR_FREE(priv), but the { } would
stay after a virMutexInit failure

With a few tweaks and edits...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John
>          return -1;
> @@ -1813,12 +1858,13 @@ nodeStateInitialize(bool privileged,
>          return -1;
>      }
>  
> +    nodeDeviceLock();
> +
> +    if (!(driver->devs = virNodeDeviceObjListNew()) ||
> +        !(priv = udevEventDataNew()))
> +        goto unlock;
> +
>      driver->privateData = priv;
> -    nodeDeviceLock();
> -
> -    if (!(driver->devs = virNodeDeviceObjListNew()))
> -        goto unlock;
> -
>      driver->nodeDeviceEventState = virObjectEventStateNew();
>  
>      if (udevPCITranslateInit(privileged) < 0)
> @@ -1836,7 +1882,7 @@ nodeStateInitialize(bool privileged,
>  #endif
>  
>      priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
> -    if (priv->udev_monitor == NULL) {
> +    if (!priv->udev_monitor) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("udev_monitor_new_from_netlink returned NULL"));
>          goto unlock;
> diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h
> index 9a07ab77e..f15e5204c 100644
> --- a/src/node_device/node_device_udev.h
> +++ b/src/node_device/node_device_udev.h
> @@ -23,9 +23,6 @@
>  #include <libudev.h>
>  #include <stdint.h>
>  
> -typedef struct _udevPrivate udevPrivate;
> -
>  #define SYSFS_DATA_SIZE 4096
> -#define DRV_STATE_UDEV_MONITOR(ds) (((udevPrivate *)((ds)->privateData))->udev_monitor)
>  #define DMI_DEVPATH "/sys/devices/virtual/dmi/id"
>  #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id"
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]
> > +struct _udevEventData {
> > +    virObjectLockable parent;
> > +
> >      struct udev_monitor *udev_monitor;
> >      int watch;
> >      bool privileged;
> >  };
>
> Mental note - maybe the driver->privateData should change to
> driver->udevEventData in _virNodeDeviceDriverState

I like the notion of generic private data per backend, even though one of them
doesn't use it all. But in principle, I agree with you, I'd just wait 'till we
decide that we can  safely deprecate hal backend in upstream completely.

>
> You interchange using @priv and @data throughout which right now could
> make sense, but in the future may make less sense. I have a preference
> for consistency, so calling @data in some places and @priv in others
> leads to inconsistency. I believe pick one and be consistent.

Good point, noted.

>
> >
> > +static virClassPtr udevEventDataClass;
> > +
> > +static void
> > +udevEventDataDispose(void *obj)
> > +{
> > +    struct udev *udev = NULL;
> > +    udevEventDataPtr data = obj;
> > +
> > +    if (data->watch != -1)
> > +        virEventRemoveHandle(data->watch);
> > +
> > +    if (data->udev_monitor)
> > +        udev = udev_monitor_get_udev(data->udev_monitor);
> > +
> > +    udev_unref(udev);
> > +    udev_monitor_unref(data->udev_monitor);
>
> This is a different order than previous cleanup - perhaps should be:
>
>     if (data->udev_monitor) {
>         udev = udev_monitor_get_udev(data->udev_monitor);
>         udev_monitor_unref(data->udev_monitor);
>         udev_unref(udev);
>     }
>

libudev handles NULLs gracefully...

>
> or
>
>     if (!data->udev_monitor)
>         return;
>
>     udev = udev_monitor_get_udev(data->udev_monitor);
>     udev_monitor_unref(data->udev_monitor);
>     udev_unref(udev);

I'll go with this version then, I did it this way, because udev keeps a pointer
to the context withing the monitor, but doesn't touch it when freeing, leaving
the responsibility to the caller. But I agree that releasing the monitor before
releasing the context makes more sense.

>
> Just not clear what happens to udev if you unref udev before unref
> udev_monitor.  Better safe than sorry and go in the opposite order of
> Initialization.
>
> > +}
> > +
> > +
> > +static int
> > +udevEventDataOnceInit(void)
> > +{
> > +    if (!(udevEventDataClass = virClassNew(virClassForObjectLockable(),
> > +                                           "udevEventData",
> > +                                           sizeof(udevEventData),
> > +                                           udevEventDataDispose)))
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +VIR_ONCE_GLOBAL_INIT(udevEventData)
> > +
> > +static udevEventDataPtr
> > +udevEventDataNew(void)
> > +{
> > +    udevEventDataPtr ret = NULL;
> > +
> > +    if (udevEventDataInitialize() < 0)
> > +        return NULL;
> > +
> > +    if (!(ret = virObjectLockableNew(udevEventDataClass)))
> > +        return NULL;
> > +
> > +    ret->watch = -1;
> > +    return ret;
> > +}
> > +
> > +
> > +static bool
> > +udevEventDataIsPrivileged(udevEventDataPtr data)
> > +{
> > +    bool privileged;
>
> Hmmm... is @data privileged or is the driver that needs the privilege
> check... IOW: should privileged by a DriverState value (even though it'd
> be unused in _hal).  It comes in as a virDrvStateInitialize value. That
> way it's just a driver->privileged check regardless of whether there is
> an event thread running.

Good observation, a patch will be added.

>
> I know - should have thought of this sooner, but sometimes things are
> more obvious at odd points in review time. Moving privileged would be a
> patch 1.5 and thus make this helper unnecessary. It'd be a simple move
> from private to NodeDeviceDriverState and then store/read from there
> instead of @priv.  Then udevGetDMIData wouldn't need to get @priv.

No problem.


[..]
> >  {
> > +    udevEventDataPtr priv = driver->privateData;
> >      struct udev_device *device = NULL;
> > -    struct udev_monitor *udev_monitor = NULL;
> >
> > -    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>
> Should we add a "if (!priv) return" before this?

This would only be necessary if that was a user's input, but it's us who's
completely responsible for that and @data are only touched by stateCleanup,
which runs after the eventloop ends, so this check would be essentially useless.

>
> > +    virObjectLock(priv);
> >
> > -    if (!udevEventMonitorSanityCheck(udev_monitor, fd))
> > +    if (!udevEventMonitorSanityCheck(priv, fd)) {
> > +        virObjectUnlock(priv);
> >          return;
> > +    }
> > +
> > +    device = udev_monitor_receive_device(priv->udev_monitor);
> > +    virObjectUnlock(priv);
> >
> > -    device = udev_monitor_receive_device(udev_monitor);
> >      if (device == NULL) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                         _("udev_monitor_receive_device returned NULL"));
> > @@ -1675,12 +1725,13 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> >  static void
> >  udevGetDMIData(virNodeDevCapSystemPtr syscap)
> >  {
> > +    udevEventDataPtr priv = driver->privateData;
> >      struct udev *udev = NULL;
> >      struct udev_device *device = NULL;
> >      virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
> >      virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
> >
> > -    udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driver));
> > +    udev = udev_monitor_get_udev(priv->udev_monitor);
>
> There's no lock/unlock around priv-> access here (although there is
> earlier).

this is called from within Initialize which holds the lock almost the whole
time, so this would cause a deadlock (I only realized this when I tried it). So
because what you say makes more sense, I'll add a separate patch that will move
the object lock in Initialize before trying to set up the system node (there's
no need to hold it the whole time) and then we can go with this suggestion.

>
> >
> >      device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
> >      if (device == NULL) {
> > @@ -1791,15 +1842,9 @@ nodeStateInitialize(bool privileged,
> >                      virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> >                      void *opaque ATTRIBUTE_UNUSED)
> >  {
> > -    udevPrivate *priv = NULL;
> > +    udevEventDataPtr priv = NULL;
> >      struct udev *udev = NULL;
> >
> > -    if (VIR_ALLOC(priv) < 0)
> > -        return -1;
> > -
> > -    priv->watch = -1;
> > -    priv->privileged = privileged;
>
> FWIW: You lost priv->privileged setting in this patch... So it'd always
> be false

Great eyes ;).

>
> > -
> >      if (VIR_ALLOC(driver) < 0) {
> >          VIR_FREE(priv);
>
> ^^ won't be necessary and then of course the { } won't be either...
>
> Same a few lines lower where there's VIR_FREE(priv), but the { } would
> stay after a virMutexInit failure

It's always mistakes like these that are so obvious yet they can be only
spotted during a review :), thanks. Also, I appreciate the RBs but since I'll
be adding 3 more patches (even though trivial ones) I'd still appreciate one
more (final) review.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list