From 365553645c92cd34fbceb1fc3b523cb129e79399 Mon Sep 17 00:00:00 2001 From: Erik Skultety Date: Fri, 6 Oct 2017 15:21:55 +0200 Subject: [PATCH] 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 --- src/node_device/node_device_udev.c | 128 ++++++++++++++++++----------- src/node_device/node_device_udev.h | 3 - 2 files changed, 82 insertions(+), 49 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 8314b3834e..a7b6281531 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) - 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 9a07ab77e4..f15e5204c3 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -23,9 +23,6 @@ #include #include -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" -- GitLab