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(a)gmx.de&gt; > > 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&gt; > --- > 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().