10:58 a.m.
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
10:58 a.m.
New subject: [PATCH 1/2] interface_udev: Replace udevNumOfInterfacesByStatus by udevListInterfacesByStatus
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(a)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
10:58 a.m.
New subject: [PATCH 2/2] udevListInterfacesByStatus: Don't try to return NULL names
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(a)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
2:28 a.m.
New subject: [PATCH 0/2] interface: Refactor code and fix old listing API when
interface is unbound
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(a)redhat.com>
Michal
164
days inactive
165
days old
3 comments
2 participants
participants (2)
-
Michal Prívozník -
Peter Krempa