9:23 a.m.
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(a)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(a)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"