[libvirt] [PATCH v2 6/8] node_device: Implement event queue in udev
Cole Robinson
crobinso at redhat.com
Fri Jul 29 15:48:30 UTC 2016
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
More information about the libvir-list
mailing list