On Mon, Jun 23, 2025 at 04:11:30PM +0200, Michal Prívozník via Devel wrote:
> On 4/21/25 21:38, Maximilian Martin via Devel wrote:
> > From: Maximilian Martin <maximilian_mar...@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_mar...@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().
I'm wondering why we're ignoring invalid input for the previous
two cases too ? Quietly ignoring incorrect user specified data
is generally an anti-pattern we aim to avoid.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
