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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
