8:52 a.m.
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(a)redhat.com>
---
src/node_device/node_device_udev.c | 134 +++++++++++++++++++++++--------------
src/node_device/node_device_udev.h | 3 -
2 files changed, 85 insertions(+), 52 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 8314b3834..a7b628153 100644
--- 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)
+ if (VIR_ALLOC(driver) < 0)
return -1;
- priv->watch = -1;
-
- if (VIR_ALLOC(driver) < 0) {
- VIR_FREE(priv);
- 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;
+ 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)
@@ -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 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