Re: [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