Re: [PATCH v2 6/8] node_device: Implement event queue in udev

[Cole Robinson <crobinso@xxxxxxxxxx>]

On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
> ---
>  src/node_device/node_device_udev.c | 46 ++++++++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 76c60ea..e7a407f 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -28,6 +28,7 @@
>  
>  #include "dirname.h"
>  #include "node_device_conf.h"
> +#include "node_device_event.h"
>  #include "node_device_driver.h"
>  #include "node_device_linux_sysfs.h"
>  #include "node_device_udev.h"
> @@ -1024,22 +1025,31 @@ static int udevGetDeviceDetails(struct udev_device *device,
>  static int udevRemoveOneDevice(struct udev_device *device)
>  {
>      virNodeDeviceObjPtr dev = NULL;
> +    virObjectEventPtr event = NULL;
>      const char *name = NULL;
> -    int ret = 0;
> +    int ret = -1;
>  
>      name = udev_device_get_syspath(device);
>      dev = virNodeDeviceFindBySysfsPath(&driver->devs, name);
>  
> -    if (dev != NULL) {
> -        VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> -                  dev->def->name, name);
> -        virNodeDeviceObjRemove(&driver->devs, dev);
> -    } else {
> +    if (!dev) {
>          VIR_DEBUG("Failed to find device to remove that has udev name '%s'",
>                    name);
> -        ret = -1;
> +        goto cleanup;
>      }
>  
> +    event = virNodeDeviceEventLifecycleNew(dev->def->name,
> +                                           VIR_NODE_DEVICE_EVENT_DELETED,
> +                                           0);
> +
> +    VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> +              dev->def->name, name);
> +    virNodeDeviceObjRemove(&driver->devs, dev);
> +
> +    ret = 0;
> + cleanup:
> +    if (event)
> +        virObjectEventStateQueue(driver->nodeDeviceEventState, event);
>      return ret;
>  }
>  
> @@ -1096,6 +1106,8 @@ static int udevAddOneDevice(struct udev_device *device)
>  {
>      virNodeDeviceDefPtr def = NULL;
>      virNodeDeviceObjPtr dev = NULL;
> +    virObjectEventPtr event = NULL;
> +    bool new_device;
>      int ret = -1;
>  
>      if (VIR_ALLOC(def) != 0)
> @@ -1119,17 +1131,28 @@ static int udevAddOneDevice(struct udev_device *device)
>      if (udevSetParent(device, def) != 0)
>          goto cleanup;
>  
> +    dev = virNodeDeviceFindByName(&driver->devs, def->name);
> +    new_device = !!dev;

I know I recommended this little tidbit offline, but the logic is actually
wrong :) new_device should be !dev

> +    if (new_device)
> +        nodeDeviceUnlock();
> +

Wrong unlock call, we need to unlock 'dev', not the whole driver.

>      /* If this is a device change, the old definition will be freed
>       * and the current definition will take its place. */
>      dev = virNodeDeviceAssignDef(&driver->devs, def);
> -    if (dev == NULL)
> -        goto cleanup;
> +

We shouldn't drop this NULL check either.

These were the only issues in the patch series, so applied locally with this
stuff fixed:

diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index e7a407f..4182d5b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1107,7 +1107,7 @@ static int udevAddOneDevice(struct udev_device *device)
     virNodeDeviceDefPtr def = NULL;
     virNodeDeviceObjPtr dev = NULL;
     virObjectEventPtr event = NULL;
-    bool new_device;
+    bool new_device = true;
     int ret = -1;

     if (VIR_ALLOC(def) != 0)
@@ -1132,13 +1132,16 @@ static int udevAddOneDevice(struct udev_device *device)
         goto cleanup;

     dev = virNodeDeviceFindByName(&driver->devs, def->name);
-    new_device = !!dev;
-    if (new_device)
-        nodeDeviceUnlock();
+    if (dev) {
+        virNodeDeviceObjUnlock(dev);
+        new_device = false;
+    }

     /* If this is a device change, the old definition will be freed
      * and the current definition will take its place. */
     dev = virNodeDeviceAssignDef(&driver->devs, def);
+    if (dev == NULL)
+        goto cleanup;

     if (new_device)
         event = virNodeDeviceEventLifecycleNew(dev->def->name,


I'll push after the 2.1.0 release is out!

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list