2010/1/25 Dave Allan <dallan@xxxxxxxxxx>:
> On 01/25/2010 02:33 PM, Matthias Bolte wrote:
>>
>> 2010/1/25 Dave Allan<dallan@xxxxxxxxxx>:
>>>
>>> On 01/25/2010 06:37 AM, Daniel P. Berrange wrote:
>>>>
>>>> On Sun, Jan 24, 2010 at 11:07:59PM +0100, Matthias Bolte wrote:
>>>>>
>>>>> udevDeviceMonitorStartup registers udevEventHandleCallback as event
>>>>> handle, but doesn't store the returned watch id to remove it later on.
>>>>> Also it's not clear to me whether the event handle should be register
>>>>> for the whole lifetime of the udev driver instance or just for the
>>>>> udevEnumerateDevices call.
>>>>
>>>> The handler should be active for the lifetime of libvirtd, since the
>>>> udev driver has to detect hotplug/unplug events over time.
>>>>
>>>>>
>>>>> If for example the call to udevSetupSystemDev [1] fails
>>>>> udevDeviceMonitorShutdown is called to cleanup, but
>>>>> udevEventHandleCallback is still registered and may be called when
>>>>> driverState is NULL again, resulting in a segfault in
>>>>> udevEventHandleCallback.
>>>>>
>>>>> So to solve this the udevEventHandleCallback event handle must be
>>>>> removed at the appropriate place.
>>>>
>>>> Yes, sounds like its needs to be removed in the failure path there
>>>
>>> Matthias,
>>>
>>> Indeed, that's correct--can you submit a patch?
>>>
>>> Dave
>>>
>>
>> Yes, im going to do that.
>>
>> One last question. The udev driver stores the udev monitor handle in
>> the private data pointer of the gloval driver state variable.
>>
>> driverState->privateData = udev_monitor;
>>
>> To solve the event handle problem we need to store the watch id
>> returned by virEventAddHandle somewhere. We could either add a new
>> private data struct to hold the udev_monitor pointer and the watch id,
>> or store the watch id variable globally side by side with the driver
>> state variable. I prefer the first option, because it's cleaner and
>> the DRV_STATE_UDEV_MONITOR define allows for changing the storage
>> location of the udev_monitor pointer easily.
>
> Yes, I agree--a struct is the right approach. Thanks for doing this!
>
> Dave
>
Here's the patch. Maximilian Wilhelm tested it.
Matthias
From 07678696504179c70b987947061de2ff658e665c Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Tue, 26 Jan 2010 02:58:37 +0100
Subject: [PATCH] udev: Remove event handle on shutdown
This fixes a segfault when the event handler is called after shutdown
when the global driver state is NULL again.
Also fix a locking issue in an error path.
---
src/node_device/node_device_udev.c | 49 +++++++++++++++++++++++++++---------
src/node_device/node_device_udev.h | 4 ++-
2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 2e459d1..a625d76 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -41,6 +41,11 @@
#define VIR_FROM_THIS VIR_FROM_NODEDEV
+struct _udevPrivate {
+ struct udev_monitor *udev_monitor;
+ int watch;
+};
+
static virDeviceMonitorStatePtr driverState = NULL;
static int udevStrToLong_ull(char const *s,
@@ -1354,12 +1359,18 @@ static int udevDeviceMonitorShutdown(void)
{
int ret = 0;
+ udevPrivate *priv = NULL;
struct udev_monitor *udev_monitor = NULL;
struct udev *udev = NULL;
if (driverState) {
-
nodeDeviceLock(driverState);
+
+ priv = driverState->privateData;
+
+ if (priv->watch != -1)
+ virEventRemoveHandle(priv->watch);
+
udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
if (udev_monitor != NULL) {
@@ -1375,7 +1386,7 @@ static int udevDeviceMonitorShutdown(void)
nodeDeviceUnlock(driverState);
virMutexDestroy(&driverState->lock);
VIR_FREE(driverState);
-
+ VIR_FREE(priv);
} else {
ret = -1;
}
@@ -1534,18 +1545,28 @@ out:
static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
{
+ udevPrivate *priv = NULL;
struct udev *udev = NULL;
- struct udev_monitor *udev_monitor = NULL;
int ret = 0;
+ if (VIR_ALLOC(priv) < 0) {
+ virReportOOMError(NULL);
+ ret = -1;
+ goto out;
+ }
+
+ priv->watch = -1;
+
if (VIR_ALLOC(driverState) < 0) {
virReportOOMError(NULL);
+ VIR_FREE(priv);
ret = -1;
goto out;
}
if (virMutexInit(&driverState->lock) < 0) {
VIR_ERROR0("Failed to initialize mutex for driverState");
+ VIR_FREE(priv);
VIR_FREE(driverState);
ret = -1;
goto out;
@@ -1562,18 +1583,19 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
udev = udev_new();
udev_set_log_fn(udev, udevLogFunction);
- udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
- if (udev_monitor == NULL) {
+ 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;
}
- udev_monitor_enable_receiving(udev_monitor);
+ udev_monitor_enable_receiving(priv->udev_monitor);
/* udev can be retrieved from udev_monitor */
- driverState->privateData = udev_monitor;
- nodeDeviceUnlock(driverState);
+ driverState->privateData = priv;
/* We register the monitor with the event callback so we are
* notified by udev of device changes before we enumerate existing
@@ -1583,14 +1605,17 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
* enumeration. The alternative is to register the callback after
* we enumerate, in which case we will fail to create any devices
* that appear while the enumeration is taking place. */
- if (virEventAddHandle(udev_monitor_get_fd(udev_monitor),
- VIR_EVENT_HANDLE_READABLE,
- udevEventHandleCallback,
- NULL, NULL) == -1) {
+ priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
+ VIR_EVENT_HANDLE_READABLE,
+ udevEventHandleCallback, NULL, NULL);
+ if (priv->watch == -1) {
+ nodeDeviceUnlock(driverState);
ret = -1;
goto out;
}
+ nodeDeviceUnlock(driverState);
+
/* Create a fictional 'computer' device to root the device tree. */
if (udevSetupSystemDev() != 0) {
ret = -1;
diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h
index 6c83412..8367494 100644
--- a/src/node_device/node_device_udev.h
+++ b/src/node_device/node_device_udev.h
@@ -23,8 +23,10 @@
#include <libudev.h>
#include <stdint.h>
+typedef struct _udevPrivate udevPrivate;
+
#define SYSFS_DATA_SIZE 4096
-#define DRV_STATE_UDEV_MONITOR(ds) ((struct udev_monitor *)((ds)->privateData))
+#define DRV_STATE_UDEV_MONITOR(ds) (((udevPrivate *)((ds)->privateData))->udev_monitor)
#define DMI_DEVPATH "/sys/devices/virtual/dmi/id"
#define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id"
#define PROPERTY_FOUND 0
--
1.6.3.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list