[libvirt] [PATCH 7/9] node_device: Implement event queue in udev

[Cole Robinson]

I'd title this:

  node_device: udev: implement lifecycle events

On 07/20/2016 09:50 AM, Jovanka Gulicoska wrote:
> ---
>  src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 76c60ea..a4748ef 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,6 +1025,7 @@ 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;

Make this 'ret = -1'. So 'goto cleanup'; can be used for error conditions.

>  
> @@ -1031,15 +1033,24 @@ static int udevRemoveOneDevice(struct udev_device *device)
>      dev = virNodeDeviceFindBySysfsPath(&driver->devs, name);
>  
>      if (dev != NULL) {
> +        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);
> +        goto cleanup;
>      } else {
>          VIR_DEBUG("Failed to find device to remove that has udev name '%s'",
>                    name);
>          ret = -1;
> +        goto cleanup;
>      }
>  

Take this else block and raise it up to be if (!dev) ... goto cleanup;
Then handle the lifecycle event creation outside of an if block.
And put ret = 0; right before the 'cleanup:'. This is typically how the
pattern works

> + cleanup:
> +    if (event)
> +        virObjectEventStateQueue(driver->nodeDeviceEventState, event);
>      return ret;
>  }
> 
> @@ -1096,6 +1107,7 @@ static int udevAddOneDevice(struct udev_device *device)
>  {
>      virNodeDeviceDefPtr def = NULL;
>      virNodeDeviceObjPtr dev = NULL;
> +    virObjectEventPtr event = NULL;
>      int ret = -1;
>  
>      if (VIR_ALLOC(def) != 0)
> @@ -1125,11 +1137,18 @@ static int udevAddOneDevice(struct udev_device *device)
>      if (dev == NULL)
>          goto cleanup;
>  
> +    event = virNodeDeviceEventLifecycleNew(dev->def->name,
> +                                           VIR_NODE_DEVICE_EVENT_CREATED,
> +                                           0);
> +
>      virNodeDeviceObjUnlock(dev);
>  

This section is problematic if we want to add a separate UPDATED event, since
this same function is used for both an updated and added device. I think
before calling NodeDeviceAssignDef, you'll want to call
virNodeDeviceFindByName to check if the device already exists or not. Only
emit the CREATED event if it's a brand new device.

>      ret = 0;
>  
>   cleanup:
> +    if (event)
> +        virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +
>      if (ret != 0) {
>          VIR_DEBUG("Discarding device %d %p %s", ret, def,
>                    def ? NULLSTR(def->sysfs_path) : "");
> @@ -1241,6 +1260,8 @@ static int nodeStateCleanup(void)
>  
>      nodeDeviceLock();
>  
> +    virObjectEventStateFree(driver->nodeDeviceEventState);
> +
>      priv = driver->privateData;
>  
>      if (priv) {
> @@ -1456,6 +1477,7 @@ static int nodeStateInitialize(bool privileged,
>  
>      driver->privateData = priv;
>      nodeDeviceLock();
> +    driver->nodeDeviceEventState = virObjectEventStateNew();
>  
>      if (udevPCITranslateInit(privileged) < 0)
>          goto cleanup;
> @@ -1526,6 +1548,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {
>      .nodeNumOfDevices = nodeNumOfDevices, /* 0.7.3 */
>      .nodeListDevices = nodeListDevices, /* 0.7.3 */
>      .connectListAllNodeDevices = nodeConnectListAllNodeDevices, /* 0.10.2 */
> +    .connectNodeDeviceEventRegisterAny = nodeConnectNodeDeviceEventRegisterAny, /* 2.1.0 */
> +    .connectNodeDeviceEventDeregisterAny = nodeConnectNodeDeviceEventDeregisterAny, /* 2.1.0 */

Update these comments to 2.2.0

Thanks,
Cole

>      .nodeDeviceLookupByName = nodeDeviceLookupByName, /* 0.7.3 */
>      .nodeDeviceLookupSCSIHostByWWN = nodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */
>      .nodeDeviceGetXMLDesc = nodeDeviceGetXMLDesc, /* 0.7.3 */
>