On 01/26/2010 05:39 PM, Matthias Bolte wrote: Hi Matthias, Thanks for your catch of the priv == NULL case in the shutdown code. Priv does NOT leak, however, as it is freed in one place in the shutdown code in all cases. Updated patch to follow. Dave

2010/1/26 David Allan<dallan@redhat.com>:

...

--- src/node_device/node_device_udev.c | 34 +++++++++++++--------------------- 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index dcd889d..6895ac5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1368,8 +1368,9 @@ static int udevDeviceMonitorShutdown(void)

priv = driverState->privateData;

- if (priv->watch != -1) + if (priv->watch != -1) { virEventRemoveHandle(priv->watch); + }

udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);

Due to your changes to udevDeviceMonitorStartup it is possible that udevDeviceMonitorShutdown is called with driverState != NULL and driverState->privateData == NULL. In this case 'if (priv->watch != -1)' will segfault and 'udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);' will segfault also. Changing it like this will fix the problem:

if (priv != NULL) { if (priv->watch != -1) virEventRemoveHandle(priv->watch);

udev_monitor = DRV_STATE_UDEV_MONITOR(driverState); }

...

@@ -1387,6 +1388,7 @@ static int udevDeviceMonitorShutdown(void) virMutexDestroy(&driverState->lock); VIR_FREE(driverState); VIR_FREE(priv); + } else { ret = -1; } @@ -1547,28 +1549,22 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED) { udevPrivate *priv = NULL; struct udev *udev = NULL; - int ret = 0; + int ret = -1;

- if (VIR_ALLOC(priv)< 0) { + if (VIR_ALLOC(driverState)< 0) { virReportOOMError(NULL); - ret = -1; goto out; }

- priv->watch = -1; - - if (VIR_ALLOC(driverState)< 0) { + if (VIR_ALLOC(priv)< 0) { virReportOOMError(NULL); - VIR_FREE(priv); - ret = -1; goto out; }

+ priv->watch = -1; + if (virMutexInit(&driverState->lock)< 0) { VIR_ERROR0("Failed to initialize mutex for driverState"); - VIR_FREE(priv); - VIR_FREE(driverState); - ret = -1;

priv leaks here.

...

goto out; }

@@ -1585,10 +1581,8 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)

priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { - VIR_FREE(priv);

priv leaks here.

Moving the driverState->privateData = priv; line directly after the priv->watch = -1; will fix this leaks.

...

nodeDeviceUnlock(driverState); VIR_ERROR0("udev_monitor_new_from_netlink returned NULL"); - ret = -1; goto out; }

@@ -1608,27 +1602,25 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED) priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, NULL, NULL); + + nodeDeviceUnlock(driverState); + 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; goto out; }

/* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) { - ret = -1; goto out; }

+ ret = 0; + out: if (ret == -1) { udevDeviceMonitorShutdown(); -- 1.6.5.5

Matthias