Re: [PATCH v5 2/6] nodedev: udev: Convert udev private data to a lockable object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux