[PATCH 0/2] interface: Refactor code and fix old listing API when interface is unbound
Patch 1/2 removes a pointless helper in favor of making an almost identical function more universal, so that patch 2/2 then fixes all cases in one place. Peter Krempa (2): interface_udev: Replace udevNumOfInterfacesByStatus by udevListInterfacesByStatus udevListInterfacesByStatus: Don't try to return NULL names src/interface/interface_backend_udev.c | 84 +++++++++----------------- 1 file changed, 30 insertions(+), 54 deletions(-) -- 2.44.0
Make the array-filling operation of udevListInterfacesByStatus optional and replace the completely redundant udevNumOfInterfacesByStatus by it. Further patches fixing the listing will not need to be duplicated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/interface/interface_backend_udev.c | 76 ++++++++------------------ 1 file changed, 22 insertions(+), 54 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 4091483060..826f486049 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -137,55 +137,21 @@ udevGetDevices(struct udev *udev, virUdevStatus status) return enumerate; } -static int -udevNumOfInterfacesByStatus(virConnectPtr conn, virUdevStatus status, - virInterfaceObjListFilter filter) -{ - struct udev *udev = udev_ref(driver->udev); - struct udev_enumerate *enumerate = NULL; - struct udev_list_entry *devices; - struct udev_list_entry *dev_entry; - int count = 0; - - enumerate = udevGetDevices(udev, status); - - if (!enumerate) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to get number of %1$s interfaces on host"), - virUdevStatusString(status)); - count = -1; - goto cleanup; - } - - /* Do the scan to load up the enumeration */ - udev_enumerate_scan_devices(enumerate); - - /* Get a list we can walk */ - devices = udev_enumerate_get_list_entry(enumerate); - - /* For each item so we can count */ - udev_list_entry_foreach(dev_entry, devices) { - struct udev_device *dev; - const char *path; - g_autoptr(virInterfaceDef) def = NULL; - - path = udev_list_entry_get_name(dev_entry); - dev = udev_device_new_from_syspath(udev, path); - - def = udevGetMinimalDefForDevice(dev); - if (filter(conn, def)) - count++; - udev_device_unref(dev); - } - - cleanup: - if (enumerate) - udev_enumerate_unref(enumerate); - udev_unref(udev); - - return count; -} +/** + * udevListInterfacesByStatus: + * + * @conn: connection object + * @names: optional pointer to array to be filled with interface names + * @names_len: size of @names + * @status: status of interfaces to be listed + * @filter: ACL filter function + * + * Lists interfaces with status matching @status filling them into @names (if + * non-NULL) and returns the number of such interfaces. + * + * In case of an error -1 is returned and no interfaces are filled into @names. + */ static int udevListInterfacesByStatus(virConnectPtr conn, char **const names, @@ -222,7 +188,7 @@ udevListInterfacesByStatus(virConnectPtr conn, g_autoptr(virInterfaceDef) def = NULL; /* Ensure we won't exceed the size of our array */ - if (count >= names_len) + if (names && count >= names_len) break; path = udev_list_entry_get_name(dev_entry); @@ -230,7 +196,8 @@ udevListInterfacesByStatus(virConnectPtr conn, def = udevGetMinimalDefForDevice(dev); if (filter(conn, def)) { - names[count] = g_strdup(udev_device_get_sysname(dev)); + if (names) + names[count] = g_strdup(udev_device_get_sysname(dev)); count++; } udev_device_unref(dev); @@ -242,14 +209,15 @@ udevListInterfacesByStatus(virConnectPtr conn, return count; } + static int udevConnectNumOfInterfaces(virConnectPtr conn) { if (virConnectNumOfInterfacesEnsureACL(conn) < 0) return -1; - return udevNumOfInterfacesByStatus(conn, VIR_UDEV_IFACE_ACTIVE, - virConnectNumOfInterfacesCheckACL); + return udevListInterfacesByStatus(conn, NULL, 0, VIR_UDEV_IFACE_ACTIVE, + virConnectNumOfInterfacesCheckACL); } static int @@ -271,8 +239,8 @@ udevConnectNumOfDefinedInterfaces(virConnectPtr conn) if (virConnectNumOfDefinedInterfacesEnsureACL(conn) < 0) return -1; - return udevNumOfInterfacesByStatus(conn, VIR_UDEV_IFACE_INACTIVE, - virConnectNumOfDefinedInterfacesCheckACL); + return udevListInterfacesByStatus(conn, NULL, 0, VIR_UDEV_IFACE_INACTIVE, + virConnectNumOfDefinedInterfacesCheckACL); } static int -- 2.44.0
In case when the interface is being detached/reattached it may happen that udev will return NULL from 'udev_device_get_sysname()'. As the RPC code requires nonnull strings in the return array it fails to serialize such reply: libvirt: XML-RPC error : Unable to encode message payload Fix this by simply ignoring such interfaces as there's nothing we can report in such case. A similar fix was done to 'udevConnectListAllInterfaces' in commit 2ca94317ac6. Resolves: https://issues.redhat.com/browse/RHEL-34615 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/interface/interface_backend_udev.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 826f486049..8bb19d7764 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -185,6 +185,7 @@ udevListInterfacesByStatus(virConnectPtr conn, udev_list_entry_foreach(dev_entry, devices) { struct udev_device *dev; const char *path; + const char *name; g_autoptr(virInterfaceDef) def = NULL; /* Ensure we won't exceed the size of our array */ @@ -194,10 +195,17 @@ udevListInterfacesByStatus(virConnectPtr conn, path = udev_list_entry_get_name(dev_entry); dev = udev_device_new_from_syspath(udev, path); + if (!(name = udev_device_get_sysname(dev))) { + /* Name can be NULL in case when the interface is being unbound + * from the driver. The list API requires names to be present */ + VIR_DEBUG("Skipping interface '%s', name == NULL", path); + continue; + } + def = udevGetMinimalDefForDevice(dev); if (filter(conn, def)) { if (names) - names[count] = g_strdup(udev_device_get_sysname(dev)); + names[count] = g_strdup(name); count++; } udev_device_unref(dev); -- 2.44.0
On 5/6/24 17:58, Peter Krempa wrote:
Patch 1/2 removes a pointless helper in favor of making an almost identical function more universal, so that patch 2/2 then fixes all cases in one place.
Peter Krempa (2): interface_udev: Replace udevNumOfInterfacesByStatus by udevListInterfacesByStatus udevListInterfacesByStatus: Don't try to return NULL names
src/interface/interface_backend_udev.c | 84 +++++++++----------------- 1 file changed, 30 insertions(+), 54 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa