nodedev: udev: Convert udev private data to a lockable object

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>

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 8314b3834e2f7f2b90f3c937070b830dfd92a5d5..a7b6281531d87866d710f9338bedcdd4cf0d91fe 100644 (file)
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -53,11 +53,65 @@ 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;
 };
 
+static virClassPtr udevEventDataClass;
+
+static void
+udevEventDataDispose(void *obj)
+{
+    struct udev *udev = NULL;
+    udevEventDataPtr priv = obj;
+
+    if (priv->watch != -1)
+        virEventRemoveHandle(priv->watch);
+
+    if (!priv->udev_monitor)
+        return;
+
+    udev = udev_monitor_get_udev(priv->udev_monitor);
+    udev_monitor_unref(priv->udev_monitor);
+    udev_unref(udev);
+}
+
+
+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
 udevHasDeviceProperty(struct udev_device *dev,
@@ -1562,39 +1616,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;
@@ -1618,16 +1651,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"),
@@ -1653,15 +1687,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"));
@@ -1678,12 +1716,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) {
@@ -1794,34 +1833,26 @@ 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;
-
-    if (VIR_ALLOC(driver) < 0) {
-        VIR_FREE(priv);
+    if (VIR_ALLOC(driver) < 0)
         return -1;
-    }
 
     if (virMutexInit(&driver->lock) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Unable to initialize mutex"));
-        VIR_FREE(priv);
         VIR_FREE(driver);
         return -1;
     }
 
-    driver->privileged = privileged;
-    driver->privateData = priv;
     nodeDeviceLock();
 
-    if (!(driver->devs = virNodeDeviceObjListNew()))
+    if (!(driver->devs = virNodeDeviceObjListNew()) ||
+        !(priv = udevEventDataNew()))
         goto unlock;
 
+    driver->privateData = priv;
     driver->nodeDeviceEventState = virObjectEventStateNew();
 
     if (udevPCITranslateInit(privileged) < 0)
@@ -1838,8 +1869,10 @@ nodeStateInitialize(bool privileged,
     udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction);
 #endif
 
+    virObjectLock(priv);
+
     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;
@@ -1874,6 +1907,7 @@ nodeStateInitialize(bool privileged,
     if (udevSetupSystemDev() != 0)
         goto unlock;
 
+    virObjectUnlock(priv);
     nodeDeviceUnlock();
 
     /* Populate with known devices */
@@ -1887,6 +1921,8 @@ nodeStateInitialize(bool privileged,
     return -1;
 
  unlock:
+    if (priv)
+        virObjectUnlock(priv);
     nodeDeviceUnlock();
     goto cleanup;
 }

diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h
index 9a07ab77e43daefd988c1fb3c4725a248a395dc3..f15e5204c36402890c6c0f55f33596a9821878d8 100644 (file)
--- 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"