On 06/14/2013 02:43 PM, david.waring@xxxxxxxxxxxx wrote:
> From: David Waring <davidjw@xxxxxxxxxxxx>
The commit message is missing and the title is misleading since we knew how to
find the devices by vendor and product before.
Please put it here instead of the cover letter, since cover letters don't get
into git.
>
> ---
> docs/formatdomain.html.in | 21 +++++++++++---------
> src/conf/domain_conf.c | 18 +++++++++++++++--
> src/conf/domain_conf.h | 1 +
RNG schema in docs/schemas will need an update too.
It would be nice to test the new element in tests/domainschematest too.
> src/conf/node_device_conf.c | 5 +++++
> src/conf/node_device_conf.h | 1 +
> src/lxc/lxc_hostdev.c | 5 +++--
> src/qemu/qemu_hostdev.c | 5 +++--
> src/util/virusb.c | 47 +++++++++++++++++++++++++++++++++++++++++----
> src/util/virusb.h | 2 ++
> 9 files changed, 86 insertions(+), 19 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2373397..1ef775d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3541,6 +3541,16 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node,
> "%s", _("usb product needs id"));
> goto out;
> }
> + } else if (xmlStrEqual(cur->name, BAD_CAST "serial")) {
> + char* serial = virXMLPropString(cur, "id");
> +
> + if (serial) {
> + def->source.subsys.u.usb.serial = serial;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("usb serial needs id"));
> + goto out;
> + }
The check later in this function should be extended to reject serial id if no
vendor or product were specified.
For vendor and product, there are numeric ids and string names (which cannot
be used here), but for serial there is just the string. How about:
<serial>012345678</serial> instead of <serial id='012345678'/>?
virXMLPropString allocates memory that needs to be freed in
virDomainHostdevDefClear.
> } else if (xmlStrEqual(cur->name, BAD_CAST "address")) {
> char *bus, *device;
>
> @@ -9532,9 +9542,10 @@ virDomainHostdevMatchSubsysUSB(virDomainHostdevDefPtr a,
> a->source.subsys.u.usb.device == b->source.subsys.u.usb.device)
> return 1;
> } else {
> - /* specified by product & vendor id */
> + /* specified by product, vendor id and optionally serial number */
> if (a->source.subsys.u.usb.product == b->source.subsys.u.usb.product &&
> - a->source.subsys.u.usb.vendor == b->source.subsys.u.usb.vendor)
> + a->source.subsys.u.usb.vendor == b->source.subsys.u.usb.vendor &&
> + STREQ_NULLABLE(a->source.subsys.u.usb.serial, b->source.subsys.u.usb.serial))
> return 1;
> }
> return 0;
> @@ -14343,6 +14354,9 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
> def->source.subsys.u.usb.vendor);
> virBufferAsprintf(buf, "<product id='0x%.4x'/>\n",
> def->source.subsys.u.usb.product);
> + if (def->source.subsys.u.usb.serial != NULL)
> + virBufferAsprintf(buf, "<serial id='%s' />\n",
> + def->source.subsys.u.usb.serial);
If you use virBufferEscapeString it will escape any characters that aren't
XML-safe and you don't have to check it for NULL (virBufferEscapeString prints
nothing if the string is NULL).
> }
> if (def->source.subsys.u.usb.bus ||
> def->source.subsys.u.usb.device) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5b159ac..d1086f4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -413,6 +413,7 @@ struct _virDomainHostdevSubsys {
>
> unsigned vendor;
> unsigned product;
> + char *serial;
> } usb;
> struct {
> virDevicePCIAddress addr; /* host address */
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 4eeb3b3..7a56cc3 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -347,6 +347,9 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def)
> data->usb_dev.vendor_name);
> else
> virBufferAddLit(&buf, " />\n");
> + if (data->usb_dev.serial)
This if is not necessary.
> + virBufferEscapeString(&buf, " <serial id='%s' />\n",
> + data->usb_dev.serial);
> break;
> case VIR_NODE_DEV_CAP_USB_INTERFACE:
> virBufferAsprintf(&buf, " <number>%d</number>\n",
> @@ -952,6 +955,7 @@ virNodeDevCapUsbDevParseXML(xmlXPathContextPtr ctxt,
>
> data->usb_dev.vendor_name = virXPathString("string(./vendor[1])", ctxt);
> data->usb_dev.product_name = virXPathString("string(./product[1])", ctxt);
> + data->usb_dev.serial = virXPathString("string(./serial[1]/@id)", ctxt);
>
> ret = 0;
> out:
> @@ -1383,6 +1387,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> case VIR_NODE_DEV_CAP_USB_DEV:
> VIR_FREE(data->usb_dev.product_name);
> VIR_FREE(data->usb_dev.vendor_name);
> + VIR_FREE(data->usb_dev.serial);
> break;
> case VIR_NODE_DEV_CAP_USB_INTERFACE:
> VIR_FREE(data->usb_if.description);
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 1c5855c..2c14d63 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -118,6 +118,7 @@ struct _virNodeDevCapsDef {
> unsigned int vendor;
> char *product_name;
> char *vendor_name;
> + char *serial;
> } usb_dev;
> struct {
> unsigned int number;
> diff --git a/src/lxc/lxc_hostdev.c b/src/lxc/lxc_hostdev.c
> index 257e93b..ec931bc 100644
> --- a/src/lxc/lxc_hostdev.c
> +++ b/src/lxc/lxc_hostdev.c
> @@ -127,6 +127,7 @@ virLXCFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> {
> unsigned vendor = hostdev->source.subsys.u.usb.vendor;
> unsigned product = hostdev->source.subsys.u.usb.product;
> + const char *serial = hostdev->source.subsys.u.usb.serial;
> unsigned bus = hostdev->source.subsys.u.usb.bus;
> unsigned device = hostdev->source.subsys.u.usb.device;
> bool autoAddress = hostdev->source.subsys.u.usb.autoAddress;
> @@ -135,7 +136,7 @@ virLXCFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> *usb = NULL;
>
> if (vendor && bus) {
Now that we search by serial too, the debug and error messages need to include
it as well.
> - rc = virUSBDeviceFind(vendor, product, bus, device,
> + rc = virUSBDeviceFind(vendor, product, serial, bus, device,
> NULL,
> autoAddress ? false : mandatory,
> usb);
> @@ -157,7 +158,7 @@ virLXCFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> if (vendor) {
> virUSBDeviceList *devs;
>
> - rc = virUSBDeviceFindByVendor(vendor, product,
> + rc = virUSBDeviceFindByVendor(vendor, product, serial,
Trailing space at the end of line (this breaks 'make syntax-check')
> NULL,
> mandatory, &devs);
> if (rc < 0)
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 9013f60..d8582d7 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -722,6 +722,7 @@ qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> {
> unsigned vendor = hostdev->source.subsys.u.usb.vendor;
> unsigned product = hostdev->source.subsys.u.usb.product;
> + const char *serial = hostdev->source.subsys.u.usb.serial;
> unsigned bus = hostdev->source.subsys.u.usb.bus;
> unsigned device = hostdev->source.subsys.u.usb.device;
> bool autoAddress = hostdev->source.subsys.u.usb.autoAddress;
> @@ -730,7 +731,7 @@ qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> *usb = NULL;
>
> if (vendor && bus) {
> - rc = virUSBDeviceFind(vendor, product, bus, device,
> + rc = virUSBDeviceFind(vendor, product, serial, bus, device,
> NULL,
> autoAddress ? false : mandatory,
> usb);
Same goes for the debug/error messages here.
> @@ -752,7 +753,7 @@ qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> if (vendor) {
> virUSBDeviceListPtr devs;
>
> - rc = virUSBDeviceFindByVendor(vendor, product, NULL, mandatory, &devs);
> + rc = virUSBDeviceFindByVendor(vendor, product, serial, NULL, mandatory, &devs);
> if (rc < 0)
> return -1;
>
> diff --git a/src/util/virusb.c b/src/util/virusb.c
> index d34e44f..5ca396e 100644
> --- a/src/util/virusb.c
> +++ b/src/util/virusb.c
> @@ -54,6 +54,7 @@ struct _virUSBDevice {
>
> char name[USB_ADDR_LEN]; /* domain:bus:slot.function */
> char id[USB_ID_LEN]; /* product vendor */
> + char *serial; /* serial number */
> char *path;
> const char *used_by; /* name of the domain using this dev */
> };
> @@ -87,6 +88,30 @@ static int virUSBOnceInit(void)
>
> VIR_ONCE_GLOBAL_INIT(virUSB)
>
> +static int virUSBSysReadFileString(const char *f_name, const char *d_name, char **buf)
> +{
> + int ret = -1, tmp;
> + char *filename = NULL;
> +
> + tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name, f_name);
> + if (tmp < 0) {
This might look nicer without the tmp variable.
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + tmp = virFileReadAll(filename, 1024, buf);
This fills my log with errors for all the USB devices without serial numbers.
if (!virFileExists(filename)) {
*buf = NULL;
goto cleanup;
}
> + if (tmp < 0)
> + goto cleanup;
> +
> + if (tmp > 0 && (*buf)[tmp-1] == '\n')
> + (*buf)[tmp-1]=0;
Missing spaces around '-' and '='.
This might look nicer if we rename the 'buf' parameter to 'value', making buf
the local variable for the file contents and then:
if (VIR_STRNDUP(*value, buf, strchrnul(buf, '\n')) < 0)
goto cleanup;
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(filename);
> + return ret;
> +}
> +
> static int virUSBSysReadFile(const char *f_name, const char *d_name,
> int base, unsigned int *value)
> {
> @@ -120,6 +145,7 @@ cleanup:
> static virUSBDeviceListPtr
> virUSBDeviceSearch(unsigned int vendor,
> unsigned int product,
> + const char *serial,
> unsigned int bus,
> unsigned int devno,
> const char *vroot,
> @@ -128,6 +154,7 @@ virUSBDeviceSearch(unsigned int vendor,
> DIR *dir = NULL;
> bool found = false;
> char *ignore = NULL;
> + char *found_serial = NULL;
> struct dirent *de;
> virUSBDeviceListPtr list = NULL, ret = NULL;
> virUSBDevicePtr usb;
> @@ -158,6 +185,10 @@ virUSBDeviceSearch(unsigned int vendor,
> 16, &found_prod) < 0)
> goto cleanup;
>
> + VIR_FREE(found_serial);
> + found_serial=NULL;
Redundant, VIR_FREE sets it to NULL already.
> + virUSBSysReadFileString("serial", de->d_name, &found_serial);
> +
> if (STRPREFIX(de->d_name, "usb"))
> tmpstr += 3;
>
> @@ -173,7 +204,8 @@ virUSBDeviceSearch(unsigned int vendor,
> goto cleanup;
>
> if ((flags & USB_DEVICE_FIND_BY_VENDOR) &&
> - (found_prod != product || found_vend != vendor))
> + (found_prod != product || found_vend != vendor ||
> + (serial != NULL && !STREQ_NULLABLE(found_serial, serial))))
> continue;
>
> if (flags & USB_DEVICE_FIND_BY_BUS) {
> @@ -197,6 +229,8 @@ virUSBDeviceSearch(unsigned int vendor,
> ret = list;
>
> cleanup:
> + VIR_FREE(found_serial);
> +
> if (dir) {
> int saved_errno = errno;
> closedir(dir);
> @@ -211,6 +245,7 @@ cleanup:
> int
> virUSBDeviceFindByVendor(unsigned int vendor,
> unsigned int product,
> + const char *serial,
All these functions should report the serial id in the debug/error messages.
> const char *vroot,
> bool mandatory,
> virUSBDeviceListPtr *devices)
Jan
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list