1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v6 0/9] Work around the kernel mdev uevent race in nodedev
  4. View patch

[libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node

Erik Skultety posted 9 patches 7 years, 6 months ago
[libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node
Posted by Erik Skultety 7 years, 6 months ago 
udevSetupSystemDev only needs the udev data lock to be locked because of
calling udevGetDMIData which accesses some protected structure members,
but it can do that on its own just fine, no need to hold the lock the
whole time.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index e0e5ba799..6882517e6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
     virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
     virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
 
+    virObjectLock(priv);
     udev = udev_monitor_get_udev(priv->udev_monitor);
 
     device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
@@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
             return;
         }
     }
+    virObjectUnlock(priv);
 
     if (udevGetStringSysfsAttr(device, "product_name",
                                &syscap->product_name) < 0)
@@ -1898,11 +1900,11 @@ nodeStateInitialize(bool privileged,
     if (priv->watch == -1)
         goto unlock;
 
+    virObjectUnlock(priv);
+
     /* Create a fictional 'computer' device to root the device tree. */
     if (udevSetupSystemDev() != 0)
-        goto unlock;
-
-    virObjectUnlock(priv);
+        goto cleanup;
 
     /* Populate with known devices */
     if (udevEnumerateDevices(udev) != 0)
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node
Posted by John Ferlan 7 years, 6 months ago 

On 10/18/2017 09:52 AM, Erik Skultety wrote:
> udevSetupSystemDev only needs the udev data lock to be locked because of
> calling udevGetDMIData which accesses some protected structure members,
> but it can do that on its own just fine, no need to hold the lock the
> whole time.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index e0e5ba799..6882517e6 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
>      virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
>      virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
>  
> +    virObjectLock(priv);
>      udev = udev_monitor_get_udev(priv->udev_monitor);
>  
>      device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
> @@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)

               virObjectUnlock(priv);

Pesky return statements ;-)

John

>              return;
>          }
>      }
> +    virObjectUnlock(priv);
>  
>      if (udevGetStringSysfsAttr(device, "product_name",
>                                 &syscap->product_name) < 0)
> @@ -1898,11 +1900,11 @@ nodeStateInitialize(bool privileged,
>      if (priv->watch == -1)
>          goto unlock;
>  
> +    virObjectUnlock(priv);
> +
>      /* Create a fictional 'computer' device to root the device tree. */
>      if (udevSetupSystemDev() != 0)
> -        goto unlock;
> -
> -    virObjectUnlock(priv);
> +        goto cleanup;
>  
>      /* Populate with known devices */
>      if (udevEnumerateDevices(udev) != 0)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node
Posted by Erik Skultety 7 years, 6 months ago 
On Wed, Oct 18, 2017 at 05:13:41PM -0400, John Ferlan wrote:
>
>
> On 10/18/2017 09:52 AM, Erik Skultety wrote:
> > udevSetupSystemDev only needs the udev data lock to be locked because of
> > calling udevGetDMIData which accesses some protected structure members,
> > but it can do that on its own just fine, no need to hold the lock the
> > whole time.
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index e0e5ba799..6882517e6 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
> >      virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
> >      virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
> >
> > +    virObjectLock(priv);
> >      udev = udev_monitor_get_udev(priv->udev_monitor);
> >
> >      device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
> > @@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
>
>                virObjectUnlock(priv);
>
> Pesky return statements ;-)
>

Wow, you really must have been a sharpshooter in the previous life ;), thanks a
lot.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node
Posted by John Ferlan 7 years, 6 months ago 

On 10/19/2017 02:54 AM, Erik Skultety wrote:
> On Wed, Oct 18, 2017 at 05:13:41PM -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2017 09:52 AM, Erik Skultety wrote:
>>> udevSetupSystemDev only needs the udev data lock to be locked because of
>>> calling udevGetDMIData which accesses some protected structure members,
>>> but it can do that on its own just fine, no need to hold the lock the
>>> whole time.
>>>
>>> Signed-off-by: Erik Skultety <eskultet@redhat.com>
>>> ---
>>>  src/node_device/node_device_udev.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>>> index e0e5ba799..6882517e6 100644
>>> --- a/src/node_device/node_device_udev.c
>>> +++ b/src/node_device/node_device_udev.c
>>> @@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
>>>      virNodeDevCapSystemHardwarePtr hardware = &syscap->hardware;
>>>      virNodeDevCapSystemFirmwarePtr firmware = &syscap->firmware;
>>>
>>> +    virObjectLock(priv);
>>>      udev = udev_monitor_get_udev(priv->udev_monitor);
>>>
>>>      device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
>>> @@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
>>
>>                virObjectUnlock(priv);
>>
>> Pesky return statements ;-)
>>
> 
> Wow, you really must have been a sharpshooter in the previous life ;), thanks a
> lot.
> 
> Erik
> 

Sadly they seem to work better when reviewing code... They often fail me
on my own code ;-)

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.