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

[Dave Allan]

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 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