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@xxxxxxxxxxxxx>
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
> ---
> 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
