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

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:



I'll push after the 2.1.0 release is out!

Thanks,
Cole

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