[libvirt] [PATCH 0/6] Enhance save/restore/migration of domains with USB devices
USB devices can disappear without OS being mad about it, which makes them ideal for startupPolicy so that a domain with USB devices can be migrated to a host that lacks the USB devices. Moreover, this series allows USB devices to be re-plugged or a host to be rebooted while domains with USB devices are (managed)saved. Jiri Denemark (6): conf: Add support for startupPolicy for USB devices qemu: Introduce qemuFindHostdevUSBDevice qemu: Add option to treat missing USB devices as success qemu: Implement startupPolicy for USB passed through devices Add MIGRATABLE flag for virDomainGetXMLDesc qemu: Make save/restore with USB devices usable docs/formatdomain.html.in | 28 ++++++-- docs/schemas/domaincommon.rng | 3 + include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 45 ++++++++++++- src/conf/domain_conf.h | 4 ++ src/qemu/qemu_cgroup.c | 2 + src/qemu/qemu_command.c | 20 ++++-- src/qemu/qemu_domain.c | 21 +++--- src/qemu/qemu_domain.h | 11 ++-- src/qemu/qemu_driver.c | 19 +++--- src/qemu/qemu_hostdev.c | 137 +++++++++++++++++++++++++++++----------- src/qemu/qemu_hostdev.h | 6 +- src/qemu/qemu_hotplug.c | 33 +--------- src/qemu/qemu_migration.c | 12 ++-- src/qemu/qemu_process.c | 16 ++--- src/security/security_dac.c | 16 +++-- src/security/security_selinux.c | 16 +++-- src/util/hostusb.c | 91 ++++++++++++++++++-------- src/util/hostusb.h | 22 ++++--- tools/virsh-domain.c | 4 ++ tools/virsh.pod | 7 +- 21 files changed, 351 insertions(+), 163 deletions(-) -- 1.7.12
USB devices can disappear without OS being mad about it, which makes them ideal for startupPolicy. With this attribute, USB devices can be configured to be mandatory (the default), requisite (will disappear during migration if they cannot be found), or completely optional. --- docs/formatdomain.html.in | 28 ++++++++++++++++++++++++---- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 32 +++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 1 + 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3cc6c51..44fa0d9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2056,7 +2056,7 @@ ... <devices> <hostdev mode='subsystem' type='usb'> - <source> + <source startupPolicy='optional'> <vendor id='0x1234'/> <product id='0xbeef'/> </source> @@ -2100,9 +2100,29 @@ <dd>The source element describes the device as seen from the host. The USB device can either be addressed by vendor / product id using the <code>vendor</code> and <code>product</code> elements or by the device's - address on the hosts using the <code>address</code> element. - PCI devices on the other hand can only be described by their - <code>address</code></dd> + address on the hosts using the <code>address</code> element. PCI devices + on the other hand can only be described by their <code>address</code>. + + <span class="since">Since 0.10.3</span>, the <code>source</code> element + of USB devices may contain <code>startupPolicy</code> attribute which can + be used to define policy what to do if the specified host USB device is + not found. The attribute accepts the following values: + <table class="top_table"> + <tr> + <td> mandatory </td> + <td> fail if missing for any reason (the default) </td> + </tr> + <tr> + <td> requisite </td> + <td> fail if missing on boot up, + drop if missing on migrate/restore/revert </td> + </tr> + <tr> + <td> optional </td> + <td> drop if missing at any start attempt </td> + </tr> + </table> + </dd> <dt><code>vendor</code>, <code>product</code></dt> <dd>The <code>vendor</code> and <code>product</code> elements each have an <code>id</code> attribute that specifies the USB vendor and product id. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..38dab7e 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2763,6 +2763,9 @@ </optional> <group> <element name="source"> + <optional> + <ref name="startupPolicy"/> + </optional> <choice> <group> <ref name="usbproduct"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..da2d86d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2694,6 +2694,20 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, int ret = -1; int got_product, got_vendor; xmlNodePtr cur; + char *startupPolicy = NULL; + + if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) { + def->startupPolicy = + virDomainStartupPolicyTypeFromString(startupPolicy); + if (def->startupPolicy <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown startup policy '%s'"), + startupPolicy); + VIR_FREE(startupPolicy); + goto out; + } + VIR_FREE(startupPolicy); + } /* Product can validly be 0, so we need some extra help to determine * if it is uninitialized*/ @@ -2957,6 +2971,15 @@ virDomainHostdevPartsParse(xmlNodePtr node, _("Missing <source> element in hostdev device")); goto error; } + + if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && + virXPathBoolean("boolean(./source/@startupPolicy)", ctxt)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting startupPolicy is only allowed for USB" + " devices")); + goto error; + } + switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (virDomainHostdevSubsysPciDefParseXML(sourcenode, def, flags) < 0) @@ -12043,7 +12066,14 @@ virDomainHostdevSourceFormat(virBufferPtr buf, unsigned int flags, bool includeTypeInAddr) { - virBufferAddLit(buf, "<source>\n"); + virBufferAddLit(buf, "<source"); + if (def->startupPolicy) { + const char *policy; + policy = virDomainStartupPolicyTypeToString(def->startupPolicy); + virBufferAsprintf(buf, " startupPolicy='%s'", policy); + } + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); switch (def->source.subsys.type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..fa5d1f4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -384,6 +384,7 @@ struct _virDomainHostdevSubsys { struct _virDomainHostdevDef { virDomainDeviceDef parent; /* higher level Def containing this */ int mode; /* enum virDomainHostdevMode */ + int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int managed : 1; union { virDomainHostdevSubsys subsys; -- 1.7.12
On Tue, Oct 09, 2012 at 02:13:22PM +0200, Jiri Denemark wrote:
USB devices can disappear without OS being mad about it, which makes them ideal for startupPolicy. With this attribute, USB devices can be configured to be mandatory (the default), requisite (will disappear during migration if they cannot be found), or completely optional. --- docs/formatdomain.html.in | 28 ++++++++++++++++++++++++---- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 32 +++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 1 + 4 files changed, 59 insertions(+), 5 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The code which looks up a USB device specified by hostdev is duplicated in two places. This patch creates a dedicated function that can be called in both places. --- src/qemu/qemu_hostdev.c | 73 +++++++++++++++++++++++++++---------------------- src/qemu/qemu_hostdev.h | 1 + src/qemu/qemu_hotplug.c | 33 +--------------------- 3 files changed, 42 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 7375d26..017f0ec 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -641,6 +641,44 @@ error: return -1; } +usbDevice * +qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev) +{ + usbDevice *usb = NULL; + unsigned vendor = hostdev->source.subsys.u.usb.vendor; + unsigned product = hostdev->source.subsys.u.usb.product; + unsigned bus = hostdev->source.subsys.u.usb.bus; + unsigned device = hostdev->source.subsys.u.usb.device; + + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device); + + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); + if (!devs) + return NULL; + + if (usbDeviceListCount(devs) > 1) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple USB devices for %x:%x, " + "use <address> to specify one"), vendor, product); + usbDeviceListFree(devs); + return NULL; + } + usb = usbDeviceListGet(devs, 0); + usbDeviceListSteal(devs, usb); + usbDeviceListFree(devs); + + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + + } else if (!vendor && bus) { + usb = usbFindDeviceByBus(bus, device); + } + + return usb; +} + static int qemuPrepareHostUSBDevices(struct qemud_driver *driver, virDomainDefPtr def) @@ -663,45 +701,14 @@ qemuPrepareHostUSBDevices(struct qemud_driver *driver, */ for (i = 0 ; i < nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; - usbDevice *usb = NULL; + usbDevice *usb; if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue; - unsigned vendor = hostdev->source.subsys.u.usb.vendor; - unsigned product = hostdev->source.subsys.u.usb.product; - unsigned bus = hostdev->source.subsys.u.usb.bus; - unsigned device = hostdev->source.subsys.u.usb.device; - - if (vendor && bus) { - usb = usbFindDevice(vendor, product, bus, device); - - } else if (vendor && !bus) { - usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); - if (!devs) - goto cleanup; - - if (usbDeviceListCount(devs) > 1) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple USB devices for %x:%x, " - "use <address> to specify one"), vendor, product); - usbDeviceListFree(devs); - goto cleanup; - } - usb = usbDeviceListGet(devs, 0); - usbDeviceListSteal(devs, usb); - usbDeviceListFree(devs); - - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); - - } else if (!vendor && bus) { - usb = usbFindDeviceByBus(bus, device); - } - - if (!usb) + if (!(usb = qemuFindHostdevUSBDevice(hostdev))) goto cleanup; if (usbDeviceListAdd(list, usb) < 0) { diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 14f1fca..204b2d4 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -36,6 +36,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, const unsigned char *uuid, virDomainHostdevDefPtr *hostdevs, int nhostdevs); +usbDevice *qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev); int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, usbDeviceList *list); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a738b19..59de8e4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1149,38 +1149,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto cleanup; if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - unsigned vendor = hostdev->source.subsys.u.usb.vendor; - unsigned product = hostdev->source.subsys.u.usb.product; - unsigned bus = hostdev->source.subsys.u.usb.bus; - unsigned device = hostdev->source.subsys.u.usb.device; - - if (vendor && bus) { - usb = usbFindDevice(vendor, product, bus, device); - - } else if (vendor && !bus) { - usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); - if (!devs) - goto cleanup; - - if (usbDeviceListCount(devs) > 1) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple USB devices for %x:%x, " - "use <address> to specify one"), vendor, product); - usbDeviceListFree(devs); - goto cleanup; - } - usb = usbDeviceListGet(devs, 0); - usbDeviceListSteal(devs, usb); - usbDeviceListFree(devs); - - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); - - } else if (!vendor && bus) { - usb = usbFindDeviceByBus(bus, device); - } - - if (!usb) + if (!(usb = qemuFindHostdevUSBDevice(hostdev))) goto cleanup; if (usbDeviceListAdd(list, usb) < 0) { -- 1.7.12
On Tue, Oct 09, 2012 at 02:13:23PM +0200, Jiri Denemark wrote:
The code which looks up a USB device specified by hostdev is duplicated in two places. This patch creates a dedicated function that can be called in both places. --- src/qemu/qemu_hostdev.c | 73 +++++++++++++++++++++++++++---------------------- src/qemu/qemu_hostdev.h | 1 + src/qemu/qemu_hotplug.c | 33 +--------------------- 3 files changed, 42 insertions(+), 65 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
All USB device lookup functions emit an error when they cannot find the requested device. With this patch, their caller can choose if a missing device is an error or normal condition. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_hostdev.c | 57 ++++++++++++++++---------- src/qemu/qemu_hostdev.h | 4 +- src/qemu/qemu_hotplug.c | 2 +- src/security/security_dac.c | 16 ++++++-- src/security/security_selinux.c | 16 ++++++-- src/util/hostusb.c | 91 ++++++++++++++++++++++++++++++----------- src/util/hostusb.h | 22 ++++++---- 8 files changed, 145 insertions(+), 64 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fa5d1f4..3cb1193 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -386,6 +386,7 @@ struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int managed : 1; + unsigned int missing : 1; union { virDomainHostdevSubsys subsys; struct { diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 017f0ec..32c858b 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -641,42 +641,57 @@ error: return -1; } -usbDevice * -qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev) +int +qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev, + bool mandatory, + usbDevice **usb) { - usbDevice *usb = NULL; unsigned vendor = hostdev->source.subsys.u.usb.vendor; unsigned product = hostdev->source.subsys.u.usb.product; unsigned bus = hostdev->source.subsys.u.usb.bus; unsigned device = hostdev->source.subsys.u.usb.device; + int rc; - if (vendor && bus) { - usb = usbFindDevice(vendor, product, bus, device); + *usb = NULL; + if (vendor && bus) { + rc = usbFindDevice(vendor, product, bus, device, mandatory, usb); + if (rc < 0) + return -1; } else if (vendor && !bus) { - usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); - if (!devs) - return NULL; + usbDeviceList *devs; - if (usbDeviceListCount(devs) > 1) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple USB devices for %x:%x, " - "use <address> to specify one"), vendor, product); - usbDeviceListFree(devs); - return NULL; + rc = usbFindDeviceByVendor(vendor, product, mandatory, &devs); + if (rc < 0) + return -1; + + if (rc == 1) { + *usb = usbDeviceListGet(devs, 0); + usbDeviceListSteal(devs, *usb); } - usb = usbDeviceListGet(devs, 0); - usbDeviceListSteal(devs, usb); usbDeviceListFree(devs); - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + if (rc == 0) { + goto out; + } else if (rc > 1) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple USB devices for %x:%x, " + "use <address> to specify one"), + vendor, product); + return -1; + } + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(*usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(*usb); } else if (!vendor && bus) { - usb = usbFindDeviceByBus(bus, device); + if (usbFindDeviceByBus(bus, device, mandatory, usb) < 0) + return -1; } - return usb; +out: + if (!*usb) + hostdev->missing = 1; + return 0; } static int @@ -708,7 +723,7 @@ qemuPrepareHostUSBDevices(struct qemud_driver *driver, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue; - if (!(usb = qemuFindHostdevUSBDevice(hostdev))) + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) goto cleanup; if (usbDeviceListAdd(list, usb) < 0) { diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 204b2d4..74dd2ce 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -36,7 +36,9 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, const unsigned char *uuid, virDomainHostdevDefPtr *hostdevs, int nhostdevs); -usbDevice *qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev); +int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev, + bool mandatory, + usbDevice **usb); int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, usbDeviceList *list); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 59de8e4..2ff9e7a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1149,7 +1149,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto cleanup; if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (!(usb = qemuFindHostdevUSBDevice(hostdev))) + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) goto cleanup; if (usbDeviceListAdd(list, usb) < 0) { diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 22edba2..f126aa5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -495,9 +495,13 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, switch (dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - usbDevice *usb = usbGetDevice(dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device); + usbDevice *usb; + if (dev->missing) + return 0; + + usb = usbGetDevice(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); if (!usb) goto done; @@ -568,9 +572,13 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, switch (dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - usbDevice *usb = usbGetDevice(dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device); + usbDevice *usb; + + if (dev->missing) + return 0; + usb = usbGetDevice(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); if (!usb) goto done; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 883a82b..d55c60d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1101,9 +1101,13 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN switch (dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - usbDevice *usb = usbGetDevice(dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device); + usbDevice *usb; + if (dev->missing) + return 0; + + usb = usbGetDevice(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); if (!usb) goto done; @@ -1174,9 +1178,13 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUT switch (dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - usbDevice *usb = usbGetDevice(dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device); + usbDevice *usb; + + if (dev->missing) + return 0; + usb = usbGetDevice(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); if (!usb) goto done; diff --git a/src/util/hostusb.c b/src/util/hostusb.c index f0196a8..81a9f5a 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -186,76 +186,117 @@ cleanup: return ret; } -usbDeviceList * -usbFindDeviceByVendor(unsigned int vendor, unsigned product) +int +usbFindDeviceByVendor(unsigned int vendor, + unsigned product, + bool mandatory, + usbDeviceList **devices) { - usbDeviceList *list; + int count; + if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR))) - return NULL; + return -1; if (list->count == 0) { + usbDeviceListFree(list); + if (!mandatory) { + VIR_DEBUG("Did not find USB device %x:%x", + vendor, product); + if (devices) + *devices = NULL; + return 0; + } + virReportError(VIR_ERR_INTERNAL_ERROR, _("Did not find USB device %x:%x"), vendor, product); - usbDeviceListFree(list); - return NULL; + return -1; } - return list; + count = list->count; + if (devices) + *devices = list; + else + usbDeviceListFree(list); + + return count; } -usbDevice * -usbFindDeviceByBus(unsigned int bus, unsigned devno) +int +usbFindDeviceByBus(unsigned int bus, + unsigned devno, + bool mandatory, + usbDevice **usb) { - usbDevice *usb; usbDeviceList *list; if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS))) - return NULL; + return -1; if (list->count == 0) { + usbDeviceListFree(list); + if (!mandatory) { + VIR_DEBUG("Did not find USB device bus:%u device:%u", + bus, devno); + if (usb) + *usb = NULL; + return 0; + } + virReportError(VIR_ERR_INTERNAL_ERROR, _("Did not find USB device bus:%u device:%u"), bus, devno); - usbDeviceListFree(list); - return NULL; + return -1; } - usb = usbDeviceListGet(list, 0); - usbDeviceListSteal(list, usb); + if (usb) { + *usb = usbDeviceListGet(list, 0); + usbDeviceListSteal(list, *usb); + } usbDeviceListFree(list); - return usb; + return 0; } -usbDevice * +int usbFindDevice(unsigned int vendor, unsigned int product, unsigned int bus, - unsigned int devno) + unsigned int devno, + bool mandatory, + usbDevice **usb) { - usbDevice *usb; usbDeviceList *list; unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS; if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags))) - return NULL; + return -1; if (list->count == 0) { + usbDeviceListFree(list); + if (!mandatory) { + VIR_DEBUG("Did not find USB device %x:%x bus:%u device:%u", + vendor, product, bus, devno); + if (usb) + *usb = NULL; + return 0; + } + virReportError(VIR_ERR_INTERNAL_ERROR, _("Did not find USB device %x:%x bus:%u device:%u"), vendor, product, bus, devno); - usbDeviceListFree(list); - return NULL; + return -1; } - usb = usbDeviceListGet(list, 0); - usbDeviceListSteal(list, usb); + if (usb) { + *usb = usbDeviceListGet(list, 0); + usbDeviceListSteal(list, *usb); + } usbDeviceListFree(list); - return usb; + return 0; } usbDevice * diff --git a/src/util/hostusb.h b/src/util/hostusb.h index 6f5067c..4f55fdc 100644 --- a/src/util/hostusb.h +++ b/src/util/hostusb.h @@ -31,16 +31,22 @@ typedef struct _usbDeviceList usbDeviceList; usbDevice *usbGetDevice(unsigned int bus, unsigned int devno); -usbDevice *usbFindDeviceByBus(unsigned int bus, - unsigned int devno); +int usbFindDeviceByBus(unsigned int bus, + unsigned int devno, + bool mandatory, + usbDevice **usb); -usbDeviceList *usbFindDeviceByVendor(unsigned int vendor, - unsigned int product); +int usbFindDeviceByVendor(unsigned int vendor, + unsigned int product, + bool mandatory, + usbDeviceList **devices); -usbDevice *usbFindDevice(unsigned int vendor, - unsigned int product, - unsigned int bus, - unsigned int devno); +int usbFindDevice(unsigned int vendor, + unsigned int product, + unsigned int bus, + unsigned int devno, + bool mandatory, + usbDevice **usb); void usbFreeDevice (usbDevice *dev); void usbDeviceSetUsedBy(usbDevice *dev, const char *name); -- 1.7.12
On Tue, Oct 09, 2012 at 02:13:24PM +0200, Jiri Denemark wrote:
All USB device lookup functions emit an error when they cannot find the requested device. With this patch, their caller can choose if a missing device is an error or normal condition. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_hostdev.c | 57 ++++++++++++++++---------- src/qemu/qemu_hostdev.h | 4 +- src/qemu/qemu_hotplug.c | 2 +- src/security/security_dac.c | 16 ++++++-- src/security/security_selinux.c | 16 ++++++-- src/util/hostusb.c | 91 ++++++++++++++++++++++++++++++----------- src/util/hostusb.h | 22 ++++++---- 8 files changed, 145 insertions(+), 64 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fa5d1f4..3cb1193 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -386,6 +386,7 @@ struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int managed : 1; + unsigned int missing : 1; union { virDomainHostdevSubsys subsys; struct {
I was wondering if we should actually expose this in the XML, as a read-only attribute, only shown when requesting the live XML. Otherwise there's no way for an application to determine whether the USB device they requested was actually given to the guest, without looking inside the guest. ACK to the other parts of the patch Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 10, 2012 at 16:35:05 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:24PM +0200, Jiri Denemark wrote:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fa5d1f4..3cb1193 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -386,6 +386,7 @@ struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int managed : 1; + unsigned int missing : 1; union { virDomainHostdevSubsys subsys; struct {
I was wondering if we should actually expose this in the XML, as a read-only attribute, only shown when requesting the live XML. Otherwise there's no way for an application to determine whether the USB device they requested was actually given to the guest, without looking inside the guest.
Yeah, this sounds like a useful thing. Could that be a follow up patch to avoid holding up the whole series just for this? :-)
ACK to the other parts of the patch
Jirka
On Thu, Oct 11, 2012 at 03:35:27PM +0200, Jiri Denemark wrote:
On Wed, Oct 10, 2012 at 16:35:05 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:24PM +0200, Jiri Denemark wrote:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fa5d1f4..3cb1193 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -386,6 +386,7 @@ struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int managed : 1; + unsigned int missing : 1; union { virDomainHostdevSubsys subsys; struct {
I was wondering if we should actually expose this in the XML, as a read-only attribute, only shown when requesting the live XML. Otherwise there's no way for an application to determine whether the USB device they requested was actually given to the guest, without looking inside the guest.
Yeah, this sounds like a useful thing. Could that be a follow up patch to avoid holding up the whole series just for this? :-)
As long as we do the followup now, I'm fine with it being separate. Just don't want us to release anything without it. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Oct 11, 2012 at 14:42:07 +0100, Daniel P. Berrange wrote:
On Thu, Oct 11, 2012 at 03:35:27PM +0200, Jiri Denemark wrote:
On Wed, Oct 10, 2012 at 16:35:05 +0100, Daniel P. Berrange wrote:
I was wondering if we should actually expose this in the XML, as a read-only attribute, only shown when requesting the live XML. Otherwise there's no way for an application to determine whether the USB device they requested was actually given to the guest, without looking inside the guest.
Yeah, this sounds like a useful thing. Could that be a follow up patch to avoid holding up the whole series just for this? :-)
As long as we do the followup now, I'm fine with it being separate. Just don't want us to release anything without it.
Sure, I'm already working on it. I just wanted to push this since Peter is afraid of it conflicting with his work. Jirka
--- src/qemu/qemu_cgroup.c | 2 ++ src/qemu/qemu_command.c | 20 +++++++++++++++----- src/qemu/qemu_hostdev.c | 20 +++++++++++++++----- src/qemu/qemu_hostdev.h | 3 ++- src/qemu/qemu_process.c | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 79faf8e..166f9b9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -289,6 +289,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue; + if (hostdev->missing) + continue; if ((usb = usbGetDevice(hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device)) == NULL) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f412e..d590df6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3507,17 +3507,21 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!dev->source.subsys.u.usb.bus && + if (!dev->missing && + !dev->source.subsys.u.usb.bus && !dev->source.subsys.u.usb.device) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("USB host device is missing bus/device information")); return NULL; } - virBufferAsprintf(&buf, "usb-host,hostbus=%d,hostaddr=%d,id=%s", - dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device, - dev->info->alias); + virBufferAddLit(&buf, "usb-host"); + if (!dev->missing) { + virBufferAsprintf(&buf, ",hostbus=%d,hostaddr=%d", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + } + virBufferAsprintf(&buf, ",id=%s", dev->info->alias); if (qemuBuildDeviceAddressStr(&buf, dev->info, caps) < 0) goto error; @@ -3577,6 +3581,12 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev) { char *ret = NULL; + if (dev->missing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't not support missing USB devices")); + return NULL; + } + if (!dev->source.subsys.u.usb.bus && !dev->source.subsys.u.usb.device) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 32c858b..90dfd28 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -696,7 +696,8 @@ out: static int qemuPrepareHostUSBDevices(struct qemud_driver *driver, - virDomainDefPtr def) + virDomainDefPtr def, + bool coldBoot) { int i, ret = -1; usbDeviceList *list; @@ -716,6 +717,7 @@ qemuPrepareHostUSBDevices(struct qemud_driver *driver, */ for (i = 0 ; i < nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; + bool required = true; usbDevice *usb; if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) @@ -723,10 +725,15 @@ qemuPrepareHostUSBDevices(struct qemud_driver *driver, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue; - if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) + if (hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL || + (hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE && + !coldBoot)) + required = false; + + if (qemuFindHostdevUSBDevice(hostdev, required, &usb) < 0) goto cleanup; - if (usbDeviceListAdd(list, usb) < 0) { + if (usb && usbDeviceListAdd(list, usb) < 0) { usbFreeDevice(usb); goto cleanup; } @@ -756,7 +763,8 @@ cleanup: } int qemuPrepareHostDevices(struct qemud_driver *driver, - virDomainDefPtr def) + virDomainDefPtr def, + bool coldBoot) { if (!def->nhostdevs) return 0; @@ -764,7 +772,7 @@ int qemuPrepareHostDevices(struct qemud_driver *driver, if (qemuPrepareHostPCIDevices(driver, def) < 0) return -1; - if (qemuPrepareHostUSBDevices(driver, def) < 0) + if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0) return -1; return 0; @@ -891,6 +899,8 @@ qemuDomainReAttachHostUsbDevices(struct qemud_driver *driver, continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue; + if (hostdev->missing) + continue; usb = usbGetDevice(hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device); diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 74dd2ce..0da25f9 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -43,7 +43,8 @@ int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, usbDeviceList *list); int qemuPrepareHostDevices(struct qemud_driver *driver, - virDomainDefPtr def); + virDomainDefPtr def, + bool coldBoot); void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver); void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, const char *name, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 31909b7..0a50c78 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3382,7 +3382,7 @@ int qemuProcessStart(virConnectPtr conn, /* Must be run before security labelling */ VIR_DEBUG("Preparing host devices"); - if (qemuPrepareHostDevices(driver, vm->def) < 0) + if (qemuPrepareHostDevices(driver, vm->def, !migrateFrom) < 0) goto cleanup; VIR_DEBUG("Preparing chr devices"); -- 1.7.12
On Tue, Oct 09, 2012 at 02:13:25PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_cgroup.c | 2 ++ src/qemu/qemu_command.c | 20 +++++++++++++++----- src/qemu/qemu_hostdev.c | 20 +++++++++++++++----- src/qemu/qemu_hostdev.h | 3 ++- src/qemu/qemu_process.c | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f412e..d590df6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3507,17 +3507,21 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, { virBuffer buf = VIR_BUFFER_INITIALIZER;
- if (!dev->source.subsys.u.usb.bus && + if (!dev->missing && + !dev->source.subsys.u.usb.bus && !dev->source.subsys.u.usb.device) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("USB host device is missing bus/device information")); return NULL; }
- virBufferAsprintf(&buf, "usb-host,hostbus=%d,hostaddr=%d,id=%s", - dev->source.subsys.u.usb.bus, - dev->source.subsys.u.usb.device, - dev->info->alias); + virBufferAddLit(&buf, "usb-host"); + if (!dev->missing) { + virBufferAsprintf(&buf, ",hostbus=%d,hostaddr=%d", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + } + virBufferAsprintf(&buf, ",id=%s", dev->info->alias);
I'm curious about this - it will result in -device usb-host,id=XXXXX shouldn't we actually just leave out the entire arg. I'm not sure what QEMU would do with such a device specification. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 10, 2012 at 16:39:06 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:25PM +0200, Jiri Denemark wrote:
+ virBufferAddLit(&buf, "usb-host"); + if (!dev->missing) { + virBufferAsprintf(&buf, ",hostbus=%d,hostaddr=%d", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + } + virBufferAsprintf(&buf, ",id=%s", dev->info->alias);
I'm curious about this - it will result in
-device usb-host,id=XXXXX
shouldn't we actually just leave out the entire arg. I'm not sure what QEMU would do with such a device specification.
According to Gerd (and my testing) it's the desired command line. QEMU needs to know about the usb-host device during incoming migration to avoid aborting on unknown section in migration data. It's not necessary when starting a domain from scratch but it doesn't seem to cause any harm. Jirka
On Wed, Oct 10, 2012 at 10:39:53PM +0200, Jiri Denemark wrote:
On Wed, Oct 10, 2012 at 16:39:06 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:25PM +0200, Jiri Denemark wrote:
+ virBufferAddLit(&buf, "usb-host"); + if (!dev->missing) { + virBufferAsprintf(&buf, ",hostbus=%d,hostaddr=%d", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + } + virBufferAsprintf(&buf, ",id=%s", dev->info->alias);
I'm curious about this - it will result in
-device usb-host,id=XXXXX
shouldn't we actually just leave out the entire arg. I'm not sure what QEMU would do with such a device specification.
According to Gerd (and my testing) it's the desired command line. QEMU needs to know about the usb-host device during incoming migration to avoid aborting on unknown section in migration data. It's not necessary when starting a domain from scratch but it doesn't seem to cause any harm.
Ok, that makes sense now. ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Using VIR_DOMAIN_XML_MIGRATABLE flag, one can request domain's XML configuration that is suitable for migration or save/restore. Such XML may contain extra run-time stuff internal to libvirt and some default configuration may be removed for better compatibility of the XML with older libvirt releases. This flag may serve as an easy way to get the XML that can be passed (after desired modifications) to APIs that accept custom XMLs, such as virDomainMigrate{,ToURI}2 or virDomainSaveFlags. --- include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 3 ++- src/qemu/qemu_domain.c | 21 ++++++++------------- src/qemu/qemu_domain.h | 11 ++++++----- src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_migration.c | 11 +++++------ src/qemu/qemu_process.c | 14 +++++++------- tools/virsh-domain.c | 4 ++++ tools/virsh.pod | 7 +++++-- 9 files changed, 47 insertions(+), 41 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 81f12a4..b1a3e25 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1651,6 +1651,7 @@ typedef enum { VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ } virDomainXMLFlags; char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da2d86d..c32ce8d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13294,7 +13294,8 @@ virDomainHubDefFormat(virBufferPtr buf, #define DUMPXML_FLAGS \ (VIR_DOMAIN_XML_SECURE | \ VIR_DOMAIN_XML_INACTIVE | \ - VIR_DOMAIN_XML_UPDATE_CPU) + VIR_DOMAIN_XML_UPDATE_CPU | \ + VIR_DOMAIN_XML_MIGRATABLE) verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17ae3b9..ff56c46 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -46,10 +46,6 @@ #define QEMU_NAMESPACE_HREF "http://libvirt.org/schemas/domain/qemu/1.0" -#define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \ - (VIR_DOMAIN_XML_SECURE | \ - VIR_DOMAIN_XML_UPDATE_CPU) - VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST, "none", "query", @@ -1220,7 +1216,6 @@ int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr def, unsigned int flags, - bool compatible, virBuffer *buf) { int ret = -1; @@ -1245,7 +1240,7 @@ qemuDomainDefFormatBuf(struct qemud_driver *driver, def->cpu = cpu; } - if (compatible) { + if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) { int i; virDomainControllerDefPtr usb = NULL; @@ -1297,12 +1292,11 @@ cleanup: char *qemuDomainDefFormatXML(struct qemud_driver *driver, virDomainDefPtr def, - unsigned int flags, - bool compatible) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuDomainDefFormatBuf(driver, def, flags, compatible, &buf) < 0) { + if (qemuDomainDefFormatBuf(driver, def, flags, &buf) < 0) { virBufferFreeAndReset(&buf); return NULL; } @@ -1318,8 +1312,7 @@ char *qemuDomainDefFormatXML(struct qemud_driver *driver, char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, - unsigned int flags, - bool compatible) + unsigned int flags) { virDomainDefPtr def; @@ -1328,7 +1321,7 @@ char *qemuDomainFormatXML(struct qemud_driver *driver, else def = vm->def; - return qemuDomainDefFormatXML(driver, def, flags, compatible); + return qemuDomainDefFormatXML(driver, def, flags); } char * @@ -1341,8 +1334,10 @@ qemuDomainDefFormatLive(struct qemud_driver *driver, if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; + if (compatible) + flags |= VIR_DOMAIN_XML_MIGRATABLE; - return qemuDomainDefFormatXML(driver, def, flags, compatible); + return qemuDomainDefFormatXML(driver, def, flags); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 97e4346..26b6c55 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -39,6 +39,10 @@ (1 << VIR_DOMAIN_VIRT_KVM) | \ (1 << VIR_DOMAIN_VIRT_XEN)) +# define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \ + (VIR_DOMAIN_XML_SECURE | \ + VIR_DOMAIN_XML_UPDATE_CPU) + # if ULONG_MAX == 4294967295 /* Qemu has a 64-bit limit, but we are limited by our historical choice of * representing bandwidth in a long instead of a 64-bit int. */ @@ -253,18 +257,15 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr vm, unsigned int flags, - bool compatible, virBuffer *buf); char *qemuDomainDefFormatXML(struct qemud_driver *driver, virDomainDefPtr vm, - unsigned int flags, - bool compatible); + unsigned int flags); char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, - unsigned int flags, - bool compatible); + unsigned int flags); char *qemuDomainDefFormatLive(struct qemud_driver *driver, virDomainDefPtr def, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 97ad23e..094e9ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4966,7 +4966,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, if (fd < 0) goto cleanup; - ret = qemuDomainDefFormatXML(driver, def, flags, false); + ret = qemuDomainDefFormatXML(driver, def, flags); cleanup: virDomainDefFree(def); @@ -5010,8 +5010,8 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE, - true); + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE); if (!xml) goto cleanup; len = strlen(xml) + 1; @@ -5161,7 +5161,10 @@ endjob: } } - ret = qemuDomainFormatXML(driver, vm, flags, false); + if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) + flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; + + ret = qemuDomainFormatXML(driver, vm, flags); cleanup: if (vm) @@ -5201,7 +5204,7 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, goto cleanup; } - xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE, false); + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE); cleanup: virDomainDefFree(def); @@ -11731,8 +11734,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!(xml = qemuDomainDefFormatXML(driver, snap->def->dom, VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE, - false))) + VIR_DOMAIN_XML_SECURE))) goto cleanup; config = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index db69a0a..7cc1f98 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -432,8 +432,8 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, if (qemuDomainDefFormatBuf(driver, mig->persistent, VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE, - true, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE, buf) < 0) return -1; virBufferAdjustIndent(buf, -2); @@ -1264,7 +1264,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, int hookret; if (!(xml = qemuDomainDefFormatXML(driver, def, - VIR_DOMAIN_XML_SECURE, false))) + VIR_DOMAIN_XML_SECURE))) goto cleanup; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, def->name, @@ -2213,9 +2213,8 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, * and pass it to Prepare2. */ if (!(dom_xml = qemuDomainFormatXML(driver, vm, - VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_UPDATE_CPU, - true))) + QEMU_DOMAIN_FORMAT_LIVE_FLAGS | + VIR_DOMAIN_XML_MIGRATABLE))) return -1; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0a50c78..26374e4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3137,7 +3137,7 @@ qemuProcessReconnect(void *opaque) /* Run an hook to allow admins to do some magic */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, obj->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, obj->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, @@ -3365,7 +3365,7 @@ int qemuProcessStart(virConnectPtr conn, /* Run an early hook to set-up missing devices */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3582,7 +3582,7 @@ int qemuProcessStart(virConnectPtr conn, /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3823,7 +3823,7 @@ int qemuProcessStart(virConnectPtr conn, /* finally we can call the 'started' hook script if any */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -4003,7 +4003,7 @@ void qemuProcessStop(struct qemud_driver *driver, /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -4098,7 +4098,7 @@ retry: /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -4300,7 +4300,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, /* Run an hook to allow admins to do some magic */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 505169b..1df0872 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6399,6 +6399,7 @@ static const vshCmdOptDef opts_dumpxml[] = { {"inactive", VSH_OT_BOOL, 0, N_("show inactive defined XML")}, {"security-info", VSH_OT_BOOL, 0, N_("include security sensitive information in XML dump")}, {"update-cpu", VSH_OT_BOOL, 0, N_("update guest CPU according to host CPU")}, + {"migratable", VSH_OT_BOOL, 0, N_("provide XML suitable for migrations")}, {NULL, 0, 0, NULL} }; @@ -6412,6 +6413,7 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) bool inactive = vshCommandOptBool(cmd, "inactive"); bool secure = vshCommandOptBool(cmd, "security-info"); bool update = vshCommandOptBool(cmd, "update-cpu"); + bool migratable = vshCommandOptBool(cmd, "migratable"); if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -6419,6 +6421,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_SECURE; if (update) flags |= VIR_DOMAIN_XML_UPDATE_CPU; + if (migratable) + flags |= VIR_DOMAIN_XML_MIGRATABLE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index ac8a00f..ea5060c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -948,7 +948,7 @@ NOTE: Some hypervisors may require the user to manually ensure proper permissions on file and path specified by argument I<corefilepath>. =item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] +[I<--update-cpu>] [I<--migratable>] Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -956,7 +956,10 @@ used. I<--inactive> tells virsh to dump domain configuration that will be used on next start of the domain as opposed to the current domain configuration. Using I<--security-info> will also include security sensitive information in the XML dump. I<--update-cpu> updates domain CPU requirements according to -host CPU. +host CPU. With I<--migratable> one can request an XML that is suitable for +migrations, i.e., compatible with older libvirt releases and possibly amended +with internal run-time options. This option may automatically enable other +options (I<--update-cpu>, I<--security-info>, ...) as necessary. =item B<edit> I<domain> -- 1.7.12
On 10/09/12 14:13, Jiri Denemark wrote:
Using VIR_DOMAIN_XML_MIGRATABLE flag, one can request domain's XML configuration that is suitable for migration or save/restore. Such XML may contain extra run-time stuff internal to libvirt and some default configuration may be removed for better compatibility of the XML with older libvirt releases.
This flag may serve as an easy way to get the XML that can be passed (after desired modifications) to APIs that accept custom XMLs, such as virDomainMigrate{,ToURI}2 or virDomainSaveFlags. --- include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 3 ++- src/qemu/qemu_domain.c | 21 ++++++++------------- src/qemu/qemu_domain.h | 11 ++++++----- src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_migration.c | 11 +++++------ src/qemu/qemu_process.c | 14 +++++++------- tools/virsh-domain.c | 4 ++++ tools/virsh.pod | 7 +++++-- 9 files changed, 47 insertions(+), 41 deletions(-)
ACK. Peter
Save/restore with passed through USB devices currently only works if the USB device can be found at the same USB address where it used to be before saving a domain. This makes sense in case a user explicitly configure the USB address in domain XML. However, if the device was found automatically by vendor/product identification, we should try to search for that device when restoring the domain and use any device we find as long as there is only one available. In other words, the USB device can now be removed and plugged again or the host can be rebooted between saving and restoring the domain. --- src/conf/domain_conf.c | 10 ++++++++++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_hostdev.c | 47 ++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_migration.c | 3 ++- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c32ce8d..0cb686d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2695,6 +2695,7 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, int got_product, got_vendor; xmlNodePtr cur; char *startupPolicy = NULL; + char *autoAddress; if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) { def->startupPolicy = @@ -2709,6 +2710,12 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, VIR_FREE(startupPolicy); } + if ((autoAddress = virXMLPropString(node, "autoAddress"))) { + if (STREQ(autoAddress, "yes")) + def->source.subsys.u.usb.autoAddress = true; + VIR_FREE(autoAddress); + } + /* Product can validly be 0, so we need some extra help to determine * if it is uninitialized*/ got_product = 0; @@ -12072,6 +12079,9 @@ virDomainHostdevSourceFormat(virBufferPtr buf, policy = virDomainStartupPolicyTypeToString(def->startupPolicy); virBufferAsprintf(buf, " startupPolicy='%s'", policy); } + if (def->source.subsys.u.usb.autoAddress && + (flags & VIR_DOMAIN_XML_MIGRATABLE)) + virBufferAddLit(buf, " autoAddress='yes'"); virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3cb1193..27f9538 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -370,6 +370,8 @@ struct _virDomainHostdevSubsys { int type; /* enum virDomainHostdevSubsysType */ union { struct { + bool autoAddress; /* bus/device were filled automatically based + on vedor/product */ unsigned bus; unsigned device; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 094e9ab..6022f97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11183,7 +11183,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, false)) || + if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true)) || !(def->dom = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE))) @@ -11734,7 +11734,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!(xml = qemuDomainDefFormatXML(driver, snap->def->dom, VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE))) + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; config = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 90dfd28..e24c022 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -650,15 +650,31 @@ qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev, unsigned product = hostdev->source.subsys.u.usb.product; unsigned bus = hostdev->source.subsys.u.usb.bus; unsigned device = hostdev->source.subsys.u.usb.device; + bool autoAddress = hostdev->source.subsys.u.usb.autoAddress; int rc; *usb = NULL; if (vendor && bus) { - rc = usbFindDevice(vendor, product, bus, device, mandatory, usb); - if (rc < 0) + rc = usbFindDevice(vendor, product, bus, device, + autoAddress ? false : mandatory, + usb); + if (rc < 0) { return -1; - } else if (vendor && !bus) { + } 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); + } + } + + /* 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) { usbDeviceList *devs; rc = usbFindDeviceByVendor(vendor, product, mandatory, &devs); @@ -674,15 +690,32 @@ qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev, if (rc == 0) { goto out; } else if (rc > 1) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple USB devices for %x:%x, " - "use <address> to specify one"), - vendor, product); + if (autoAddress) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Multiple USB devices for %x:%x were found," + " but none of them is at bus:%u device:%u"), + vendor, product, bus, device); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Multiple USB devices for %x:%x, " + "use <address> to specify one"), + vendor, product); + } return -1; } hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(*usb); hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(*usb); + hostdev->source.subsys.u.usb.autoAddress = true; + + if (autoAddress) { + VIR_INFO("USB device %x:%x found at bus:%u device:%u (moved" + " from bus:%u device:%u)", + vendor, product, + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + bus, device); + } } else if (!vendor && bus) { if (usbFindDeviceByBus(bus, device, mandatory, usb) < 0) return -1; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7cc1f98..68d614d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1264,7 +1264,8 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, int hookret; if (!(xml = qemuDomainDefFormatXML(driver, def, - VIR_DOMAIN_XML_SECURE))) + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, def->name, -- 1.7.12
On Tue, Oct 09, 2012 at 02:13:27PM +0200, Jiri Denemark wrote:
Save/restore with passed through USB devices currently only works if the USB device can be found at the same USB address where it used to be before saving a domain. This makes sense in case a user explicitly configure the USB address in domain XML. However, if the device was found automatically by vendor/product identification, we should try to search for that device when restoring the domain and use any device we find as long as there is only one available. In other words, the USB device can now be removed and plugged again or the host can be rebooted between saving and restoring the domain. --- src/conf/domain_conf.c | 10 ++++++++++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_hostdev.c | 47 ++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_migration.c | 3 ++- 5 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c32ce8d..0cb686d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2695,6 +2695,7 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, int got_product, got_vendor; xmlNodePtr cur; char *startupPolicy = NULL; + char *autoAddress;
if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) { def->startupPolicy = @@ -2709,6 +2710,12 @@ virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, VIR_FREE(startupPolicy); }
+ if ((autoAddress = virXMLPropString(node, "autoAddress"))) { + if (STREQ(autoAddress, "yes")) + def->source.subsys.u.usb.autoAddress = true; + VIR_FREE(autoAddress); + } + /* Product can validly be 0, so we need some extra help to determine * if it is uninitialized*/ got_product = 0; @@ -12072,6 +12079,9 @@ virDomainHostdevSourceFormat(virBufferPtr buf, policy = virDomainStartupPolicyTypeToString(def->startupPolicy); virBufferAsprintf(buf, " startupPolicy='%s'", policy); } + if (def->source.subsys.u.usb.autoAddress && + (flags & VIR_DOMAIN_XML_MIGRATABLE)) + virBufferAddLit(buf, " autoAddress='yes'"); virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
Do we really need to add a new attribute for this. IMHO if the user has specified a vendor+product, then it is always auto-address, even if they have also given a dev+address. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 10, 2012 at 16:40:59 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:27PM +0200, Jiri Denemark wrote:
@@ -12072,6 +12079,9 @@ virDomainHostdevSourceFormat(virBufferPtr buf, policy = virDomainStartupPolicyTypeToString(def->startupPolicy); virBufferAsprintf(buf, " startupPolicy='%s'", policy); } + if (def->source.subsys.u.usb.autoAddress && + (flags & VIR_DOMAIN_XML_MIGRATABLE)) + virBufferAddLit(buf, " autoAddress='yes'"); virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
Do we really need to add a new attribute for this. IMHO if the user has specified a vendor+product, then it is always auto-address, even if they have also given a dev+address.
We do need it. If both vendor/product and bus/dev are specified, the code doesn't try to find the device at different address. See the following code snippet from usbDeviceSearch: if ((flags & USB_DEVICE_FIND_BY_VENDOR) && (found_prod != product || found_vend != vendor)) continue; if (flags & USB_DEVICE_FIND_BY_BUS) { if (found_bus != bus || found_devno != devno) continue; found = true; } And it also make sense because if there are more devices with the same vendor/product id, we require bus/dev to be configured so that we can assign the right device and don't mess with other similar devices. However, if the address was autofilled by libvirt, we can try to look at other addresses if the device cannot be found at its original address (i.e., it was unplugged and plugged again). Jirka
On Thu, Oct 11, 2012 at 11:42:50AM +0200, Jiri Denemark wrote:
On Wed, Oct 10, 2012 at 16:40:59 +0100, Daniel P. Berrange wrote:
On Tue, Oct 09, 2012 at 02:13:27PM +0200, Jiri Denemark wrote:
@@ -12072,6 +12079,9 @@ virDomainHostdevSourceFormat(virBufferPtr buf, policy = virDomainStartupPolicyTypeToString(def->startupPolicy); virBufferAsprintf(buf, " startupPolicy='%s'", policy); } + if (def->source.subsys.u.usb.autoAddress && + (flags & VIR_DOMAIN_XML_MIGRATABLE)) + virBufferAddLit(buf, " autoAddress='yes'"); virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
Do we really need to add a new attribute for this. IMHO if the user has specified a vendor+product, then it is always auto-address, even if they have also given a dev+address.
We do need it. If both vendor/product and bus/dev are specified, the code doesn't try to find the device at different address. See the following code snippet from usbDeviceSearch:
if ((flags & USB_DEVICE_FIND_BY_VENDOR) && (found_prod != product || found_vend != vendor)) continue;
if (flags & USB_DEVICE_FIND_BY_BUS) { if (found_bus != bus || found_devno != devno) continue; found = true; }
And it also make sense because if there are more devices with the same vendor/product id, we require bus/dev to be configured so that we can assign the right device and don't mess with other similar devices. However, if the address was autofilled by libvirt, we can try to look at other addresses if the device cannot be found at its original address (i.e., it was unplugged and plugged again).
Ok, ACK to the patch Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Oct 09, 2012 at 14:13:21 +0200, Jiri Denemark wrote:
USB devices can disappear without OS being mad about it, which makes them ideal for startupPolicy so that a domain with USB devices can be migrated to a host that lacks the USB devices. Moreover, this series allows USB devices to be re-plugged or a host to be rebooted while domains with USB devices are (managed)saved.
Jiri Denemark (6): conf: Add support for startupPolicy for USB devices qemu: Introduce qemuFindHostdevUSBDevice qemu: Add option to treat missing USB devices as success qemu: Implement startupPolicy for USB passed through devices Add MIGRATABLE flag for virDomainGetXMLDesc qemu: Make save/restore with USB devices usable
I pushed this series. Thanks for the reviews. Jirka
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Peter Krempa