[v1] udev: only report a warning if udev_enumerate_scan_devices fails

[libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails

Posted by Marc Hartmayer 5 years, 2 months ago

Even if an error is reported by `udev_enumerate_scan_devices`,
e.g. because a driver of a device has an bug, we can still enumerate
all other devices. Additionally the documentation of
udev_enumerate_scan_devices says that on success an integer >= 0 is
returned (see man udev_enumerate_scan_devices(3)).

Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/node_device/node_device_udev.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 299f55260129..90168eb8a969 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
     if (udevEnumerateAddMatches(udev_enumerate) < 0)
         goto cleanup;
 
-    ret = udev_enumerate_scan_devices(udev_enumerate);
-    if (ret != 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("udev scan devices returned %d"),
-                       ret);
-        goto cleanup;
-    }
+    if (udev_enumerate_scan_devices(udev_enumerate) < 0)
+        VIR_WARN("udev scan devices failed");
 
     udev_list_entry_foreach(list_entry,
                             udev_enumerate_get_list_entry(udev_enumerate)) {
-- 
2.17.0

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

Re: [libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails

Posted by John Ferlan 5 years, 2 months ago

On 2/13/19 7:38 AM, Marc Hartmayer wrote:
> Even if an error is reported by `udev_enumerate_scan_devices`,
> e.g. because a driver of a device has an bug, we can still enumerate
> all other devices. Additionally the documentation of
> udev_enumerate_scan_devices says that on success an integer >= 0 is
> returned (see man udev_enumerate_scan_devices(3)).
> 
> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/node_device/node_device_udev.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 

Interesting - looking at many examples of udev_enumerate_scan_devices
usage shows a lack of testing the return value and as is done here just
using @udev_enumerate to add devices after the call.

Eventually found some source code for enumerator_scan_devices_tags which
I believe is what device_enumerator_scan_devices would call due to what
our AddMatches does. It seems that code works until it finds an error,
but still would return a partially enumerated list.

Long way of saying I think this is fine... However, now @ret = -1
doesn't ever get changed, so the caller would still fail. So it's a nice
way to test your other patch ;-)

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 299f55260129..90168eb8a969 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
>      if (udevEnumerateAddMatches(udev_enumerate) < 0)
>          goto cleanup;
>  
> -    ret = udev_enumerate_scan_devices(udev_enumerate);
> -    if (ret != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("udev scan devices returned %d"),
> -                       ret);
> -        goto cleanup;
> -    }
> +    if (udev_enumerate_scan_devices(udev_enumerate) < 0)
> +        VIR_WARN("udev scan devices failed");

Either before or after this, set ret = 0... or change the default from
-1 to 0 and only change if the AddMatches fails.

I think the other patch would still be necessary since if
udevEnumerateAddMatches fails, then wouldn't the issue of setting
threadQuit still exist?

>  
>      udev_list_entry_foreach(list_entry,
>                              udev_enumerate_get_list_entry(udev_enumerate)) {
> 

BTW: Using udevProcessDeviceListEntry as the 'example' of not failing if
an element of udev_enumerate is problematic, I think logically if we
don't get a full list we'd be OK to continue as well.

John

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

Re: [libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails

Posted by Marc Hartmayer 5 years, 2 months ago

On Wed, Feb 13, 2019 at 03:56 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 2/13/19 7:38 AM, Marc Hartmayer wrote:
>> Even if an error is reported by `udev_enumerate_scan_devices`,
>> e.g. because a driver of a device has an bug, we can still enumerate
>> all other devices. Additionally the documentation of
>> udev_enumerate_scan_devices says that on success an integer >= 0 is
>> returned (see man udev_enumerate_scan_devices(3)).
>>
>> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  src/node_device/node_device_udev.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>
> Interesting - looking at many examples of udev_enumerate_scan_devices
> usage shows a lack of testing the return value and as is done here just
> using @udev_enumerate to add devices after the call.
>
> Eventually found some source code for enumerator_scan_devices_tags which
> I believe is what device_enumerator_scan_devices would call due to what
> our AddMatches does. It seems that code works until it finds an error,
> but still would return a partially enumerated list.

Yep, I’ve also looked at the source code. Unfortunately the behavior is
not documented… I’ve also looked for 'udev_enumerate_get_list_entry' and
it can handle NULL pointers.

>
> Long way of saying I think this is fine... However, now @ret = -1
> doesn't ever get changed, so the caller would still fail. So it's a nice
> way to test your other patch ;-)

Yes… I’ll send a v2.

>
>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>> index 299f55260129..90168eb8a969 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
>>      if (udevEnumerateAddMatches(udev_enumerate) < 0)
>>          goto cleanup;
>>
>> -    ret = udev_enumerate_scan_devices(udev_enumerate);
>> -    if (ret != 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("udev scan devices returned %d"),
>> -                       ret);
>> -        goto cleanup;
>> -    }
>> +    if (udev_enumerate_scan_devices(udev_enumerate) < 0)
>> +        VIR_WARN("udev scan devices failed");
>
> Either before or after this, set ret = 0... or change the default from
> -1 to 0 and only change if the AddMatches fails.

I’ll set 'ret = 0;' at the end.

>
> I think the other patch would still be necessary since if
> udevEnumerateAddMatches fails, then wouldn't the issue of setting
> threadQuit still exist?
>
>>
>>      udev_list_entry_foreach(list_entry,
>>                              udev_enumerate_get_list_entry(udev_enumerate)) {
>>
>
> BTW: Using udevProcessDeviceListEntry as the 'example' of not failing if
> an element of udev_enumerate is problematic, I think logically if we
> don't get a full list we'd be OK to continue as well.

I agree.

>
> John
>
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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