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