4:02 p.m.
Matthias' patch made me realize that I didn't write the udev init code
as cleanly as I'd like, and the error handling was starting to get a
little messy. The attached patch cleans it up. There should be no
functional change.
Dave
4:02 p.m.
New subject: [libvirt] [PATCH 1/1] Cleanups for udev init code
---
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);
@@ -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;
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);
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
4:39 p.m.
New subject: [libvirt] [PATCH 1/1] Cleanups for udev init code
2010/1/26 David Allan <dallan(a)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
9:34 a.m.
New subject: [libvirt] [PATCH 1/1] Cleanups for udev init code
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(a)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
5413
days inactive
5414
days old
3 comments
3 participants
participants (3)
-
Dave Allan -
David Allan
-
Matthias Bolte