[libvirt] [PATCH 0/9] Support disk-hotremove and controller hotplugging
Hi, this patch reworks libvirt's disk-hotadd support and introduces support for disk controller hotplugging and disk-hotremove (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860 for more details). Currently, it targets only qemu and also requires some additions to qemu that have only recently been submitted, which is why this is cross-posted to qemu-devel. Hopefully the lack of support in qemu does not prevent the community from reviewing the code, though ;-) Notice that no documentation and testcases are included yet; I'll supply those once the interface is fixed and possible issues are settled. Thanks for your comments, Wolfgang docs/schemas/domain.rng | 145 ++++++++++---- src/domain_conf.c | 208 ++++++++++++++++++++- src/domain_conf.h | 39 ++++- src/libvirt_private.syms | 4 +- src/qemu_driver.c | 479 +++++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 802 insertions(+), 73 deletions(-) -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux
The instruction "See Makefile.am" in libvirt.private_syms always makes me think that this file is autogenerated and should not be touched manually. This patch spares every reader of libvirt.private_syms the hassle of reading Makefile.am before augmenting libvirt.private_syms. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/libvirt_private.syms | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 867678f..f724493 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1,5 +1,6 @@ # -# General private symbols. See Makefile.am. +# General private symbols. Add symbols here, and see Makefile.am for +# more details. # -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:08PM +0200, Wolfgang Mauerer wrote:
The instruction "See Makefile.am" in libvirt.private_syms always makes me think that this file is autogenerated and should not be touched manually. This patch spares every reader of libvirt.private_syms the hassle of reading Makefile.am before augmenting libvirt.private_syms.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/libvirt_private.syms | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 867678f..f724493 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1,5 +1,6 @@ # -# General private symbols. See Makefile.am. +# General private symbols. Add symbols here, and see Makefile.am for +# more details. #
ACK, this is fine Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This allows us to connect a disk with a specific controller, which is required for disk hotadd/remove. A new XML child element is added to the <disk> container: <disk> ... <controller pci_addr="<addr>" id="<identifier>" bus="<number>" unit="<number>"/> </disk> Either id _or_ pci_addr can be specified. When the controller has been brought into the system via tghe hotplug mechanism also included in this patch series, it will typically have an ID. In other cases, the controller can also be specified with the PCI address. Wrt. the data structures touched in this commit, notice that while "bus" as subelement of target specifies the type of bus (e.g., scsi, ide,...), it is used to enumerate the buses on the controller here. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/domain_conf.h | 5 ++++- 2 files changed, 33 insertions(+), 1 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 5ae0775..a6120c8 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -650,7 +650,12 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *driverType = NULL; char *source = NULL; char *target = NULL; + char *controller = NULL; + char *bus_id = NULL; + char *unit_id = NULL; + char *controller_id = NULL; char *bus = NULL; + char *unit = NULL; char *cachetag = NULL; char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; @@ -694,12 +699,18 @@ virDomainDiskDefParseXML(virConnectPtr conn, (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = virXMLPropString(cur, "dev"); bus = virXMLPropString(cur, "bus"); + unit = virXMLPropString(cur, "unit"); /* HACK: Work around for compat with Xen * driver in previous libvirt releases */ if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); + } else if ((controller == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "controller"))) { + controller_id = virXMLPropString(cur, "id"); + bus_id = virXMLPropString(cur, "bus"); + unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); @@ -800,6 +811,24 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } + if (controller) { + def->controller_id = controller_id; + + def->bus_id = -1; + if (bus_id && virStrToLong_i(bus_id, NULL, 10, &def->bus_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'bus' attribute")); + goto error; + } + + def->unit_id = -1; + if (unit_id && virStrToLong_i(unit_id, NULL, 10, &def->unit_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'unit' attribute")); + goto error; + } + } + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus != VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, diff --git a/src/domain_conf.h b/src/domain_conf.h index 09368d9..898f6c9 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -105,9 +105,12 @@ typedef virDomainDiskDef *virDomainDiskDefPtr; struct _virDomainDiskDef { int type; int device; - int bus; + int bus; /* Bus type, e.g. scsi or ide */ + int bus_id; /* Bus number on the controller */ + int unit_id; /* Unit on the controller */ char *src; char *dst; + char *controller_id; char *driverName; char *driverType; char *serial; -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:09PM +0200, Wolfgang Mauerer wrote:
This allows us to connect a disk with a specific controller, which is required for disk hotadd/remove. A new XML child element is added to the <disk> container:
<disk> ... <controller pci_addr="<addr>" id="<identifier>" bus="<number>" unit="<number>"/> </disk>
Either id _or_ pci_addr can be specified. When the controller has been brought into the system via tghe hotplug mechanism also included in this patch series, it will typically have an ID. In other cases, the controller can also be specified with the PCI address.
I think it would be desriable to make 'id' the mandatory argument and not use 'pci_addr' in this area of the XML. <controller id="<identifier>" bus="<number>" unit="<number>"/> If fact I think it'd be good to rename 'id' to 'name' since in libvirt context we typically use 'id' to be be positive integer identifier. 'name' reflects the fact that this device idenifier is really a string. Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML
memmove(target, target+6, strlen(target)-5); + } else if ((controller == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "controller"))) { + controller_id = virXMLPropString(cur, "id"); + bus_id = virXMLPropString(cur, "bus"); + unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); @@ -800,6 +811,24 @@ virDomainDiskDefParseXML(virConnectPtr conn, } }
+ if (controller) { + def->controller_id = controller_id; + + def->bus_id = -1; + if (bus_id && virStrToLong_i(bus_id, NULL, 10, &def->bus_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'bus' attribute")); + goto error; + } + + def->unit_id = -1; + if (unit_id && virStrToLong_i(unit_id, NULL, 10, &def->unit_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'unit' attribute")); + goto error; + } + } +
There's a few whitespace issues here - I guess some tabs crept into the patch Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2009/9/24 Daniel P. Berrange <berrange@redhat.com>:
Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML
Would this device name be read-only, or would it be editable/writable by the user (e.g. through virsh edit)? If it should be read-only, I see no problem supporting device names in the ESX driver. But If it should be editable/writable it may be a problem to implement this for the ESX driver, as I'm currently not aware of any sensible way to store such an custom device name in a persistent manner on an ESX server. Matthias
Matthias Bolte wrote:
2009/9/24 Daniel P. Berrange <berrange@redhat.com>:
Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML
Would this device name be read-only, or would it be editable/writable by the user (e.g. through virsh edit)? If it should be read-only, I see no problem supporting device names in the ESX driver. But If it should be editable/writable it may be a problem to implement this for the ESX driver, as I'm currently not aware of any sensible way to store such an custom device name in a persistent manner on an ESX server.
I suppose having the name read-only suffices. At least I don't see any particularly important use cases for a writeable name, especially considering that the name can be chosen arbitrarily at creation time. And there's still the possibility to shut down the virtual machine, change names, and restart again if new names are absolutely required. Thanks, Wolfgang
On Thu, Sep 24, 2009 at 11:39:08PM +0200, Matthias Bolte wrote:
2009/9/24 Daniel P. Berrange <berrange@redhat.com>:
Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML
Would this device name be read-only, or would it be editable/writable by the user (e.g. through virsh edit)? If it should be read-only, I see no problem supporting device names in the ESX driver. But If it should be editable/writable it may be a problem to implement this for the ESX driver, as I'm currently not aware of any sensible way to store such an custom device name in a persistent manner on an ESX server.
Latest QEMU allows the name to be choosen by the application arbitrarily, but older QEMU auto-assigned names itself. I don't see any compelling reason to make the names editable by applications, so it is probably sufficient to say they'll be read-only / output-only attributes in the XML assigned by the hypervisor driver as required. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:09PM +0200, Wolfgang Mauerer wrote:
This allows us to connect a disk with a specific controller, which is required for disk hotadd/remove. A new XML child element is added to the <disk> container:
<disk> ... <controller pci_addr="<addr>" id="<identifier>" bus="<number>" unit="<number>"/> </disk>
Either id _or_ pci_addr can be specified. When the controller has been brought into the system via tghe hotplug mechanism also included in this patch series, it will typically have an ID. In other cases, the controller can also be specified with the PCI address.
I think it would be desriable to make 'id' the mandatory argument and not use 'pci_addr' in this area of the XML.
<controller id="<identifier>" bus="<number>" unit="<number>"/>
If fact I think it'd be good to rename 'id' to 'name' since in libvirt context we typically use 'id' to be be positive integer identifier. 'name' reflects the fact that this device idenifier is really a string.
I'm find with s/id/name/g. I completely agree with you that using a name instead of a PCI address would suit the generic spirit of libvirt much better. However, do all hypervisors allow us to attach individual controllers, or are there cases where controller are implicitely added to the system? In this case, assigning names to controllers might be impractical or maybe impossible without further ado. So for the time being, I would suggest to keep the pci_addr parameter, but clearly state that name is much preferred.
Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML
There's a few whitespace issues here - I guess some tabs crept into the patch
Whops - I'll eliminate them in the next series. Thanks, Wolfgang
This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by <controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller> where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus. The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests Routines for parsing this definition and the associated data structures are included in this commit. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- docs/schemas/domain.rng | 145 ++++++++++++++++++++++++++++++++++------------- src/domain_conf.c | 125 ++++++++++++++++++++++++++++++++++++++++ src/domain_conf.h | 24 ++++++++ 3 files changed, 255 insertions(+), 39 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 70e98a7..dc9b849 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -364,49 +364,116 @@ <define name="disk"> <element name="disk"> <optional> - <attribute name="device"> + <attribute name="device"> + <choice> + <value>floppy</value> + <value>disk</value> + <value>cdrom</value> + </choice> + </attribute> + </optional> + <optional> + <element name="controller"> + <optional> + <attribute name="bus"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="unit"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <choice> + <attribute name="id"> + <ref name="genericName"/> + </attribute> + <attribute name="pciaddr"> + <!-- Just for testing --> + <ref name="deviceName"/> + </attribute> + </choice> + </element> + </optional> + <choice> + <group> + <attribute name="type"> + <value>file</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + </optional> + <ref name="diskspec"/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dev"> + <ref name="deviceName"/> + </attribute> + <empty/> + </element> + </optional> + <ref name="diskspec"/> + </interleave> + </group> + <ref name="diskspec"/> + </choice> + </element> + </define> + <define name="target"> + <element name="target"> + <attribute name="dev"> + <ref name="deviceName"/> + </attribute> + <optional> + <attribute name="bus"> <choice> - <value>floppy</value> - <value>disk</value> - <value>cdrom</value> + <value>ide</value> + <value>fdc</value> + <value>scsi</value> + <value>virtio</value> + <value>xen</value> + <value>usb</value> + <value>uml</value> </choice> </attribute> </optional> - <choice> - <group> - <attribute name="type"> - <value>file</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - <empty/> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dev"> - <ref name="deviceName"/> - </attribute> - <empty/> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <ref name="diskspec"/> - </choice> + </element> + </define> + + <define name="controller"> + <element name="controller"> + <interleave> + <attribute name="type"> + <choice> + <!-- For now, only SCSI is supported --> + <value>scsi</value> + </choice> + </attribute> + <attribute name="id"> + <ref name="genericName"/> + </attribute> + </interleave> + <optional> + <element name="bus"> + <attribute name="addr"> + <!-- Just for testing, should be pciaddress --> + <ref name="deviceName"/> + </attribute> + </element> + </optional> </element> </define> <define name="target"> diff --git a/src/domain_conf.c b/src/domain_conf.c index a6120c8..bbaf944 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -79,6 +79,7 @@ VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST, VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "disk", + "controller", "filesystem", "interface", "input", @@ -294,6 +295,18 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def); } +void virDomainControllerDefFree(virDomainControllerDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->type); + VIR_FREE(def->id); + VIR_FREE(def->pci_addr); + + VIR_FREE(def); +} + void virDomainFSDefFree(virDomainFSDefPtr def) { if (!def) @@ -651,6 +664,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *source = NULL; char *target = NULL; char *controller = NULL; + char *controller_pci_addr = NULL; char *bus_id = NULL; char *unit_id = NULL; char *controller_id = NULL; @@ -709,6 +723,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, } else if ((controller == NULL) && (xmlStrEqual(cur->name, BAD_CAST "controller"))) { controller_id = virXMLPropString(cur, "id"); + controller_pci_addr = virXMLPropString(cur, "pci_addr"); bus_id = virXMLPropString(cur, "bus"); unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && @@ -827,6 +842,19 @@ virDomainDiskDefParseXML(virConnectPtr conn, _("Cannot parse <controller> 'unit' attribute")); goto error; } + + /* TODO: Should we re-use devaddr for this purpose, + or is an extra field justified? */ + if (controller_pci_addr && + sscanf(controller_pci_addr, "%x:%x:%x", + &def->controller_pci_addr.domain, + &def->controller_pci_addr.bus, + &def->controller_pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse <controller> 'pci_addr' parameter '%s'"), + controller_pci_addr); + goto error; + } } if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -895,6 +923,77 @@ cleanup: } +/* Parse the XML definition for a controller + * @param node XML nodeset to parse for controller definition + */ +static virDomainControllerDefPtr +virDomainControllerDefParseXML(virConnectPtr conn, + xmlNodePtr node, + int flags ATTRIBUTE_UNUSED) { + virDomainControllerDefPtr def; + xmlNodePtr cur; + char *type = NULL; + char *bus_addr = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(conn); + return NULL; + } + + type = virXMLPropString(node, "type"); + def->type = VIR_DOMAIN_DISK_BUS_SCSI; + if (type) { + if ((def->type = virDomainDiskBusTypeFromString(type)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown disk bus type '%s'"), type); + goto error; + } + } + + def->id = virXMLPropString(node, "id"); + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE && + bus_addr == NULL && + xmlStrEqual(cur->name, BAD_CAST "bus")) { + + bus_addr = virXMLPropString(cur, "addr"); + + def->pci_addr.domain = -1; + def->pci_addr.bus = -1; + def->pci_addr.slot = -1; + if (bus_addr && + sscanf(bus_addr, "%x:%x:%x", + &def->pci_addr.domain, + &def->pci_addr.bus, + &def->pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "Unable to parse <bus> 'addr' parameter '%s'", + bus_addr); + goto error; + } + + VIR_DEBUG("Parse PCI address of controller as %d:%d:%d\n", + def->pci_addr.domain, def->pci_addr.bus, + def->pci_addr.slot); + } + cur = cur->next; + } + + +cleanup: + VIR_FREE(type); + VIR_FREE(bus_addr); + + return def; + + error: + virDomainControllerDefFree(def); + def = NULL; + goto cleanup; +} + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2375,6 +2474,11 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, dev->type = VIR_DOMAIN_DEVICE_DISK; if (!(dev->data.disk = virDomainDiskDefParseXML(conn, node, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "controller")) { + dev->type = VIR_DOMAIN_DEVICE_CONTROLLER; + if (!(dev->data.controller = + virDomainControllerDefParseXML(conn, node, flags))) + goto error; } else if (xmlStrEqual(node->name, BAD_CAST "filesystem")) { dev->type = VIR_DOMAIN_DEVICE_FS; if (!(dev->data.fs = virDomainFSDefParseXML(conn, node, flags))) @@ -2783,6 +2887,27 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, } VIR_FREE(nodes); + /* analysis of the disk controllers */ + if ((n = virXPathNodeSet(conn, "./devices/controller", ctxt, &nodes)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract controller devices")); + goto error; + } + if (n && VIR_ALLOC_N(def->controllers, n) < 0) + goto no_memory; + for (i = 0 ; i < n ; i++) { + virDomainControllerDefPtr controller = + virDomainControllerDefParseXML(conn, nodes[i], flags); + if (!controller) + goto error; + + def->controllers[def->ncontrollers++] = controller; + } + /* qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers), + virDomainControllerQSort); */ + VIR_FREE(nodes); + + /* analysis of the filesystems */ if ((n = virXPathNodeSet(conn, "./devices/filesystem", ctxt, &nodes)) < 0) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, diff --git a/src/domain_conf.h b/src/domain_conf.h index 898f6c9..6b3cb09 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,6 +111,11 @@ struct _virDomainDiskDef { char *src; char *dst; char *controller_id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } controller_pci_addr; char *driverName; char *driverType; char *serial; @@ -125,6 +130,19 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; }; +/* Stores the virtual disk controller configuration */ +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; +struct _virDomainControllerDef { + int type; + char *id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; +}; + static inline int virDiskHasValidPciAddr(virDomainDiskDefPtr def) { @@ -441,6 +459,7 @@ enum virDomainDeviceType { VIR_DOMAIN_DEVICE_SOUND, VIR_DOMAIN_DEVICE_VIDEO, VIR_DOMAIN_DEVICE_HOSTDEV, + VIR_DOMAIN_DEVICE_CONTROLLER, VIR_DOMAIN_DEVICE_LAST, }; @@ -451,6 +470,7 @@ struct _virDomainDeviceDef { int type; union { virDomainDiskDefPtr disk; + virDomainControllerDefPtr controller; virDomainFSDefPtr fs; virDomainNetDefPtr net; virDomainInputDefPtr input; @@ -561,6 +581,9 @@ struct _virDomainDef { int ndisks; virDomainDiskDefPtr *disks; + int ncontrollers; + virDomainControllerDefPtr *controllers; + int nfss; virDomainFSDefPtr *fss; @@ -637,6 +660,7 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); void virDomainDiskDefFree(virDomainDiskDefPtr def); +void virDomainControllerDefFree(virDomainControllerDefPtr def); void virDomainFSDefFree(virDomainFSDefPtr def); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by
<controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller>
where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus.
The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better called 'name'. For PCI addresses, it is desirable to fully normalize the XML format, by which I mean have separate attributes for domain, bus and slot. We already have a syntax for PCI addresses used for host device passthrough, so it'd make sense to use the same syntax here for controllers. More broadly, we're probably going to have to add a PCI address element to all our devices. While it is unlikely we'll need non-PCI addresses, it doesn't hurt to make it explicit by adding a type='pci' attribute Thus I'd suggest <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/> Instead of <bus addr="<Domain>:<Bus>:<Slot>"> In the domain_conf.c/.h parser, we could have a datatype like enum { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress; struct _virDomainDevicePCIAddress { unsigned domain; unsigned bus; unsigned slot; unsigned function; }; typedef struct _virDomainDeviceAddress virDomainDeviceAddress; struct _virDomainDeviceAddress { int type; union { virDomainDevicePCIAddress pci; } data; }; Which we then use in all the places in domain_conf.h wanting address information. Obviously we'd not use the 'function' field in most places, but doesn't hurt to have it. And a pair of methods for parsing/formatting this address info we can call from all the appropriate locations.
diff --git a/src/domain_conf.h b/src/domain_conf.h index 898f6c9..6b3cb09 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,6 +111,11 @@ struct _virDomainDiskDef { char *src; char *dst; char *controller_id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } controller_pci_addr;
I think we should stick to just using the controller name as the mandatory identifier for cross-referencing disks to controllers.
char *driverName; char *driverType; char *serial; @@ -125,6 +130,19 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; };
+/* Stores the virtual disk controller configuration */ +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; +struct _virDomainControllerDef { + int type; + char *id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; +};
With the generic address data type and s/id/name/, this would be just struct _virDomainControllerDef { int type; char *name; virDomainDeviceAddress addr; }; Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by
<controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller>
where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus.
The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better called 'name'.
For PCI addresses, it is desirable to fully normalize the XML format, by which I mean have separate attributes for domain, bus and slot. We already have a syntax for PCI addresses used for host device passthrough, so it'd make sense to use the same syntax here for controllers. More broadly, we're probably going to have to add a PCI address element to all our devices. While it is unlikely we'll need non-PCI addresses, it doesn't hurt to make it explicit by adding a type='pci' attribute
Thus I'd suggest
<address type='pci' domain='0x0000' bus='0x06' slot='0x12'/>
Instead of
<bus addr="<Domain>:<Bus>:<Slot>">
In the domain_conf.c/.h parser, we could have a datatype like
enum { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST };
typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress; struct _virDomainDevicePCIAddress { unsigned domain; unsigned bus; unsigned slot; unsigned function; }; typedef struct _virDomainDeviceAddress virDomainDeviceAddress; struct _virDomainDeviceAddress { int type; union { virDomainDevicePCIAddress pci; } data; };
Which we then use in all the places in domain_conf.h wanting address information. Obviously we'd not use the 'function' field in most places, but doesn't hurt to have it.
And a pair of methods for parsing/formatting this address info we can call from all the appropriate locations.
using a generic format for PCi addresses surely makes sense, especially wrt. the common data structure and parsing routines - I'll add these into the next series. However, I'm not sure if using <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/> instead of a simple string provides much of an advantage and does not just increase the typing strain (though there are maybe some small advantages for computerised parsing of libvirt configuration files by other apps). There's already a generic definition of PCI addresses in docs/schemas/domain.rng:\approx 1140, I suppose it's your intention to extend if by a type field? Thanks, Wolfgang
On Fri, Sep 25, 2009 at 03:03:27PM +0200, Wolfgang Mauerer wrote:
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by
<controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller>
where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus.
The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better called 'name'.
For PCI addresses, it is desirable to fully normalize the XML format, by which I mean have separate attributes for domain, bus and slot. We already have a syntax for PCI addresses used for host device passthrough, so it'd make sense to use the same syntax here for controllers. More broadly, we're probably going to have to add a PCI address element to all our devices. While it is unlikely we'll need non-PCI addresses, it doesn't hurt to make it explicit by adding a type='pci' attribute
Thus I'd suggest
<address type='pci' domain='0x0000' bus='0x06' slot='0x12'/>
Instead of
<bus addr="<Domain>:<Bus>:<Slot>">
In the domain_conf.c/.h parser, we could have a datatype like
enum { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST };
typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress; struct _virDomainDevicePCIAddress { unsigned domain; unsigned bus; unsigned slot; unsigned function; }; typedef struct _virDomainDeviceAddress virDomainDeviceAddress; struct _virDomainDeviceAddress { int type; union { virDomainDevicePCIAddress pci; } data; };
Which we then use in all the places in domain_conf.h wanting address information. Obviously we'd not use the 'function' field in most places, but doesn't hurt to have it.
And a pair of methods for parsing/formatting this address info we can call from all the appropriate locations.
using a generic format for PCi addresses surely makes sense, especially wrt. the common data structure and parsing routines - I'll add these into the next series. However, I'm not sure if using <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/> instead of a simple string provides much of an advantage and does not just increase the typing strain (though there are maybe some small advantages for computerised parsing of libvirt configuration files by other apps).
I think its valuable to have the consistent representation across the different areas of the XML, and although its obviously more verbose, it is beneficial for applications to only need an XML parser and not have to then write further parsers for attribute values.
There's already a generic definition of PCI addresses in docs/schemas/domain.rng:\approx 1140, I suppose it's your intention to extend if by a type field?
Yes, prettty much Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
When disks are added to a qemu backend with attach-device, not just the disk, but a complete new PCI controller with the disk attached is brought into the system. This patch implements a proper disk hotplugging scheme for qemu. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.c | 46 ++++++++-- src/domain_conf.h | 2 +- src/qemu_driver.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 277 insertions(+), 28 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index bbaf944..d0fda64 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -663,7 +663,6 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *driverType = NULL; char *source = NULL; char *target = NULL; - char *controller = NULL; char *controller_pci_addr = NULL; char *bus_id = NULL; char *unit_id = NULL; @@ -720,12 +719,13 @@ virDomainDiskDefParseXML(virConnectPtr conn, if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); - } else if ((controller == NULL) && + } else if ((controller_id == NULL && + controller_pci_addr == NULL) && (xmlStrEqual(cur->name, BAD_CAST "controller"))) { - controller_id = virXMLPropString(cur, "id"); - controller_pci_addr = virXMLPropString(cur, "pci_addr"); - bus_id = virXMLPropString(cur, "bus"); - unit_id = virXMLPropString(cur, "unit"); + controller_id = virXMLPropString(cur, "id"); + controller_pci_addr = virXMLPropString(cur, "pciaddr"); + bus_id = virXMLPropString(cur, "bus"); + unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); @@ -826,7 +826,13 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } - if (controller) { + if (controller_id && controller_pci_addr) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Both controller ID and address specified!")); + goto error; + } + + if (controller_id) { def->controller_id = controller_id; def->bus_id = -1; @@ -857,12 +863,35 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } + /* TODO: Should we re-use devaddr for this purpose, + or is an extra field justified? */ + def->controller_pci_addr.domain = -1; + def->controller_pci_addr.bus = -1; + def->controller_pci_addr.slot = -1; + + if (controller_pci_addr && + sscanf(controller_pci_addr, "%x:%x:%x", + &def->controller_pci_addr.domain, + &def->controller_pci_addr.bus, + &def->controller_pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse <controller> 'pciaddr' parameter '%s'"), + controller_pci_addr); + goto error; + } + + VIR_DEBUG("Controller PCI addr for disk parsed as %d:%d:%d\n", + def->controller_pci_addr.domain, + def->controller_pci_addr.bus, + def->controller_pci_addr.slot); + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus != VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, _("Invalid bus type '%s' for floppy disk"), bus); goto error; } + if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus == VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -888,6 +917,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, goto error; } + VIR_DEBUG("Disk PCI address parsed as %d:%d:%d\n", + def->pci_addr.domain, def->pci_addr.bus, def->pci_addr.slot); + def->src = source; source = NULL; def->dst = target; diff --git a/src/domain_conf.h b/src/domain_conf.h index 6b3cb09..41df8f6 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -107,7 +107,7 @@ struct _virDomainDiskDef { int device; int bus; /* Bus type, e.g. scsi or ide */ int bus_id; /* Bus number on the controller */ - int unit_id; /* Unit on the controller */ + int unit_id; /* Unit on the controller (both -1 if unspecified) */ char *src; char *dst; char *controller_id; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a65334f..4a30615 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5264,7 +5264,7 @@ qemudParsePciAddReply(virDomainObjPtr vm, { char *s, *e; - DEBUG("%s: pci_add reply: %s", vm->def->name, reply); + VIR_DEBUG("%s: pci_add reply: %s", vm->def->name, reply); /* If the command succeeds qemu prints: * OK bus 0, slot XXX... @@ -5322,16 +5322,70 @@ qemudParsePciAddReply(virDomainObjPtr vm, return 0; } -static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) +static int +qemudParseDriveAddReply(virDomainObjPtr vm, + const char *reply, + int *bus, + int *unit) +{ + char *s, *e; + + DEBUG("%s: drive_add reply: %s", vm->def->name, reply); + + /* If the command succeeds qemu prints: + * OK bus X, unit Y + */ + + if (!(s = strstr(reply, "OK "))) + return -1; + + s += 3; + + if (STRPREFIX(s, "bus ")) { + s += strlen("bus "); + + if (virStrToLong_i(s, &e, 10, bus) == -1) { + VIR_WARN(_("Unable to parse bus '%s'\n"), s); + return -1; + } + + if (!STRPREFIX(e, ", ")) { + VIR_WARN(_("Expected ', ' parsing drive_add reply '%s'\n"), s); + return -1; + } + s = e + 2; + } + + if (!STRPREFIX(s, "unit ")) { + VIR_WARN(_("Expected 'unit ' parsing drive_add reply '%s'\n"), s); + return -1; + } + s += strlen("bus "); + + if (virStrToLong_i(s, &e, 10, unit) == -1) { + VIR_WARN(_("Unable to parse unit number '%s'\n"), s); + return -1; + } + + return 0; +} + +static int qemudDomainAttachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { int ret, i; char *cmd, *reply; char *safe_path; + /* NOTE: Later patch will use type of the controller instead + of the disk */ const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus); int tryOldSyntax = 0; unsigned domain, bus, slot; + int bus_id, unit_id; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char* bus_unit_string; + int controller_specified; for (i = 0 ; i < vm->def->ndisks ; i++) { if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { @@ -5353,8 +5407,48 @@ try_command: return -1; } - ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + controller_specified = 0; + /* Partially specified PCI addresses (should not happen, anyway) + don't count as specification */ + if (dev->data.disk->controller_id != NULL || + (dev->data.disk->controller_pci_addr.domain != -1 && + dev->data.disk->controller_pci_addr.bus != -1 && + dev->data.disk->controller_pci_addr.slot != -1)) { + controller_specified = 1; + } + + if (controller_specified) { + /* NOTE: Proper check for the controller will be implemented + in a later commit */ + domain = dev->data.disk->controller_pci_addr.domain; + bus = dev->data.disk->controller_pci_addr.bus; + slot = dev->data.disk->controller_pci_addr.slot; + + if (dev->data.disk->bus_id != -1) { + virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); + } + + if (dev->data.disk->unit_id != -1) { + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); + } + + bus_unit_string = virBufferContentAndReset(&buf); + + VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, bus_unit_string); + ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, bus_unit_string); + } + else { + /* Old-style behaviour: If no <controller> reference is given, then + hotplug a new controller with the disk attached. */ + VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name, + (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", + (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + } + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); @@ -5370,26 +5464,149 @@ try_command: VIR_FREE(cmd); - if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; + if (controller_specified) { + if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + + if (dev->data.disk->bus_id == -1) { + dev->data.disk->bus_id = bus_id; + } + + if (dev->data.disk->unit_id == -1) { + dev->data.disk->unit_id = unit_id; + } + } else { + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + } + + dev->data.disk->pci_addr.domain = domain; + dev->data.disk->pci_addr.bus = bus; + dev->data.disk->pci_addr.slot = slot; + + virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + + + return 0; +} + +static int qemudDomainAttachDiskController(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret, i; + char *cmd, *reply; + char *tmp; + const char* type = virDomainDiskBusTypeToString(dev->data.controller->type); + int tryOldSyntax = 0; + int controllerAddressSpecified = 0; + unsigned domain, bus, slot; + + /* Test if the controller already exists, both by ID and + by PCI address */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if ((dev->data.controller->id && + STREQ(vm->def->controllers[i]->id, dev->data.controller->id)) || + (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot)) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller (id:%s, %d:%d:%d) already exists"), + dev->data.controller->id ? + dev->data.controller->id : "(none)", + dev->data.controller->pci_addr.domain, + dev->data.controller->pci_addr.bus, + dev->data.controller->pci_addr.slot); + return -1; } + } - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers+1) < 0) { + virReportOOMError(conn); return -1; } - VIR_FREE(reply); + if (dev->data.controller->pci_addr.domain != -1 && + dev->data.controller->pci_addr.bus != -1 && + dev->data.controller->pci_addr.slot != -1) { + controllerAddressSpecified = 1; + } - dev->data.disk->pci_addr.domain = domain; - dev->data.disk->pci_addr.bus = bus; - dev->data.disk->pci_addr.slot = slot; +try_command: + if (controllerAddressSpecified) { + domain = dev->data.controller->pci_addr.domain; + bus = dev->data.controller->pci_addr.bus; + slot = dev->data.controller->pci_addr.slot; + + ret = virAsprintf(&tmp, "%d:%d:%d", domain, bus, slot); + ret |= virAsprintf(&cmd, "pci_add %s%s storage if=%s", + (tryOldSyntax ? "" : "pci_addr="), tmp, type); + } else { + ret = virAsprintf(&cmd, "pci_add %s storage if=%s", + (tryOldSyntax ? "0" : "pci_addr=auto"), type); + } - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + if (ret == -1) { + virReportOOMError(conn); + return ret; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s controller"), type); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(cmd); + + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s controller failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + + /* Also fill in when the address was explicitely specified in + case qemu changed it (TODO: Can this really happen?) */ + dev->data.controller->pci_addr.domain = domain; + dev->data.controller->pci_addr.bus = bus; + dev->data.controller->pci_addr.slot = slot; + + VIR_FREE(reply); + + /* NOTE: Sort function is added in a later commit */ + vm->def->controllers[vm->def->ncontrollers++] = dev->data.controller; + /* qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks), + virDomainDiskQSort);*/ return 0; } @@ -5868,7 +6085,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom, ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, vm, dev); } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { - ret = qemudDomainAttachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainAttachDiskDevice(dom->conn, vm, dev); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk bus '%s' cannot be hotplugged."), -- 1.6.4
This enables to hot-add disk controllers without attached disks into the system. Previously, it was only possible to (implicitly) add disk controllers in the static machine configuration. Notice that the actual functionality is only available for qemu at present, but other emulators can be extended likewise. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.c | 26 +++++++++++++++++++++++--- src/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu_driver.c | 21 +++++++++++++++++---- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index d0fda64..ea51fda 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -647,7 +647,6 @@ void virDomainRemoveInactive(virDomainObjListPtr doms, } - /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2554,6 +2553,27 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, #endif +static int virDomainControllerCompare(virDomainControllerDefPtr a, + virDomainControllerDefPtr b) { + if (a->pci_addr.bus == b->pci_addr.bus) { + if (a->pci_addr.domain == b->pci_addr.domain) + return a->pci_addr.slot - b->pci_addr.slot; + + return a->pci_addr.domain - b->pci_addr.domain; + } + + return a->pci_addr.bus - b->pci_addr.bus; +} + + +int virDomainControllerQSort(const void *a, const void *b) +{ + const virDomainControllerDefPtr *da = a; + const virDomainControllerDefPtr *db = b; + + return virDomainControllerCompare(*da, *db); +} + int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { @@ -2935,8 +2955,8 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, def->controllers[def->ncontrollers++] = controller; } - /* qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers), - virDomainControllerQSort); */ + qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers), + virDomainControllerQSort); VIR_FREE(nodes); diff --git a/src/domain_conf.h b/src/domain_conf.h index 41df8f6..17f8b14 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -728,6 +728,8 @@ int virDomainDiskInsert(virDomainDefPtr def, void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr disk); +int virDomainControllerQSort(const void *a, const void *b); + int virDomainSaveXML(virConnectPtr conn, const char *configDir, virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f724493..ecc1123 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -86,6 +86,7 @@ virDomainDiskDefFree; virDomainDiskDeviceTypeToString; virDomainDiskInsert; virDomainDiskInsertPreAlloced; +virDomainControllerQSort; virDomainFindByID; virDomainFindByName; virDomainFindByUUID; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 4a30615..3bdd2d7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5424,6 +5424,15 @@ try_command: bus = dev->data.disk->controller_pci_addr.bus; slot = dev->data.disk->controller_pci_addr.slot; + if (dev->data.disk->controller_id) { + /* TODO: Obtain the PCI address of the controller + from the data structures using the ID */ + } else { + domain = dev->data.disk->controller_pci_addr.domain; + bus = dev->data.disk->controller_pci_addr.bus; + slot = dev->data.disk->controller_pci_addr.slot; + } + if (dev->data.disk->bus_id != -1) { virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); } @@ -5582,6 +5591,8 @@ try_command: VIR_FREE(cmd); + /* Naturally, the controller hotplug reply is identical with + any other PCI hotplug reply */ if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); @@ -5596,17 +5607,17 @@ try_command: } /* Also fill in when the address was explicitely specified in - case qemu changed it (TODO: Can this really happen?) */ + case qemu changed it */ dev->data.controller->pci_addr.domain = domain; dev->data.controller->pci_addr.bus = bus; dev->data.controller->pci_addr.slot = slot; VIR_FREE(reply); - /* NOTE: Sort function is added in a later commit */ vm->def->controllers[vm->def->ncontrollers++] = dev->data.controller; - /* qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks), - virDomainDiskQSort);*/ + qsort(vm->def->controllers, vm->def->ncontrollers, + sizeof(*vm->def->controllers), + virDomainControllerQSort); return 0; } @@ -6104,6 +6115,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom, virCgroupDenyDevicePath(cgroup, dev->data.disk->src); } + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + ret = qemudDomainAttachDiskController(dom->conn, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev, qemuCmdFlags); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:12PM +0200, Wolfgang Mauerer wrote:
This enables to hot-add disk controllers without attached disks into the system. Previously, it was only possible to (implicitly) add disk controllers in the static machine configuration.
Notice that the actual functionality is only available for qemu at present, but other emulators can be extended likewise.
Any idea what we can do about initial startup? eg, if we start a guest with 1 SCSI controller and 1 disk, and then we've then hotplugged a 2nd controller, and 1 disk. When we later boot or migrate the same guest, we need to have suitable QEMU command line args to make sure we get 2 controllers each with the same disk, rather than 1 controller with 2 disks. I'm not sure how we do this in QEMU, hopefully the existing args support it in some way I've not realized, or failing that the new qdev -device args might help us.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.c | 26 +++++++++++++++++++++++--- src/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu_driver.c | 21 +++++++++++++++++---- 4 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/src/domain_conf.c b/src/domain_conf.c index d0fda64..ea51fda 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -647,7 +647,6 @@ void virDomainRemoveInactive(virDomainObjListPtr doms,
}
- /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2554,6 +2553,27 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, #endif
+static int virDomainControllerCompare(virDomainControllerDefPtr a, + virDomainControllerDefPtr b) { + if (a->pci_addr.bus == b->pci_addr.bus) { + if (a->pci_addr.domain == b->pci_addr.domain) + return a->pci_addr.slot - b->pci_addr.slot; + + return a->pci_addr.domain - b->pci_addr.domain; + } + + return a->pci_addr.bus - b->pci_addr.bus; +} + + +int virDomainControllerQSort(const void *a, const void *b) +{ + const virDomainControllerDefPtr *da = a; + const virDomainControllerDefPtr *db = b; + + return virDomainControllerCompare(*da, *db); +} +
I know we used todo this for disk devices, but I'd recommand not going a qsort of devices when hotplugging/unplugging. For hotplug always append to the list, for unplug just shuffle down later devices in the list to fill the hole. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:12PM +0200, Wolfgang Mauerer wrote:
This enables to hot-add disk controllers without attached disks into the system. Previously, it was only possible to (implicitly) add disk controllers in the static machine configuration.
Notice that the actual functionality is only available for qemu at present, but other emulators can be extended likewise.
Any idea what we can do about initial startup? eg, if we start a guest with 1 SCSI controller and 1 disk, and then we've then hotplugged a 2nd controller, and 1 disk. When we later boot or migrate the same guest, we need to have suitable QEMU command line args to make sure we get 2 controllers each with the same disk, rather than 1 controller with 2 disks. I'm not sure how we do this in QEMU, hopefully the existing args support it in some way I've not realized, or failing that the new qdev -device args might help us.
I'm not yet sure how to solve that best. I'll take a look at how the -device can help exactly.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.c | 26 +++++++++++++++++++++++--- src/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu_driver.c | 21 +++++++++++++++++---- 4 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/src/domain_conf.c b/src/domain_conf.c index d0fda64..ea51fda 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -647,7 +647,6 @@ void virDomainRemoveInactive(virDomainObjListPtr doms,
}
- /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2554,6 +2553,27 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, #endif
+static int virDomainControllerCompare(virDomainControllerDefPtr a, + virDomainControllerDefPtr b) { + if (a->pci_addr.bus == b->pci_addr.bus) { + if (a->pci_addr.domain == b->pci_addr.domain) + return a->pci_addr.slot - b->pci_addr.slot; + + return a->pci_addr.domain - b->pci_addr.domain; + } + + return a->pci_addr.bus - b->pci_addr.bus; +} + + +int virDomainControllerQSort(const void *a, const void *b) +{ + const virDomainControllerDefPtr *da = a; + const virDomainControllerDefPtr *db = b; + + return virDomainControllerCompare(*da, *db); +} +
I know we used todo this for disk devices, but I'd recommand not going a qsort of devices when hotplugging/unplugging. For hotplug always append to the list, for unplug just shuffle down later devices in the list to fill the hole.
okay. Thanks, Wolfgang
... by simply traversing the list of controllers to find the associated PCI address. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu_driver.c | 41 +++++++++++++++++++++++++++++++---------- 1 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3bdd2d7..990f05a 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5418,19 +5418,40 @@ try_command: } if (controller_specified) { - /* NOTE: Proper check for the controller will be implemented - in a later commit */ - domain = dev->data.disk->controller_pci_addr.domain; - bus = dev->data.disk->controller_pci_addr.bus; - slot = dev->data.disk->controller_pci_addr.slot; - if (dev->data.disk->controller_id) { - /* TODO: Obtain the PCI address of the controller - from the data structures using the ID */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (STREQ(dev->data.disk->controller_id, + vm->def->controllers[i]->id)) { + break; + } + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; } else { domain = dev->data.disk->controller_pci_addr.domain; bus = dev->data.disk->controller_pci_addr.bus; slot = dev->data.disk->controller_pci_addr.slot; + + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (domain == vm->def->controllers[i]->pci_addr.domain && + bus == vm->def->controllers[i]->pci_addr.bus && + slot == vm->def->controllers[i]->pci_addr.slot) + break; + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } } if (dev->data.disk->bus_id != -1) { @@ -5457,13 +5478,13 @@ try_command: ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); } - + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); return ret; } - + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("cannot attach %s disk"), type); -- 1.6.4
When a disk is added without an explicitly specified controller as host, then try to find the first available controller. If none exists, do not (as in previous versions) add a new PCI controller device with the disk attached, but bail out with an error. Notice that this patch changes the behaviour as compared to older libvirt releases, as has been discussed on the mailing list (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860) Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu_driver.c | 172 ++++++++++++++++++++++++++--------------------------- 1 files changed, 85 insertions(+), 87 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 990f05a..f1c2a45 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5417,68 +5417,81 @@ try_command: controller_specified = 1; } - if (controller_specified) { - if (dev->data.disk->controller_id) { - for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (STREQ(dev->data.disk->controller_id, - vm->def->controllers[i]->id)) { - break; - } - } - - if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Controller does not exist")); - return -1; - } + if (!controller_specified) { + /* Find an appropriate controller for disk-hotadd. Notice that + the admissible controller types (SCSI, virtio) have already + been checked in qemudDomainAttachDevice. */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->type == dev->data.disk->type) + break; + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Cannot find appropriate controller for hot-add")); + return -1; + } - domain = vm->def->controllers[i]->pci_addr.domain; - bus = vm->def->controllers[i]->pci_addr.bus; - slot = vm->def->controllers[i]->pci_addr.slot; - } else { - domain = dev->data.disk->controller_pci_addr.domain; - bus = dev->data.disk->controller_pci_addr.bus; - slot = dev->data.disk->controller_pci_addr.slot; - - for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (domain == vm->def->controllers[i]->pci_addr.domain && - bus == vm->def->controllers[i]->pci_addr.bus && - slot == vm->def->controllers[i]->pci_addr.slot) - break; - } + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; + } - if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Controller does not exist")); - return -1; + if (controller_specified && dev->data.disk->controller_id) { + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (STREQ(dev->data.disk->controller_id, + vm->def->controllers[i]->id)) { + break; } - } - - if (dev->data.disk->bus_id != -1) { - virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); } - if (dev->data.disk->unit_id != -1) { - virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; + } else if (controller_specified) { + domain = dev->data.disk->controller_pci_addr.domain; + bus = dev->data.disk->controller_pci_addr.bus; + slot = dev->data.disk->controller_pci_addr.slot; + + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (domain == vm->def->controllers[i]->pci_addr.domain && + bus == vm->def->controllers[i]->pci_addr.bus && + slot == vm->def->controllers[i]->pci_addr.slot) + break; } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + } - bus_unit_string = virBufferContentAndReset(&buf); - - VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, - domain, bus, slot, safe_path, type, bus_unit_string); - ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", - (tryOldSyntax ? "" : "pci_addr="), domain, bus, - slot, safe_path, type, bus_unit_string); + /* At this point, we have a valid (domain, bus, slot) triple + that identifies the controller, regardless if an explicit + controller has been given or not. */ + if (dev->data.disk->bus_id != -1) { + virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); } - else { - /* Old-style behaviour: If no <controller> reference is given, then - hotplug a new controller with the disk attached. */ - VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name, - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); - ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + + if (dev->data.disk->unit_id != -1) { + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); } + bus_unit_string = virBufferContentAndReset(&buf); + + VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, bus_unit_string); + ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, bus_unit_string); + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); @@ -5494,41 +5507,26 @@ try_command: VIR_FREE(cmd); - if (controller_specified) { - if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; - } - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); - return -1; - } - - if (dev->data.disk->bus_id == -1) { - dev->data.disk->bus_id = bus_id; - } - - if (dev->data.disk->unit_id == -1) { - dev->data.disk->unit_id = unit_id; - } - } else { - if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; - } - - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); - return -1; - } + if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; } - + + if (dev->data.disk->bus_id == -1) { + dev->data.disk->bus_id = bus_id; + } + + if (dev->data.disk->unit_id == -1) { + dev->data.disk->unit_id = unit_id; + } + dev->data.disk->pci_addr.domain = domain; dev->data.disk->pci_addr.bus = bus; dev->data.disk->pci_addr.slot = slot; -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:14PM +0200, Wolfgang Mauerer wrote:
When a disk is added without an explicitly specified controller as host, then try to find the first available controller. If none exists, do not (as in previous versions) add a new PCI controller device with the disk attached, but bail out with an error. Notice that this patch changes the behaviour as compared to older libvirt releases, as has been discussed on the mailing list (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860)
Yes, I still think is the good way to go Daniel
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu_driver.c | 172 ++++++++++++++++++++++++++--------------------------- 1 files changed, 85 insertions(+), 87 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 990f05a..f1c2a45 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5417,68 +5417,81 @@ try_command: controller_specified = 1; }
- if (controller_specified) { - if (dev->data.disk->controller_id) { - for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (STREQ(dev->data.disk->controller_id, - vm->def->controllers[i]->id)) { - break; - } - } - - if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Controller does not exist")); - return -1; - } + if (!controller_specified) { + /* Find an appropriate controller for disk-hotadd. Notice that + the admissible controller types (SCSI, virtio) have already + been checked in qemudDomainAttachDevice. */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->type == dev->data.disk->type) + break; + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Cannot find appropriate controller for hot-add")); + return -1; + }
- domain = vm->def->controllers[i]->pci_addr.domain; - bus = vm->def->controllers[i]->pci_addr.bus; - slot = vm->def->controllers[i]->pci_addr.slot; - } else { - domain = dev->data.disk->controller_pci_addr.domain; - bus = dev->data.disk->controller_pci_addr.bus; - slot = dev->data.disk->controller_pci_addr.slot; - - for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (domain == vm->def->controllers[i]->pci_addr.domain && - bus == vm->def->controllers[i]->pci_addr.bus && - slot == vm->def->controllers[i]->pci_addr.slot) - break; - } + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; + }
- if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Controller does not exist")); - return -1; + if (controller_specified && dev->data.disk->controller_id) { + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (STREQ(dev->data.disk->controller_id, + vm->def->controllers[i]->id)) { + break; } - } - - if (dev->data.disk->bus_id != -1) { - virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); }
- if (dev->data.disk->unit_id != -1) { - virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; + } else if (controller_specified) { + domain = dev->data.disk->controller_pci_addr.domain; + bus = dev->data.disk->controller_pci_addr.bus; + slot = dev->data.disk->controller_pci_addr.slot; + + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (domain == vm->def->controllers[i]->pci_addr.domain && + bus == vm->def->controllers[i]->pci_addr.bus && + slot == vm->def->controllers[i]->pci_addr.slot) + break; } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + }
- bus_unit_string = virBufferContentAndReset(&buf); - - VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, - domain, bus, slot, safe_path, type, bus_unit_string); - ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", - (tryOldSyntax ? "" : "pci_addr="), domain, bus, - slot, safe_path, type, bus_unit_string); + /* At this point, we have a valid (domain, bus, slot) triple + that identifies the controller, regardless if an explicit + controller has been given or not. */ + if (dev->data.disk->bus_id != -1) { + virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); } - else { - /* Old-style behaviour: If no <controller> reference is given, then - hotplug a new controller with the disk attached. */ - VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name, - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); - ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + + if (dev->data.disk->unit_id != -1) { + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); }
+ bus_unit_string = virBufferContentAndReset(&buf); + + VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, bus_unit_string); + ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, bus_unit_string); + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); @@ -5494,41 +5507,26 @@ try_command:
VIR_FREE(cmd);
- if (controller_specified) { - if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; - } - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); - return -1; - } - - if (dev->data.disk->bus_id == -1) { - dev->data.disk->bus_id = bus_id; - } - - if (dev->data.disk->unit_id == -1) { - dev->data.disk->unit_id = unit_id; - } - } else { - if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; - } - - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); - return -1; - } + if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; } - + + if (dev->data.disk->bus_id == -1) { + dev->data.disk->bus_id = bus_id; + } + + if (dev->data.disk->unit_id == -1) { + dev->data.disk->unit_id = unit_id; + } + dev->data.disk->pci_addr.domain = domain; dev->data.disk->pci_addr.bus = bus; dev->data.disk->pci_addr.slot = slot; -- 1.6.4
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
We need this multiple times later on. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu_driver.c | 155 +++++++++++++++++++++++++++++++++-------------------- 1 files changed, 97 insertions(+), 58 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f1c2a45..ddc46f6 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5370,44 +5370,20 @@ qemudParseDriveAddReply(virDomainObjPtr vm, return 0; } -static int qemudDomainAttachDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) -{ - int ret, i; - char *cmd, *reply; - char *safe_path; - /* NOTE: Later patch will use type of the controller instead - of the disk */ - const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus); - int tryOldSyntax = 0; - unsigned domain, bus, slot; - int bus_id, unit_id; - virBuffer buf = VIR_BUFFER_INITIALIZER; - char* bus_unit_string; - int controller_specified; - - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), dev->data.disk->dst); - return -1; - } - } - - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { - virReportOOMError(conn); - return -1; - } - -try_command: - safe_path = qemudEscapeMonitorArg(dev->data.disk->src); - if (!safe_path) { - virReportOOMError(conn); - return -1; - } +enum { + CONTROLLER_FOUND = 0, + NO_CONTROLLER_FOUND = -1, /* None specified, and none found */ + CONTROLLER_INEXISTENT = -2, /* Controller specified, but not found */ +}; +static virDomainControllerDefPtr +qemudGetDiskController(virDomainObjPtr vm, virDomainDeviceDefPtr dev, + int* ret) { + int controller_specified = 0; + int domain, bus, slot; + virDomainControllerDefPtr conPtr = NULL; + int i; + *ret = CONTROLLER_FOUND; - controller_specified = 0; /* Partially specified PCI addresses (should not happen, anyway) don't count as specification */ if (dev->data.disk->controller_id != NULL || @@ -5422,38 +5398,28 @@ try_command: the admissible controller types (SCSI, virtio) have already been checked in qemudDomainAttachDevice. */ for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (vm->def->controllers[i]->type == dev->data.disk->type) + if (vm->def->controllers[i]->type == dev->data.disk->type) + conPtr = vm->def->controllers[i]; break; } - if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Cannot find appropriate controller for hot-add")); - return -1; + if (!conPtr) { + *ret = NO_CONTROLLER_FOUND; } - - domain = vm->def->controllers[i]->pci_addr.domain; - bus = vm->def->controllers[i]->pci_addr.bus; - slot = vm->def->controllers[i]->pci_addr.slot; } if (controller_specified && dev->data.disk->controller_id) { for (i = 0 ; i < vm->def->ncontrollers ; i++) { if (STREQ(dev->data.disk->controller_id, vm->def->controllers[i]->id)) { + conPtr = vm->def->controllers[i]; break; } } - if (i == vm->def->ncontrollers) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("Controller does not exist")); - return -1; + if (!conPtr) { + *ret = CONTROLLER_INEXISTENT; } - - domain = vm->def->controllers[i]->pci_addr.domain; - bus = vm->def->controllers[i]->pci_addr.bus; - slot = vm->def->controllers[i]->pci_addr.slot; } else if (controller_specified) { domain = dev->data.disk->controller_pci_addr.domain; bus = dev->data.disk->controller_pci_addr.bus; @@ -5463,19 +5429,92 @@ try_command: if (domain == vm->def->controllers[i]->pci_addr.domain && bus == vm->def->controllers[i]->pci_addr.bus && slot == vm->def->controllers[i]->pci_addr.slot) + conPtr = vm->def->controllers[i]; break; } - if (i == vm->def->ncontrollers) { + if (!conPtr) { + *ret = CONTROLLER_INEXISTENT; + } + } + + return conPtr; +} + +static int qemudDomainAttachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret, i; + char *cmd, *reply; + char *safe_path; + const char* type = NULL; + int tryOldSyntax = 0; + int bus_id, unit_id; + int domain, bus, slot; + virDomainControllerDefPtr conPtr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char* bus_unit_string; + + /* Both controller and disk can specify a type like SCSI. While + a virtual disk as such is not bound to a specific bus type, + the specification is here for historical reasons. When controller + and disk type specification disagree, the former takes precedence + and we print a warning message, but still add the disk. */ + if (virDiskHasValidController(dev->data.disk)) { + conPtr = qemudGetDiskController(vm, dev, &ret); + if (conPtr && dev->data.disk->bus != conPtr->type) { + VIR_WARN0(_("Controller and disk bus type disagree, controller " \ + "takes precedence\n")); + type = virDomainDiskBusTypeToString(conPtr->type); + } + } + if (!type) { + type = virDomainDiskBusTypeToString(dev->data.disk->bus); + } + + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("target %s already exists"), dev->data.disk->dst); + return -1; + } + } + + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { + virReportOOMError(conn); + return -1; + } + +try_command: + safe_path = qemudEscapeMonitorArg(dev->data.disk->src); + if (!safe_path) { + virReportOOMError(conn); + return -1; + } + + conPtr = qemudGetDiskController(vm, dev, &ret); + if (!conPtr) { + switch (ret) { + case CONTROLLER_INEXISTENT: qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("Controller does not exist")); return -1; + break; /* A bit pointless */ + case NO_CONTROLLER_FOUND: + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Cannot find appropriate controller for hot-add")); + return -1; + break; } } - /* At this point, we have a valid (domain, bus, slot) triple - that identifies the controller, regardless if an explicit - controller has been given or not. */ + /* At this point, we have a valid controller, regardless if an explicit + controller has been specified or not. */ + domain = conPtr->pci_addr.domain; + bus = conPtr->pci_addr.bus; + slot = conPtr->pci_addr.slot; + if (dev->data.disk->bus_id != -1) { virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); } -- 1.6.4
Since both disks and disk controllers can be dynamically added to the system, it makes sense to be also able to remove them. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/domain_conf.h | 8 +++ src/qemu_driver.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 153 insertions(+), 16 deletions(-) diff --git a/src/domain_conf.h b/src/domain_conf.h index 17f8b14..c7d49cf 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; } +static inline int +virDiskHasValidController(virDomainDiskDefPtr def) +{ + return def->controller_id != NULL || + def->controller_pci_addr.domain || def->controller_pci_addr.domain + || def->controller_pci_addr.slot; +} + /* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ddc46f6..5ac89a1 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -6204,32 +6204,31 @@ cleanup: return ret; } -static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +static int qemudDomainDetachDiskController(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int i, ret = -1; char *cmd = NULL; char *reply = NULL; - virDomainDiskDefPtr detach = NULL; + virDomainControllerDefPtr detach = NULL; int tryOldSyntax = 0; - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { - detach = vm->def->disks[i]; +// virDomainControllerDefPtr is dev->data.controller + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot) { + detach = vm->def->controllers[i]; break; } } if (!detach) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - - if (!virDiskHasValidPciAddr(detach)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - no PCI address for device"), - detach->dst); + _("Controller %s not found"), dev->data.disk->dst); goto cleanup; } @@ -6251,7 +6250,9 @@ try_command: if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to execute detach disk %s command"), detach->dst); + _("failed to execute detach controller %d:%d:%d " \ + "command"), detach->pci_addr.domain, + detach->pci_addr.bus, detach->pci_addr.slot); goto cleanup; } @@ -6267,6 +6268,124 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach controller: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); + goto cleanup; + } + + if (vm->def->ncontrollers > 1) { + vm->def->controllers[i] = vm->def->controllers[--vm->def->ncontrollers]; + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers) < 0) { + virReportOOMError(conn); + goto cleanup; + } + qsort(vm->def->disks, vm->def->ncontrollers, + sizeof(*vm->def->controllers), + virDomainControllerQSort); + } else { + VIR_FREE(vm->def->controllers[0]); + vm->def->controllers = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + +static int qemudDomainDetachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + const char* type; + virDomainDiskDefPtr detach = NULL; + int tryOldSyntax = 0; + + /* If bus and unit are specified, use these first to identify + the disk */ + if (dev->data.disk->controller_pci_addr.domain != -1) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (dev->data.disk->bus_id != -1 && + dev->data.disk->bus_id == vm->def->disks[i]->bus_id && + dev->data.disk->unit_id != -1 && + dev->data.disk->unit_id == vm->def->disks[i]->unit_id) { + detach = vm->def->disks[i]; + break; + } + } + } + + /* If the device did not specify a controller explicitely (and therefore + lacks a bus and unit specification), revert to the explicit target + device specification to identify a device for removal. */ + if (!detach) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + detach = vm->def->disks[i]; + break; + } + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + goto cleanup; + } + + if (!virDiskHasValidPciAddr(detach) && !virDiskHasValidController(detach)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); + goto cleanup; + } + + type = virDomainDiskBusTypeToString(detach->bus); + +try_command: + if (tryOldSyntax) { + if (virAsprintf(&cmd, "drive_del 0 %d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + if (virAsprintf(&cmd, "drive_del pci_addr=%d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to execute detach disk %s command"), detach->dst); + goto cleanup; + } + + DEBUG ("%s: drive_del reply: %s",vm->def->name, reply); + + if (!tryOldSyntax && + strstr(reply, "extraneous characters")) { + tryOldSyntax = 1; + goto try_command; + } + /* OK bux x, unit x is printed on success, but this does not provide + any new information to us.*/ + if (strstr(reply, "Invalid pci address") || + strstr(reply, "No pci device with address")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), detach->dst, detach->pci_addr.domain, @@ -6274,6 +6393,11 @@ try_command: detach->pci_addr.slot, reply); goto cleanup; + } else if (strstr(reply, "Need to specify bus") || + strstr(reply, "Need to specify unit")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach disk %s: bus and unit not (internal error)"), + detach->dst); } if (vm->def->ndisks > 1) { @@ -6575,7 +6699,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) { - ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainDetachDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) @@ -6584,6 +6708,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + /* NOTE: We can unplug the controller without having to + check if all disks are unplugged; it is at the user's + responsibility to use the required OS services first. */ + ret = qemudDomainDetachDiskController(dom->conn, vm, dev); } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("only SCSI or virtio disk device can be detached dynamically")); -- 1.6.4
On Fri, Sep 18, 2009 at 05:26:16PM +0200, Wolfgang Mauerer wrote:
Since both disks and disk controllers can be dynamically added to the system, it makes sense to be also able to remove them.
This is the main patch which requires additional support in QEMu if I'm understanding your patcheset to qemu-devel correctly.
diff --git a/src/domain_conf.h b/src/domain_conf.h index 17f8b14..c7d49cf 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; }
+static inline int +virDiskHasValidController(virDomainDiskDefPtr def) +{ + return def->controller_id != NULL || + def->controller_pci_addr.domain || def->controller_pci_addr.domain + || def->controller_pci_addr.slot; +} +
/* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ddc46f6..5ac89a1 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -6204,32 +6204,31 @@ cleanup: return ret; }
-static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +static int qemudDomainDetachDiskController(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int i, ret = -1; char *cmd = NULL; char *reply = NULL; - virDomainDiskDefPtr detach = NULL; + virDomainControllerDefPtr detach = NULL; int tryOldSyntax = 0;
- for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { - detach = vm->def->disks[i]; +// virDomainControllerDefPtr is dev->data.controller + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot) { + detach = vm->def->controllers[i]; break; } }
if (!detach) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - - if (!virDiskHasValidPciAddr(detach)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - no PCI address for device"), - detach->dst); + _("Controller %s not found"), dev->data.disk->dst); goto cleanup; }
@@ -6251,7 +6250,9 @@ try_command:
if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to execute detach disk %s command"), detach->dst); + _("failed to execute detach controller %d:%d:%d " \ + "command"), detach->pci_addr.domain, + detach->pci_addr.bus, detach->pci_addr.slot); goto cleanup; }
@@ -6267,6 +6268,124 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach controller: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); + goto cleanup; + } + + if (vm->def->ncontrollers > 1) { + vm->def->controllers[i] = vm->def->controllers[--vm->def->ncontrollers]; + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers) < 0) { + virReportOOMError(conn); + goto cleanup; + } + qsort(vm->def->disks, vm->def->ncontrollers, + sizeof(*vm->def->controllers), + virDomainControllerQSort); + } else { + VIR_FREE(vm->def->controllers[0]); + vm->def->controllers = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + +static int qemudDomainDetachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + const char* type; + virDomainDiskDefPtr detach = NULL; + int tryOldSyntax = 0; + + /* If bus and unit are specified, use these first to identify + the disk */ + if (dev->data.disk->controller_pci_addr.domain != -1) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (dev->data.disk->bus_id != -1 && + dev->data.disk->bus_id == vm->def->disks[i]->bus_id && + dev->data.disk->unit_id != -1 && + dev->data.disk->unit_id == vm->def->disks[i]->unit_id) { + detach = vm->def->disks[i]; + break; + } + } + } + + /* If the device did not specify a controller explicitely (and therefore + lacks a bus and unit specification), revert to the explicit target + device specification to identify a device for removal. */ + if (!detach) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + detach = vm->def->disks[i]; + break; + } + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + goto cleanup; + } + + if (!virDiskHasValidPciAddr(detach) && !virDiskHasValidController(detach)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); + goto cleanup; + } + + type = virDomainDiskBusTypeToString(detach->bus); + +try_command: + if (tryOldSyntax) { + if (virAsprintf(&cmd, "drive_del 0 %d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + if (virAsprintf(&cmd, "drive_del pci_addr=%d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to execute detach disk %s command"), detach->dst); + goto cleanup; + } + + DEBUG ("%s: drive_del reply: %s",vm->def->name, reply); + + if (!tryOldSyntax && + strstr(reply, "extraneous characters")) { + tryOldSyntax = 1; + goto try_command; + } + /* OK bux x, unit x is printed on success, but this does not provide + any new information to us.*/ + if (strstr(reply, "Invalid pci address") || + strstr(reply, "No pci device with address")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), detach->dst, detach->pci_addr.domain, @@ -6274,6 +6393,11 @@ try_command: detach->pci_addr.slot, reply); goto cleanup; + } else if (strstr(reply, "Need to specify bus") || + strstr(reply, "Need to specify unit")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach disk %s: bus and unit not (internal error)"), + detach->dst); }
if (vm->def->ndisks > 1) { @@ -6575,7 +6699,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) { - ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainDetachDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) @@ -6584,6 +6708,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + /* NOTE: We can unplug the controller without having to + check if all disks are unplugged; it is at the user's + responsibility to use the required OS services first. */ + ret = qemudDomainDetachDiskController(dom->conn, vm, dev);
I wonder if we'd be better off raising an error if the app tries to remove a controller which still has disks attached. ie, force the app to hotunplug each disk first. These days I tend towards avoiding side-effects in operations, and reporting errors whereever there's a situation that the app could have avoided the problem. Any other opinions people... ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:16PM +0200, Wolfgang Mauerer wrote:
Since both disks and disk controllers can be dynamically added to the system, it makes sense to be also able to remove them.
This is the main patch which requires additional support in QEMu if I'm understanding your patcheset to qemu-devel correctly. Yes, that's right.
diff --git a/src/domain_conf.h b/src/domain_conf.h index 17f8b14..c7d49cf 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; }
+static inline int +virDiskHasValidController(virDomainDiskDefPtr def) +{ + return def->controller_id != NULL || + def->controller_pci_addr.domain || def->controller_pci_addr.domain + || def->controller_pci_addr.slot; +} +
/* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ddc46f6..5ac89a1 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -6204,32 +6204,31 @@ cleanup: return ret; }
-static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +static int qemudDomainDetachDiskController(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int i, ret = -1; char *cmd = NULL; char *reply = NULL; - virDomainDiskDefPtr detach = NULL; + virDomainControllerDefPtr detach = NULL; int tryOldSyntax = 0;
- for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { - detach = vm->def->disks[i]; +// virDomainControllerDefPtr is dev->data.controller + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot) { + detach = vm->def->controllers[i]; break; } }
if (!detach) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - - if (!virDiskHasValidPciAddr(detach)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - no PCI address for device"), - detach->dst); + _("Controller %s not found"), dev->data.disk->dst); goto cleanup; }
@@ -6251,7 +6250,9 @@ try_command:
if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to execute detach disk %s command"), detach->dst); + _("failed to execute detach controller %d:%d:%d " \ + "command"), detach->pci_addr.domain, + detach->pci_addr.bus, detach->pci_addr.slot); goto cleanup; }
@@ -6267,6 +6268,124 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach controller: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); + goto cleanup; + } + + if (vm->def->ncontrollers > 1) { + vm->def->controllers[i] = vm->def->controllers[--vm->def->ncontrollers]; + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers) < 0) { + virReportOOMError(conn); + goto cleanup; + } + qsort(vm->def->disks, vm->def->ncontrollers, + sizeof(*vm->def->controllers), + virDomainControllerQSort); + } else { + VIR_FREE(vm->def->controllers[0]); + vm->def->controllers = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + +static int qemudDomainDetachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + const char* type; + virDomainDiskDefPtr detach = NULL; + int tryOldSyntax = 0; + + /* If bus and unit are specified, use these first to identify + the disk */ + if (dev->data.disk->controller_pci_addr.domain != -1) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (dev->data.disk->bus_id != -1 && + dev->data.disk->bus_id == vm->def->disks[i]->bus_id && + dev->data.disk->unit_id != -1 && + dev->data.disk->unit_id == vm->def->disks[i]->unit_id) { + detach = vm->def->disks[i]; + break; + } + } + } + + /* If the device did not specify a controller explicitely (and therefore + lacks a bus and unit specification), revert to the explicit target + device specification to identify a device for removal. */ + if (!detach) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + detach = vm->def->disks[i]; + break; + } + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + goto cleanup; + } + + if (!virDiskHasValidPciAddr(detach) && !virDiskHasValidController(detach)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); + goto cleanup; + } + + type = virDomainDiskBusTypeToString(detach->bus); + +try_command: + if (tryOldSyntax) { + if (virAsprintf(&cmd, "drive_del 0 %d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + if (virAsprintf(&cmd, "drive_del pci_addr=%d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to execute detach disk %s command"), detach->dst); + goto cleanup; + } + + DEBUG ("%s: drive_del reply: %s",vm->def->name, reply); + + if (!tryOldSyntax && + strstr(reply, "extraneous characters")) { + tryOldSyntax = 1; + goto try_command; + } + /* OK bux x, unit x is printed on success, but this does not provide + any new information to us.*/ + if (strstr(reply, "Invalid pci address") || + strstr(reply, "No pci device with address")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), detach->dst, detach->pci_addr.domain, @@ -6274,6 +6393,11 @@ try_command: detach->pci_addr.slot, reply); goto cleanup; + } else if (strstr(reply, "Need to specify bus") || + strstr(reply, "Need to specify unit")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach disk %s: bus and unit not (internal error)"), + detach->dst); }
if (vm->def->ndisks > 1) { @@ -6575,7 +6699,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) { - ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainDetachDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) @@ -6584,6 +6708,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + /* NOTE: We can unplug the controller without having to + check if all disks are unplugged; it is at the user's + responsibility to use the required OS services first. */ + ret = qemudDomainDetachDiskController(dom->conn, vm, dev);
I wonder if we'd be better off raising an error if the app tries to remove a controller which still has disks attached. ie, force the app to hotunplug each disk first. These days I tend towards avoiding side-effects in operations, and reporting errors whereever there's a situation that the app could have avoided the problem. Any other opinions people... ?
I'm not sure if this is worth the extra effort: The disk needs to be removed/unmounted/unplugged from the underlying operating system before it can be removed from the hypervisor (and before the controller is unplugged), but I don't see a way for libvirt to check if the required operations have been performed or not. So if the user does things wrong and removes the disk from the hypervisor without any preparatory work in the guest OS, we will get errors in any case. If, on the other hand, the user does things right (i.e, remove the disk from the OS and then unplug the controller), removing the disks from libvirt is just an extra step. And with real hardware, I would suppose that it's possible to remove a controller with attached disks of the controller allows for PCI hotplug. No clear preference for any of these options from my side, but a slight bias towards not emitting an error message. Any other opinions people... ? ;-) Thanks, Wolfgang
On Fri, Sep 18, 2009 at 05:26:07PM +0200, Wolfgang Mauerer wrote:
Hi,
Hi Wolfgang,
this patch reworks libvirt's disk-hotadd support and introduces support for disk controller hotplugging and disk-hotremove (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860 for more details). Currently, it targets only qemu and also requires some additions to qemu that have only recently been submitted, which is why this is cross-posted to qemu-devel. Hopefully the lack of support in qemu does not prevent the community from reviewing the code, though ;-)
Could you update us on the status for those patches, is there any chance to get this in QEmu upstream ? I have only seen feddback from Gerd Hoffmann on that thread but maybe I'm missing some of the action ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/09/09 18:32, Daniel Veillard wrote:
On Fri, Sep 18, 2009 at 05:26:07PM +0200, Wolfgang Mauerer wrote:
Hi,
Hi Wolfgang,
this patch reworks libvirt's disk-hotadd support and introduces support for disk controller hotplugging and disk-hotremove (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860 for more details). Currently, it targets only qemu and also requires some additions to qemu that have only recently been submitted, which is why this is cross-posted to qemu-devel. Hopefully the lack of support in qemu does not prevent the community from reviewing the code, though ;-)
Could you update us on the status for those patches, is there any chance to get this in QEmu upstream ? I have only seen feddback from Gerd Hoffmann on that thread but maybe I'm missing some of the action !
Note: upstream qemu got device_add and device_del monitor commands meanwhile which can be used to hotplug devices. device_add accepts the same syntax the -device command line switch expects. device_del expectes an id. For libvirt it probably makes sense to start supporting this when switching over to -device (probably for qemu 0.12+). cheers, Gerd
Hi, Gerd Hoffmann wrote:
On 10/09/09 18:32, Daniel Veillard wrote:
On Fri, Sep 18, 2009 at 05:26:07PM +0200, Wolfgang Mauerer wrote:
Hi, Hi Wolfgang,
this patch reworks libvirt's disk-hotadd support and introduces support for disk controller hotplugging and disk-hotremove (see http://thread.gmane.org/gmane.comp.emulators.libvirt/15860 for more details). Currently, it targets only qemu and also requires some additions to qemu that have only recently been submitted, which is why this is cross-posted to qemu-devel. Hopefully the lack of support in qemu does not prevent the community from reviewing the code, though ;-) Could you update us on the status for those patches, is there any chance to get this in QEmu upstream ? I have only seen feddback from Gerd Hoffmann on that thread but maybe I'm missing some of the action !
sorry, I did not get that mail for some reason, so I did not reply.
Note: upstream qemu got device_add and device_del monitor commands meanwhile which can be used to hotplug devices.
device_add accepts the same syntax the -device command line switch expects.
device_del expectes an id.
For libvirt it probably makes sense to start supporting this when switching over to -device (probably for qemu 0.12+).
Since it does most likely not make sense to include my drive_del implementation into qemu 0.11 mainline, I'll add libvirt support for these after I finish doing the structural changes suggested by Daniel Berrange. Cheers, Wolfgang
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Gerd Hoffmann -
Matthias Bolte -
Wolfgang Mauerer