[libvirt] [PATCH 1/1] Change locking for udev monitor and callbacks
Daniel P. Berrange
berrange at redhat.com
Wed Apr 6 10:04:20 UTC 2011
On Tue, Apr 05, 2011 at 04:14:17PM -0500, Serge Hallyn wrote:
> We're seeing bugs apparently resulting from thread unsafety of
> libpciaccess, such as
> https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/726099
> To prevent those, as suggested by danpb on irc, move the
> nodeDeviceLock(driverState) higher into the callers. In
> particular:
>
> udevDeviceMonitorStartup should hold the lock while calling
> udevEnumerateDevices(), and udevEventHandleCallback should hold it
> over its entire execution.
>
> It's not clear to me whether it is ok to hold the
> nodeDeviceLock while taking the virNodeDeviceObjLock(dev) on a
> device. If not, then the lock will need to be dropped around
> the calling of udevSetupSystemDev(), and udevAddOneDevice()
> may not actually be safe to call from higher layers with the
> driverstate lock held.
>
> libvirt 0.8.8 with this patch on it seems to work fine for me.
> Assuming it looks ok and I haven't done anything obviously dumb,
> I'll ask the bug submitters to try this patch.
>
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> ---
> src/node_device/node_device_udev.c | 21 +++++++++------------
> 1 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 372f1d1..2139ef3 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1202,7 +1202,6 @@ static int udevRemoveOneDevice(struct udev_device *device)
> int ret = 0;
>
> name = udev_device_get_syspath(device);
> - nodeDeviceLock(driverState);
> dev = virNodeDeviceFindBySysfsPath(&driverState->devs, name);
>
> if (dev != NULL) {
> @@ -1214,7 +1213,6 @@ static int udevRemoveOneDevice(struct udev_device *device)
> name);
> ret = -1;
> }
> - nodeDeviceUnlock(driverState);
>
> return ret;
> }
> @@ -1316,9 +1314,7 @@ static int udevAddOneDevice(struct udev_device *device)
>
> /* If this is a device change, the old definition will be freed
> * and the current definition will take its place. */
> - nodeDeviceLock(driverState);
> dev = virNodeDeviceAssignDef(&driverState->devs, def);
> - nodeDeviceUnlock(driverState);
>
> if (dev == NULL) {
> VIR_ERROR(_("Failed to create device for '%s'"), def->name);
> @@ -1442,6 +1438,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> const char *action = NULL;
> int udev_fd = -1;
>
> + nodeDeviceLock(driverState);
> udev_fd = udev_monitor_get_fd(udev_monitor);
> if (fd != udev_fd) {
> VIR_ERROR(_("File descriptor returned by udev %d does not "
> @@ -1470,6 +1467,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>
> out:
> udev_device_unref(device);
> + nodeDeviceUnlock(driverState);
> return;
> }
>
> @@ -1647,10 +1645,9 @@ static int udevDeviceMonitorStartup(int privileged)
> priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
> if (priv->udev_monitor == NULL) {
> VIR_FREE(priv);
> - nodeDeviceUnlock(driverState);
> VIR_ERROR0(_("udev_monitor_new_from_netlink returned NULL"));
> ret = -1;
> - goto out;
> + goto out_unlock;
> }
>
> udev_monitor_enable_receiving(priv->udev_monitor);
> @@ -1670,26 +1667,26 @@ static int udevDeviceMonitorStartup(int privileged)
> VIR_EVENT_HANDLE_READABLE,
> udevEventHandleCallback, NULL, NULL);
> if (priv->watch == -1) {
> - nodeDeviceUnlock(driverState);
> ret = -1;
> - goto out;
> + goto out_unlock;
> }
>
> - nodeDeviceUnlock(driverState);
> -
> /* Create a fictional 'computer' device to root the device tree. */
> if (udevSetupSystemDev() != 0) {
> ret = -1;
> - goto out;
> + goto out_unlock;
> }
>
> /* Populate with known devices */
>
> if (udevEnumerateDevices(udev) != 0) {
> ret = -1;
> - goto out;
> + goto out_unlock;
> }
>
> +out_unlock:
> + nodeDeviceUnlock(driverState);
> +
> out:
> if (ret == -1) {
> udevDeviceMonitorShutdown();
ACK, it looks good to me - serializing all access to the udev
and libpciaccess APIs is a good conservative approach.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list