Re: [PATCH 2/4] domain_conf, virhostdev, virusb, virusb test: add bus/port matching

On 4/21/25 21:38, Maximilian Martin via Devel wrote:

From: Maximilian Martin <maximilian_martin@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@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