[libvirt] [PATCH 1/1] Cleanups for udev init code

[Matthias Bolte]

2010/1/26 David Allan <dallan at 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