External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2025-02-22 11:22:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

Change locking for udev monitor and callbacks

Browse Source 
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@ubuntu.com>
This commit is contained in:
Serge Hallyn 2011-04-05 16:14:17 -05:00 committed by Eric Blake
parent 25b39315d6
commit 2879582847
1 changed files with 9 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
src/node_device/node_device_udev.c
View File

@ -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();