On 4/21/25 21:38, Maximilian Martin via Devel wrote:
From: Maximilian Martin <maximilian_martin(a)gmx.de>
This patch implements USB bus/port matching. In addition
to vendor/product and bus/device matching, bus/port
matching is implemented. This involves additions and
refactorings in the domain configuration, host device
handling, and USB helpers. Also, test methods are
refactored and extended.
Signed-off-by: Maximilian Martin <maximilian_martin(a)gmx.de>
---
src/conf/domain_conf.c | 69 ++++++++++++++--
src/conf/domain_conf.h | 1 +
src/hypervisor/virhostdev.c | 131 +++++++++++++++++-------------
src/libvirt_private.syms | 2 -
src/util/virusb.c | 156 ++++++++++++------------------------
src/util/virusb.h | 32 ++++----
tests/virusbtest.c | 149 ++++++++++++++++++++++++----------
7 files changed, 312 insertions(+), 228 deletions(-)
There's still a lot happening here, but see my comments below.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 542d6ade91..423cad99e5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2670,6 +2670,15 @@ virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSI
*scsisrc)
}
}
+static void
+virDomainHostdevSubsysUSBClear(virDomainHostdevSubsysUSB *usbsrc)
+{
+ if (!usbsrc)
+ return;
+
+ VIR_FREE(usbsrc->port);
+}
+
static void
virDomainHostdevDefClear(virDomainHostdevDef *def)
@@ -2713,6 +2722,8 @@ virDomainHostdevDefClear(virDomainHostdevDef *def)
g_clear_pointer(&def->source.subsys.u.pci.origstates,
virBitmapFree);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
+ virDomainHostdevSubsysUSBClear(&def->source.subsys.u.usb);
+ break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
break;
@@ -5945,13 +5956,47 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
}
if ((addressNode = virXPathNode("./address", ctxt))) {
- if (virXMLPropUInt(addressNode, "bus", 0,
- VIR_XML_PROP_REQUIRED, &usbsrc->bus) < 0)
+ bool found_device = false;
+ bool found_port = false;
Nitpick, we prefer cammelCase.
+ char *port = NULL;
+ int ret = -1;
The 'ret' name is reserved for actual return value of functions. For
storing of retvals of other functions you can use 'rc', 'rv' or anything
else.
+
+ ret = virXMLPropUInt(addressNode, "bus", 0,
+ VIR_XML_PROP_REQUIRED, &usbsrc->bus);
+ if (ret < 0) {
+ return -1;
+ } else if (ret == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("missing bus"));
return -1;
This case can never happen. When VIR_XML_PROP_REQUIRED flag is specified
then virXMLProp*() functions return either -1 or 1.
+ }
- if (virXMLPropUInt(addressNode, "device", 0,
- VIR_XML_PROP_REQUIRED, &usbsrc->device) < 0)
+ ret = virXMLPropUInt(addressNode, "device", 0,
+ VIR_XML_PROP_NONE, &usbsrc->device);
+ if (ret < 0)
return -1;
+ else if (ret > 0)
+ found_device = true;
+
+ port = virXMLPropString(addressNode, "port");
+ if (port) {
+ if (*port) {
+ usbsrc->port = port;
+ found_port = true;
+ } else {
+ VIR_FREE(port);
If 'port' is declared as g_autofree then this is needless.
+ }
+ }
+
+ if (!found_device && !found_port) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("usb address needs either device id or
port"));
+ return -1;
+ } else if (found_device && found_port) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("found both device id and port in usb address
(ambiguous setting)"));
+ return -1;
+ }
}
return 0;
@@ -14774,8 +14819,13 @@ virDomainHostdevMatchSubsysUSB(virDomainHostdevDef *first,
virDomainHostdevSubsysUSB *first_usbsrc = &first->source.subsys.u.usb;
virDomainHostdevSubsysUSB *second_usbsrc = &second->source.subsys.u.usb;
- if (first_usbsrc->bus && first_usbsrc->device) {
- /* specified by bus location on host */
+ if (first_usbsrc->bus && first_usbsrc->port) {
+ /* specified by bus and port on host */
+ if (first_usbsrc->bus == second_usbsrc->bus &&
+ STREQ_NULLABLE(first_usbsrc->port, second_usbsrc->port))
+ return 1;
+ } else if (first_usbsrc->bus && first_usbsrc->device) {
+ /* specified by bus and device id on host */
if (first_usbsrc->bus == second_usbsrc->bus &&
first_usbsrc->device == second_usbsrc->device)
return 1;
@@ -24345,10 +24395,15 @@ virDomainHostdevDefFormatSubsysUSB(virBuffer *buf,
virBufferAsprintf(&sourceChildBuf, "<product
id='0x%.4x'/>\n", usbsrc->product);
}
- if (usbsrc->bus || usbsrc->device)
+ if (usbsrc->bus && usbsrc->port) {
+ virBufferAsprintf(&sourceChildBuf, "<address %sbus='%d'
port='%s'/>\n",
+ includeTypeInAddr ? "type='usb' " :
"",
+ usbsrc->bus, usbsrc->port);
+ } else if (usbsrc->bus || usbsrc->device) {
virBufferAsprintf(&sourceChildBuf, "<address %sbus='%d'
device='%d'/>\n",
includeTypeInAddr ? "type='usb' " :
"",
usbsrc->bus, usbsrc->device);
+ }
virXMLFormatElement(buf, "source", &sourceAttrBuf,
&sourceChildBuf);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 58b97a2b54..aaa8b8573a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -228,6 +228,7 @@ struct _virDomainHostdevSubsysUSB {
on vendor/product */
unsigned bus;
unsigned device;
+ char *port;
unsigned vendor;
unsigned product;
diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 0a1d8500d4..9e77477a9b 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -1358,6 +1358,42 @@ virHostdevMarkUSBDevices(virHostdevManager *mgr,
return -1;
}
+static int
+virHostdevFindUSBDeviceWithFlags(virDomainHostdevDef *hostdev,
+ bool mandatory,
+ unsigned int flags,
+ virUSBDevice **usb)
+{
+ virDomainHostdevSubsysUSB *usbsrc = &hostdev->source.subsys.u.usb;
+ unsigned vendor = usbsrc->vendor;
+ unsigned product = usbsrc->product;
+ unsigned bus = usbsrc->bus;
+ char *port = usbsrc->port;
+ unsigned device = usbsrc->device;
+ virUSBDeviceList *devs;
g_autoptr(virUSBDeviceList) devs = NULL;
+ int rc;
+
+ rc = virUSBDeviceFind(vendor, product, bus, device, port, NULL,
+ mandatory, flags, &devs);
+ if (rc < 0)
+ return -1;
+
+ if (rc == 1) {
+ *usb = virUSBDeviceListGet(devs, 0);
+ virUSBDeviceListSteal(devs, *usb);
+ }
+ virObjectUnref(devs);
+
+ if (rc > 1) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Multiple USB devices for %1$x:%2$x, use <address>
to specify one"),
+ vendor, product);
+ return -1;
+ }
+
+ return rc;
+}
+
int
virHostdevFindUSBDevice(virDomainHostdevDef *hostdev,
@@ -1366,77 +1402,64 @@ virHostdevFindUSBDevice(virDomainHostdevDef *hostdev,
{
virDomainHostdevSubsysUSB *usbsrc = &hostdev->source.subsys.u.usb;
unsigned vendor = usbsrc->vendor;
- unsigned product = usbsrc->product;
unsigned bus = usbsrc->bus;
unsigned device = usbsrc->device;
+ char *port = usbsrc->port;
bool autoAddress = usbsrc->autoAddress;
+ unsigned int flags = 0;
int rc;
*usb = NULL;
- if (vendor && bus) {
- rc = virUSBDeviceFind(vendor, product, bus, device,
- NULL,
- autoAddress ? false : mandatory,
- usb);
- if (rc < 0) {
- return -1;
- } else if (!autoAddress) {
- goto out;
- } else {
- VIR_INFO("USB device %x:%x could not be found at previous"
- " address (bus:%u device:%u)",
- vendor, product, bus, device);
- }
+ if (vendor)
+ flags |= USB_DEVICE_FIND_BY_VENDOR;
+ if (device)
+ flags |= USB_DEVICE_FIND_BY_DEVICE;
+ if (port)
+ flags |= USB_DEVICE_FIND_BY_PORT;
+
+ /* Rule out invalid cases. */
+ if (vendor && device && port) {
+ VIR_WARN("Cannot match USB device on vendor/product, bus/device, and
bus/port at once. Ignoring bus/device.");
+ flags &= ~((unsigned int) USB_DEVICE_FIND_BY_DEVICE);
This typecasting is unnecessary. These values are already declared as
non-negative values. And in C99 enums must fit into int. Everywhere else
in our code we clear flags without typecasting.
+ } else if (device && port) {
+ VIR_WARN("Cannot match USB device on bus/device and bus/port at once.
Ignoring bus/device.");
+ flags &= ~((unsigned int) USB_DEVICE_FIND_BY_DEVICE);
+ } else if (!vendor && !device && !port) {
+ VIR_WARN("No matching fields for USB device found. Vendor/product,
bus/device, or bus/port required.");
+ return -1;
Since this returns an error the VIR_WARN() should be promoted to
virReportError().
}
- /* When vendor is specified, its USB address is either unspecified or the
- * device could not be found at the USB device where it had been
- * automatically found before.
- */
- if (vendor) {
- g_autoptr(virUSBDeviceList) devs = NULL;
+ /* First attempt, matching on all valid fields. */
+ rc = virHostdevFindUSBDeviceWithFlags(hostdev,
+ autoAddress ? false : mandatory,
+ flags, usb);
+ if (rc < 0)
+ return -1;
- rc = virUSBDeviceFindByVendor(vendor, product, NULL, mandatory, &devs);
- if (rc < 0) {
- return -1;
- } else if (rc == 0) {
- goto out;
- } else if (rc > 1) {
- if (autoAddress) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("Multiple USB devices for %1$x:%2$x were found,
but none of them is at bus:%3$u device:%4$u"),
- vendor, product, bus, device);
- } else {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("Multiple USB devices for %1$x:%2$x, use
<address> to specify one"),
- vendor, product);
- }
+ if (rc != 1 && autoAddress && device) {
+ VIR_INFO("USB device could not be found at previous address "
+ "(bus:%u device:%u)", bus, device);
+
+ /* Second attempt, for when the device number has changed. */
+ flags &= ~((unsigned int) USB_DEVICE_FIND_BY_DEVICE);
+ usbsrc->device = 0;
+
+ rc = virHostdevFindUSBDeviceWithFlags(hostdev, mandatory,
+ flags, usb);
+ if (rc < 0)
return -1;
- }
- *usb = virUSBDeviceListGet(devs, 0);
- virUSBDeviceListSteal(devs, *usb);
+ usbsrc->autoAddress = true;
+ }
+ if (!*usb) {
+ hostdev->missing = true;
+ } else if (!usbsrc->bus || !usbsrc->device) {
usbsrc->bus = virUSBDeviceGetBus(*usb);
usbsrc->device = virUSBDeviceGetDevno(*usb);
- usbsrc->autoAddress = true;
-
- if (autoAddress) {
- VIR_INFO("USB device %x:%x found at bus:%u device:%u (moved"
- " from bus:%u device:%u)",
- vendor, product,
- usbsrc->bus, usbsrc->device,
- bus, device);
- }
- } else if (bus) {
- if (virUSBDeviceFindByBus(bus, device, NULL, mandatory, usb) < 0)
- return -1;
}
- out:
- if (!*usb)
- hostdev->missing = true;
return 0;
}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a8ebf9efd8..cd7d00674f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3679,8 +3679,6 @@ virURIResolveAlias;
# util/virusb.h
virUSBDeviceFileIterate;
virUSBDeviceFind;
-virUSBDeviceFindByBus;
-virUSBDeviceFindByVendor;
virUSBDeviceFree;
virUSBDeviceGetBus;
virUSBDeviceGetDevno;
diff --git a/src/util/virusb.c b/src/util/virusb.c
index cf3461f80a..8aca190c4c 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -61,12 +61,6 @@ struct _virUSBDeviceList {
virUSBDevice **devs;
};
-typedef enum {
- USB_DEVICE_ALL = 0,
- USB_DEVICE_FIND_BY_VENDOR = 1 << 0,
- USB_DEVICE_FIND_BY_BUS = 1 << 1,
-} virUSBDeviceFindFlags;
-
static virClass *virUSBDeviceListClass;
static void virUSBDeviceListDispose(void *obj);
@@ -102,11 +96,27 @@ static int virUSBSysReadFile(const char *f_name, const char
*d_name,
return 0;
}
+static int virUSBSysReadFileStr(const char *f_name, const char *d_name,
+ char **value)
+{
+ char *buf = NULL;
+ g_autofree char *filename = NULL;
+
+ filename = g_strdup_printf(USB_SYSFS "/devices/%s/%s", d_name, f_name);
+
+ if (virFileReadAll(filename, 1024, &buf) < 0)
+ return -1;
+
+ *value = buf;
+ return 0;
+}
+
static virUSBDeviceList *
virUSBDeviceSearch(unsigned int vendor,
unsigned int product,
unsigned int bus,
unsigned int devno,
+ const char *port,
const char *vroot,
unsigned int flags)
{
@@ -127,6 +137,8 @@ virUSBDeviceSearch(unsigned int vendor,
while ((direrr = virDirRead(dir, &de, USB_SYSFS "/devices")) > 0)
{
unsigned int found_prod, found_vend, found_bus, found_devno;
+ char *found_port;
+ bool port_matches;
char *tmpstr = de->d_name;
if (strchr(de->d_name, ':'))
@@ -154,16 +166,32 @@ virUSBDeviceSearch(unsigned int vendor,
10, &found_devno) < 0)
goto cleanup;
- if ((flags & USB_DEVICE_FIND_BY_VENDOR) &&
- (found_prod != product || found_vend != vendor))
- continue;
+ if (virUSBSysReadFileStr("devpath", de->d_name,
+ &found_port) < 0) {
+ goto cleanup;
+ } else {
+ found_port[strlen(found_port) - 1] = '\0'; /* remove newline */
virStringTrimOptionalNewline()
+ port_matches = STREQ_NULLABLE(found_port, port);
+ VIR_FREE(found_port);
+ }
- if (flags & USB_DEVICE_FIND_BY_BUS) {
+ if (flags & USB_DEVICE_FIND_BY_VENDOR) {
+ if (found_prod != product || found_vend != vendor)
+ continue;
+ }
+
+ if (flags & USB_DEVICE_FIND_BY_DEVICE) {
if (found_bus != bus || found_devno != devno)
continue;
found = true;
}
+ if (flags & USB_DEVICE_FIND_BY_PORT) {
+ if (found_bus != bus || !port_matches)
+ continue;
+ found = true;
+ }
+
usb = virUSBDeviceNew(found_bus, found_devno, vroot);
if (!usb)
Michal