[libvirt] [PATCH v2] Disk- and Controller Hotplug

Hi, this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold: 1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks. This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission. Best, Wolfgang daemon/libvirtd.c | 2 +- docs/formatdomain.html.in | 38 +++++ docs/schemas/domain.rng | 158 ++++++++++++++++----- src/conf/domain_conf.c | 314 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 62 ++++++++- src/libvirt_private.syms | 7 +- src/qemu/qemu_driver.c | 184 +++++++++++++++++++++++-- src/qemu/qemu_monitor_text.c | 197 ++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 7 + 9 files changed, 910 insertions(+), 59 deletions(-) -- Siemens AG, CT T DE Corporate Competence Centre Embedded Linux

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> The configuration file setting is overriden by -f or --config, but not with -c Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- daemon/libvirtd.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index daf06bc..5cedd17 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2931,7 +2931,7 @@ libvirt management daemon:\n\ \n\ Default paths:\n\ \n\ - Configuration file (unless overridden by -c):\n\ + Configuration file (unless overridden by -f):\n\ " SYSCONF_DIR "/libvirt/libvirtd.conf\n\ \n\ Sockets (as root):\n\ -- 1.6.4

On Tue, Nov 17, 2009 at 12:53:32AM +0100, wolfgang.mauerer@siemens.com wrote:
From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
The configuration file setting is overriden by -f or --config, but not with -c
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- daemon/libvirtd.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index daf06bc..5cedd17 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2931,7 +2931,7 @@ libvirt management daemon:\n\ \n\ Default paths:\n\ \n\ - Configuration file (unless overridden by -c):\n\ + Configuration file (unless overridden by -f):\n\ " SYSCONF_DIR "/libvirt/libvirtd.conf\n\ \n\ Sockets (as root):\n\ --
ACK, I pushed this trivial fix 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 :|

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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 310376b..fb81ace 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 Tue, Nov 17, 2009 at 12:53:33AM +0100, wolfgang.mauerer@siemens.com wrote:
From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
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 310376b..fb81ace 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. #
--
Also pushed this trivial docs fix 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 :|

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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 name="<identifier>" pci_addr="<addr>" bus="<number>" unit="<number>"/> </disk> Normally the name of the controller must be specified. Using pci_addr instead is supported for qemu guests that don't define a controller in their main configuration file and cannot hotplug controllers. When the controller has been brought into the system via the hotplug mechanism also included in this patch series, it will have an ID. 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/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++- 2 files changed, 33 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28bee54..ecddb68 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -724,7 +724,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; @@ -768,12 +773,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"); @@ -874,6 +885,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/conf/domain_conf.h b/src/conf/domain_conf.h index fadf43f..c034ae4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -107,9 +107,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

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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" name="<my_id>"> <address type="pci" domain="0xNUM" bus="0xNUM" slot="0xNUM"/> </controller> where type denotes the disk interface (scsi, ide,...), name is an arbitrary string that identifies the controller for disk hotadd/remove, and the <address> element specifies the controller address on the PCI bus. Domain, bus and slot are specified as hexadecimal numbers. The address 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 | 158 ++++++++++++++++++------ src/conf/domain_conf.c | 279 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 48 +++++++- src/libvirt_private.syms | 1 - src/qemu/qemu_driver.c | 1 - src/qemu/qemu_monitor_text.c | 47 +++++++ 6 files changed, 488 insertions(+), 46 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index b75f17e..0d5fb47 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -393,49 +393,129 @@ <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="name"> + <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="address"> + <attribute name="type"> + <ref name="adressType"/> + </attribute> + <optional> + <attribute name="domain"> + <ref name="pciDomain"/> + </attribute> + </optional> + <attribute name="bus"> + <ref name="pciBus"/> + </attribute> + <attribute name="slot"> + <ref name="pciSlot"/> + </attribute> + <attribute name="function"> + <ref name="pciFunc"/> + </attribute> + </element> + </optional> </element> </define> <define name="target"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ecddb68..4e2b84e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -80,6 +80,7 @@ VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST, VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "disk", + "controller", "filesystem", "interface", "input", @@ -88,6 +89,9 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "hostdev", "watchdog") +VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, + "pci") + VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST, "block", "file") @@ -342,6 +346,16 @@ void virDomainInputDefFree(virDomainInputDefPtr def) VIR_FREE(def); } +void virDomainDeviceAddressFree(virDomainDeviceAddressPtr def) +{ + if (!def) + return; + + VIR_FREE(def->data); + + VIR_FREE(def); +} + void virDomainDiskDefFree(virDomainDiskDefPtr def) { if (!def) @@ -357,6 +371,18 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def); } +void virDomainControllerDefFree(virDomainControllerDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->type); + VIR_FREE(def->name); + VIR_FREE(def->address); + + VIR_FREE(def); +} + void virDomainFSDefFree(virDomainFSDefPtr def) { if (!def) @@ -708,6 +734,152 @@ void virDomainRemoveInactive(virDomainObjListPtr doms, virHashRemoveEntry(doms->objs, uuidstr, virDomainObjListDeallocator); } +/* Generate a string representation of a device address + * @param address Device address to stringify + */ +const char *virDomainFormatDeviceAddress(virDomainDeviceAddressPtr address) { + int ret; + char *tmp; + + if (!address) { + return "(undefined)"; + } + + switch (address->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + ret = virAsprintf(&tmp, "%x:%x:%x", address->data.pci.domain, + address->data.pci.bus, address->data.pci.slot); + if (!ret) + return tmp; + break; + default: + return "(unknown)"; + } + + return "(error)"; +} + +/* Compare two device addresses. Returns true if addresses are + * identical and false if the they differ (or are of different types) + * @param a, @para b Pointers to the addresses + */ +int virDomainCompareDeviceAddresses(virDomainDeviceAddressPtr a, + virDomainDeviceAddressPtr b) { + if (!a || !b || a->type != b-> type) { + return 0; + } + + switch (a->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + if (a->data.pci.domain == b->data.pci.domain && + a->data.pci.bus == b->data.pci.bus && + a->data.pci.slot == b->data.pci.slot) { + return 1; + } + + return 0; + } + + return 0; +} + +/* Parse the XML definition for a device address + * @param node XML nodeset to parse for device address definition + */ +static virDomainDeviceAddressPtr +virDomainDeviceAddressParseXML(virConnectPtr conn, + xmlNodePtr node) { + virDomainDeviceAddressPtr def; + char *type = NULL; + char *domain, *slot, *bus, *function; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(conn); + return NULL; + } + + type = virXMLPropString(node, "type"); + + if (type) { + if ((def->type = virDomainDeviceAddressTypeFromString(type)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown address type '%s'"), type); + goto error; + } + } else { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("No type specified for device address")); + goto error; + } + + switch (def->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + domain = virXMLPropString(node, "domain"); + bus = virXMLPropString(node, "bus"); + slot = virXMLPropString(node, "slot"); + function = virXMLPropString(node, "function"); + + def->data.pci.domain = -1; + if (domain && + virStrToLong_i(domain, NULL, 16, &def->data.pci.domain) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'domain' attribute")); + goto error; + } + + def->data.pci.bus = -1; + if (bus && + virStrToLong_i(bus, NULL, 16, &def->data.pci.bus) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'bus' attribute")); + goto error; + } + + def->data.pci.slot = -1; + if (slot && + virStrToLong_i(slot, NULL, 16, &def->data.pci.slot) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'slot' attribute")); + goto error; + } + + def->data.pci.function = -1; + if (function && + virStrToLong_i(function, NULL, 16, &def->data.pci.function) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'function' attribute")); + goto error; + } + + if (def->data.pci.domain == -1 || def->data.pci.bus == -1 + || def->data.pci.slot == -1) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Unsufficient specification for PCI address")); + goto error; + } + + break; + default: + /* Should not happen */ + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unknown device address type")); + goto error; + } + +cleanup: + VIR_FREE(domain); + VIR_FREE(bus); + VIR_FREE(slot); + VIR_FREE(function); + + return def; + +error: + virDomainDeviceAddressFree(def); + def = NULL; + goto cleanup; +} + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition @@ -725,9 +897,10 @@ 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; + char *controller_name = NULL; char *bus = NULL; char *unit = NULL; char *cachetag = NULL; @@ -782,8 +955,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, 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"); + controller_name = virXMLPropString(cur, "name"); + controller_pci_addr = virXMLPropString(cur, "pci_addr"); + bus_id = virXMLPropString(cur, "bus"); unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { @@ -886,7 +1060,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, } if (controller) { - def->controller_id = controller_id; + def->controller_name = controller_name; def->bus_id = -1; if (bus_id && virStrToLong_i(bus_id, NULL, 10, &def->bus_id) < 0) { @@ -901,8 +1075,26 @@ 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; + } } + VIR_DEBUG("Controller PCI addr for disk parsed as %x:%x:%x\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, @@ -969,6 +1161,59 @@ 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; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(conn); + return NULL; + } + + def->address = 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->name = virXMLPropString(node, "name"); + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE && + def->address == NULL && + xmlStrEqual(cur->name, BAD_CAST "address")) { + + def->address = virDomainDeviceAddressParseXML(conn, cur); + } + + cur = cur->next; + } + + +cleanup: + VIR_FREE(type); + + 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 */ @@ -2616,6 +2861,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))) @@ -3033,6 +3283,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/conf/domain_conf.h b/src/conf/domain_conf.h index c034ae4..591143f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -63,6 +63,30 @@ enum virDomainVirtType { VIR_DOMAIN_VIRT_LAST, }; +enum virDomainDeviceAddressType { + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, + + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST +}; + +typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress; +struct _virDomainDevicePCIAddress { + int domain; + int bus; + int slot; + int function; +}; + +typedef struct _virDomainDeviceAddress virDomainDeviceAddress; +typedef virDomainDeviceAddress *virDomainDeviceAddressPtr; +struct _virDomainDeviceAddress { + int type; + union { + virDomainDevicePCIAddress pci; + } data; +}; + + /* Two types of disk backends */ enum virDomainDiskType { VIR_DOMAIN_DISK_TYPE_BLOCK, @@ -112,7 +136,12 @@ struct _virDomainDiskDef { int unit_id; /* Unit on the controller */ char *src; char *dst; - char *controller_id; + char *controller_name; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } controller_pci_addr; char *driverName; char *driverType; char *serial; @@ -127,6 +156,15 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; }; +/* Stores the virtual disk controller configuration */ +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; +struct _virDomainControllerDef { + int type; + char *name; + virDomainDeviceAddressPtr address; +}; + static inline int virDiskHasValidPciAddr(virDomainDiskDefPtr def) { @@ -483,6 +521,7 @@ enum virDomainDeviceType { VIR_DOMAIN_DEVICE_VIDEO, VIR_DOMAIN_DEVICE_HOSTDEV, VIR_DOMAIN_DEVICE_WATCHDOG, + VIR_DOMAIN_DEVICE_CONTROLLER, VIR_DOMAIN_DEVICE_LAST, }; @@ -493,6 +532,7 @@ struct _virDomainDeviceDef { int type; union { virDomainDiskDefPtr disk; + virDomainControllerDefPtr controller; virDomainFSDefPtr fs; virDomainNetDefPtr net; virDomainInputDefPtr input; @@ -605,6 +645,9 @@ struct _virDomainDef { int ndisks; virDomainDiskDefPtr *disks; + int ncontrollers; + virDomainControllerDefPtr *controllers; + int nfss; virDomainFSDefPtr *fss; @@ -691,6 +734,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); @@ -699,6 +743,7 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); +void virDomainDeviceAddressFree(virDomainDeviceAddressPtr def); void virDomainDefFree(virDomainDefPtr vm); void virDomainObjFree(virDomainObjPtr vm); @@ -832,6 +877,7 @@ VIR_ENUM_DECL(virDomainBoot) VIR_ENUM_DECL(virDomainFeature) VIR_ENUM_DECL(virDomainLifecycle) VIR_ENUM_DECL(virDomainDevice) +VIR_ENUM_DECL(virDomainDeviceAddress) VIR_ENUM_DECL(virDomainDisk) VIR_ENUM_DECL(virDomainDiskDevice) VIR_ENUM_DECL(virDomainDiskBus) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb81ace..92cf86c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -68,7 +68,6 @@ virCgroupGetCpuacctUsage; virCgroupGetFreezerState; virCgroupSetFreezerState; - # datatypes.h virGetDomain; virGetInterface; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f022f89..af7b59c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4356,7 +4356,6 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, return ret; } - static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 35cd330..f6f3e58 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1481,6 +1481,53 @@ qemuMonitorParsePciAddReply(virDomainObjPtr vm, return 0; } +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; +} int qemuMonitorAddPCIHostDevice(const virDomainObjPtr vm, unsigned hostDomain ATTRIBUTE_UNUSED, -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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/conf/domain_conf.c | 33 ++++++- src/conf/domain_conf.h | 5 +- src/libvirt_private.syms | 3 +- src/qemu/qemu_driver.c | 243 +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 278 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4e2b84e..91115b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -896,7 +896,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; @@ -953,7 +952,8 @@ virDomainDiskDefParseXML(virConnectPtr conn, if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); - } else if ((controller == NULL) && + } else if ((controller_name == NULL && + controller_pci_addr == NULL) && (xmlStrEqual(cur->name, BAD_CAST "controller"))) { controller_name = virXMLPropString(cur, "name"); controller_pci_addr = virXMLPropString(cur, "pci_addr"); @@ -1059,7 +1059,13 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } - if (controller) { + if (controller_name && controller_pci_addr) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Both controller ID and address specified!")); + goto error; + } + + if (controller_name) { def->controller_name = controller_name; def->bus_id = -1; @@ -1090,6 +1096,23 @@ 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 %x:%x:%x\n", def->controller_pci_addr.domain, def->controller_pci_addr.bus, @@ -1101,6 +1124,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, _("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, @@ -1126,6 +1150,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/conf/domain_conf.h b/src/conf/domain_conf.h index 591143f..b30d08c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -133,7 +133,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_name; @@ -744,6 +744,9 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); void virDomainDeviceAddressFree(virDomainDeviceAddressPtr def); +int virDomainCompareDeviceAddresses(virDomainDeviceAddressPtr a, + virDomainDeviceAddressPtr b); +const char *virDomainFormatDeviceAddress(virDomainDeviceAddressPtr address); void virDomainDefFree(virDomainDefPtr vm); void virDomainObjFree(virDomainObjPtr vm); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 92cf86c..e73ed94 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -152,7 +152,8 @@ virDomainObjListGetActiveIDs; virDomainObjListNumOfDomains; virDomainObjListInit; virDomainObjListDeinit; - +virDomainCompareDeviceAddresses; +virDomainFormatDeviceAddress; # domain_event.h virDomainEventCallbackListAdd; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af7b59c..9f71562 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4356,6 +4356,247 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, return ret; } +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; + } + + controller_specified = 0; + /* Partially specified PCI addresses (should not happen, anyway) + don't count as specification */ + if (dev->data.disk->controller_name != 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); + return ret; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s disk"), type); + VIR_FREE(cmd); + return -1; + } + + 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; + } + } + + 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; + unsigned domain, bus, slot; + + /* Only SCSI controllers are supported at the moment */ + if (dev->data.controller->address && + dev->data.controller->address->type != + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Only SCSI controller hotplug is supported!")); + return -1; + } + + /* 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->name && + STREQ(vm->def->controllers[i]->name, + dev->data.controller->name)) || + virDomainCompareDeviceAddresses(vm->def->controllers[i]->address, + dev->data.controller->address)) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller (id:%s, address: %s) already exists"), + dev->data.controller->name ? + dev->data.controller->name : "(none)", + virDomainFormatDeviceAddress(dev->data.controller->address)); + return -1; + } + } + + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers+1) < 0) { + virReportOOMError(conn); + return -1; + } + +try_command: + if (dev->data.controller->address != NULL) { + domain = dev->data.controller->address->data.pci.domain; + bus = dev->data.controller->address->data.pci.bus; + slot = dev->data.controller->address->data.pci.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); + } + + 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->address->data.pci.domain = domain; + dev->data.controller->address->data.pci.bus = bus; + dev->data.controller->address->data.pci.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; +} + static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) @@ -4720,7 +4961,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

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> This function is not necessary anymore Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu/qemu_driver.c | 33 --------------------------------- 1 files changed, 0 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f71562..1fbfdde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4597,39 +4597,6 @@ try_command: return 0; } -static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) -{ - int i; - const char* 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; - } - - if (qemuMonitorAddPCIDisk(vm, - dev->data.disk->src, - type, - &dev->data.disk->pci_addr.domain, - &dev->data.disk->pci_addr.bus, - &dev->data.disk->pci_addr.slot) < 0) - return -1; - - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); - - return 0; -} - static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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/conf/domain_conf.c | 5 +---- src/qemu/qemu_driver.c | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 91115b6..e90975f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -880,7 +880,6 @@ error: goto cleanup; } - /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2940,7 +2939,6 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, } #endif - int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { @@ -3326,8 +3324,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, def->controllers[def->ncontrollers++] = controller; } - /* qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers), - virDomainControllerQSort); */ + VIR_FREE(nodes); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1fbfdde..1b20937 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4413,6 +4413,15 @@ try_command: bus = dev->data.disk->controller_pci_addr.bus; slot = dev->data.disk->controller_pci_addr.slot; + if (dev->data.disk->controller_name) { + /* 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); } @@ -4568,6 +4577,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); @@ -4582,18 +4593,14 @@ 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->address->data.pci.domain = domain; dev->data.controller->address->data.pci.bus = bus; dev->data.controller->address->data.pci.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; } @@ -4947,6 +4954,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

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> ... 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/qemu_driver.c | 37 +++++++++++++++++++++++++++++-------- 1 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b20937..d3395a7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4407,19 +4407,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_name) { - /* 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_name, + vm->def->controllers[i]->name)) { + 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]->address->data.pci.domain; + bus = vm->def->controllers[i]->address->data.pci.bus; + slot = vm->def->controllers[i]->address->data.pci.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]->address->data.pci.domain && + bus == vm->def->controllers[i]->address->data.pci.bus && + slot == vm->def->controllers[i]->address->data.pci.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) { -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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/qemu_driver.c | 122 +++++++++++++++++++++++++++--------------------- 1 files changed, 68 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3395a7..4f647c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4406,68 +4406,82 @@ try_command: controller_specified = 1; } - if (controller_specified) { - if (dev->data.disk->controller_name) { - for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (STREQ(dev->data.disk->controller_name, - vm->def->controllers[i]->name)) { - 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]->address->data.pci.domain; - bus = vm->def->controllers[i]->address->data.pci.bus; - slot = vm->def->controllers[i]->address->data.pci.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]->address->data.pci.domain && - bus == vm->def->controllers[i]->address->data.pci.bus && - slot == vm->def->controllers[i]->address->data.pci.slot) - 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]->address->data.pci.domain; + bus = vm->def->controllers[i]->address->data.pci.bus; + slot = vm->def->controllers[i]->address->data.pci.slot; + } + + if (controller_specified && dev->data.disk->controller_name) { + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (STREQ(dev->data.disk->controller_name, + vm->def->controllers[i]->name)) { + 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]->address->data.pci.domain; + bus = vm->def->controllers[i]->address->data.pci.bus; + slot = vm->def->controllers[i]->address->data.pci.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]->address->data.pci.domain && + bus == vm->def->controllers[i]->address->data.pci.bus && + slot == vm->def->controllers[i]->address->data.pci.slot) + break; } - 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); + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + } + /* 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 ? 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 ? bus_unit_string : ""); + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> 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/conf/domain_conf.c | 26 ++---- src/conf/domain_conf.h | 8 ++ src/qemu/qemu_driver.c | 242 +++++++++++++++++++++++++++--------------------- 3 files changed, 153 insertions(+), 123 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e90975f..48015a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1077,37 +1077,20 @@ virDomainDiskDefParseXML(virConnectPtr conn, 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; - } - - /* 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); + _("Cannot parse <controller> 'unit' attribute")); goto error; } } /* 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'"), + _("Unable to parse <controller> 'pci_addr' parameter '%s'"), controller_pci_addr); goto error; } @@ -1215,6 +1198,11 @@ virDomainControllerDefParseXML(virConnectPtr conn, } def->name = virXMLPropString(node, "name"); + if (!def->name) { + virDomainReportError(conn, VIR_ERR_INVALID_ARG, + _("cannot use controller without name")); + goto error; + } cur = node->children; while (cur != NULL) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b30d08c..e058c56 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -171,6 +171,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) return def->pci_addr.domain || def->pci_addr.bus || def->pci_addr.slot; } +static inline int +virDiskHasValidController(virDomainDiskDefPtr def) +{ + return def->controller_name != 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/qemu_driver.c b/src/qemu/qemu_driver.c index 4f647c4..9f44685 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4356,44 +4356,20 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, return ret; } -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_name != NULL || @@ -4411,16 +4387,17 @@ 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]->address->data.pci.domain; bus = vm->def->controllers[i]->address->data.pci.bus; slot = vm->def->controllers[i]->address->data.pci.slot; @@ -4428,21 +4405,20 @@ try_command: if (controller_specified && dev->data.disk->controller_name) { for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if (STREQ(dev->data.disk->controller_name, + if (vm->def->controllers[i]->name && + STREQ(dev->data.disk->controller_name, vm->def->controllers[i]->name)) { + conPtr = vm->def->controllers[i]; + domain = vm->def->controllers[i]->address->data.pci.domain; + bus = vm->def->controllers[i]->address->data.pci.bus; + slot = vm->def->controllers[i]->address->data.pci.slot; 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]->address->data.pci.domain; - bus = vm->def->controllers[i]->address->data.pci.bus; - slot = vm->def->controllers[i]->address->data.pci.slot; + if (!conPtr) { + *ret = CONTROLLER_INEXISTENT; + } } else if (controller_specified) { domain = dev->data.disk->controller_pci_addr.domain; bus = dev->data.disk->controller_pci_addr.bus; @@ -4452,35 +4428,109 @@ try_command: if (domain == vm->def->controllers[i]->address->data.pci.domain && bus == vm->def->controllers[i]->address->data.pci.bus && slot == vm->def->controllers[i]->address->data.pci.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; - } - } - /* 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) { + 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 controller, regardless if an explicit + controller has been specified or not. */ + domain = conPtr->address->data.pci.domain; + bus = conPtr->address->data.pci.bus; + slot = conPtr->address->data.pci.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); + if (dev->data.disk->unit_id != -1) { + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); } - bus_unit_string = virBufferContentAndReset(&buf); + 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 ? 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 ? bus_unit_string : ""); + VIR_DEBUG ("%s: drive_add %.2x:%.2x:%.2x file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, + bus_unit_string ? bus_unit_string : ""); + ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, + bus_unit_string ? bus_unit_string : ""); VIR_FREE(safe_path); if (ret == -1) { @@ -4497,39 +4547,24 @@ 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 (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->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 (dev->data.disk->unit_id == -1) { + dev->data.disk->unit_id = unit_id; } dev->data.disk->pci_addr.domain = domain; @@ -4538,7 +4573,6 @@ try_command: virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); - return 0; } @@ -4590,7 +4624,7 @@ try_command: bus = dev->data.controller->address->data.pci.bus; slot = dev->data.controller->address->data.pci.slot; - ret = virAsprintf(&tmp, "%d:%d:%d", domain, bus, slot); + ret = virAsprintf(&tmp, "%.2x:%.2x:%.2x", domain, bus, slot); ret |= virAsprintf(&cmd, "pci_add %s%s storage if=%s", (tryOldSyntax ? "" : "pci_addr="), tmp, type); } else { -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> This separates the communication with qemu a bit from the more libvirtish actions. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu/qemu_driver.c | 61 +-------------------------------------- src/qemu/qemu_monitor_text.c | 65 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 2 + 3 files changed, 69 insertions(+), 59 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f44685..289c3c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4580,12 +4580,7 @@ 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; - unsigned domain, bus, slot; + int i; /* Only SCSI controllers are supported at the moment */ if (dev->data.controller->address && @@ -4618,59 +4613,7 @@ static int qemudDomainAttachDiskController(virConnectPtr conn, return -1; } -try_command: - if (dev->data.controller->address != NULL) { - domain = dev->data.controller->address->data.pci.domain; - bus = dev->data.controller->address->data.pci.bus; - slot = dev->data.controller->address->data.pci.slot; - - ret = virAsprintf(&tmp, "%.2x:%.2x:%.2x", 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); - } - - 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); - - /* 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); - 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 */ - dev->data.controller->address->data.pci.domain = domain; - dev->data.controller->address->data.pci.bus = bus; - dev->data.controller->address->data.pci.slot = slot; - - VIR_FREE(reply); - - vm->def->controllers[vm->def->ncontrollers++] = dev->data.controller; - return 0; + return qemuMonitorAttachDiskController(vm, dev); } static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index f6f3e58..7326d05 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1529,6 +1529,71 @@ qemudParseDriveAddReply(virDomainObjPtr vm, return 0; } +int qemuMonitorAttachDiskController(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret; + char *cmd, *reply; + char *tmp; + const char* type = virDomainDiskBusTypeToString(dev->data.controller->type); + int tryOldSyntax = 0; + unsigned domain, bus, slot; + +try_command: + if (dev->data.controller->address != NULL) { + domain = dev->data.controller->address->data.pci.domain; + bus = dev->data.controller->address->data.pci.bus; + slot = dev->data.controller->address->data.pci.slot; + + ret = virAsprintf(&tmp, "%.2x:%.2x:%.2x", 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); + } + + if (ret == -1) { + virReportOOMError(NULL); + return ret; + } + + if (qemuMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s controller"), type); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(cmd); + + /* Naturally, the controller hotplug reply is identical with + any other PCI hotplug reply */ + if (qemuMonitorParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + + qemudReportError (NULL, NULL, 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 */ + dev->data.controller->address->data.pci.domain = domain; + dev->data.controller->address->data.pci.bus = bus; + dev->data.controller->address->data.pci.slot = slot; + + VIR_FREE(reply); + + vm->def->controllers[vm->def->ncontrollers++] = dev->data.controller; + return 0; +} + int qemuMonitorAddPCIHostDevice(const virDomainObjPtr vm, unsigned hostDomain ATTRIBUTE_UNUSED, unsigned hostBus, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 9175456..c674507 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -154,6 +154,8 @@ int qemuMonitorAddPCINetwork(const virDomainObjPtr vm, unsigned *guestBus, unsigned *guestSlot); +int qemuMonitorAttachDiskController(virDomainObjPtr vm, + virDomainDeviceDefPtr dev); int qemuMonitorRemovePCIDevice(const virDomainObjPtr vm, unsigned guestDomain, unsigned guestBus, -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> This separates the communication with qemu a bit from the more libvirtish actions. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- src/qemu/qemu_driver.c | 82 ++-------------------------------------- src/qemu/qemu_monitor_text.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 5 ++ 3 files changed, 95 insertions(+), 77 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 289c3c6..df22035 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4445,15 +4445,8 @@ static int qemudDomainAttachDiskDevice(virConnectPtr conn, 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; + const char* type = NULL; /* Both controller and disk can specify a type like SCSI. While a virtual disk as such is not bound to a specific bus type, @@ -4485,13 +4478,6 @@ static int qemudDomainAttachDiskDevice(virConnectPtr 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) { @@ -4510,70 +4496,12 @@ try_command: /* At this point, we have a valid controller, regardless if an explicit controller has been specified or not. */ - domain = conPtr->address->data.pci.domain; - bus = conPtr->address->data.pci.bus; - slot = conPtr->address->data.pci.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 %.2x:%.2x:%.2x file=%s,if=%s%s", vm->def->name, - domain, bus, slot, safe_path, type, - bus_unit_string ? bus_unit_string : ""); - ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x file=%s,if=%s%s", - (tryOldSyntax ? "" : "pci_addr="), domain, bus, - slot, safe_path, type, - bus_unit_string ? bus_unit_string : ""); - - VIR_FREE(safe_path); - if (ret == -1) { - virReportOOMError(conn); - return ret; - } + ret = qemuMonitorAttachDiskDevice(vm, dev, conPtr, type); - if (qemudMonitorCommand(vm, cmd, &reply) < 0) { - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("cannot attach %s disk"), type); - VIR_FREE(cmd); - return -1; - } - - VIR_FREE(cmd); - - 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; - - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + if (!ret) + virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); - return 0; + return ret; } static int qemudDomainAttachDiskController(virConnectPtr conn, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7326d05..371388f 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1594,6 +1594,91 @@ try_command: return 0; } +int qemuMonitorAttachDiskDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + virDomainControllerDefPtr conPtr, + const char *type) +{ + int ret; + char *cmd, *reply; + char *safe_path; + int tryOldSyntax = 0; + int bus_id, unit_id; + int domain, bus, slot; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char* bus_unit_string; + + domain = conPtr->address->data.pci.domain; + bus = conPtr->address->data.pci.bus; + slot = conPtr->address->data.pci.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); + + safe_path = qemuMonitorEscapeArg(dev->data.disk->src); + if (!safe_path) { + virReportOOMError(NULL); + return -1; + } + +try_command: + VIR_DEBUG ("%s: drive_add %.2x:%.2x:%.2x file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, + bus_unit_string ? bus_unit_string : ""); + ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, + bus_unit_string ? bus_unit_string : ""); + + VIR_FREE(safe_path); + if (ret == -1) { + virReportOOMError(NULL); + return ret; + } + + if (qemuMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s disk"), type); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(cmd); + + 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 (NULL, NULL, 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; + + return 0; +} + int qemuMonitorAddPCIHostDevice(const virDomainObjPtr vm, unsigned hostDomain ATTRIBUTE_UNUSED, unsigned hostBus, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index c674507..6a2d898 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -156,6 +156,11 @@ int qemuMonitorAddPCINetwork(const virDomainObjPtr vm, int qemuMonitorAttachDiskController(virDomainObjPtr vm, virDomainDeviceDefPtr dev); +int qemuMonitorAttachDiskDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + virDomainControllerDefPtr conPtr, + const char *type); + int qemuMonitorRemovePCIDevice(const virDomainObjPtr vm, unsigned guestDomain, unsigned guestBus, -- 1.6.4

From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> Document the controller specification, that is, the <controller> element within <domain> --- docs/formatdomain.html.in | 38 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 52889af..aada7d9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -345,6 +345,7 @@ <disk type='file'> <driver name="tap" type="aio"> <source file='/var/lib/xen/images/fv0'/> + <controller name='controller0' bus='0' unit='5'> <target dev='hda' bus='ide'/> <encryption type='...'> ... @@ -366,6 +367,13 @@ specifies the fully-qualified path to the file holding the disk. If the disk <code>type</code> is "block", then the <code>dev</code> attribute specifies the path to the host device to serve as the disk. <span class="since">Since 0.0.3</span></dd> + <dt><code>controller</code></dt> + <dd>If present, specifies the controller to which the disk is + connected. This is useful for disk hotplug as it allows to add + disks to specific controlers. This is currently only possible + for scsi drivers. <code>bus</code> and <code>unit</unit> are + facultative and specify the drive position on the controller. + </dd> <dt><code>target</code></dt> <dd>The <code>target</code> element controls the bus / device under which the disk is exposed to the guest OS. The <code>dev</code> attribute indicates @@ -675,6 +683,36 @@ qemu-kvm -net nic,model=? /dev/null It takes values "xen" (paravirtualized), "ps2" and "usb".</dd> </dl> + <h4><a name="elementsController">Disk controllers</a></h4> + + <p> + Disk controllers are hosts for hard disks. + </p> + + <pre> + ... + <controller type='scsi' name='controller0'> + <address type='pci' bus='0x00' domain='0x00' slot='0x0a'/> + </controller> + ...</pre> + + <dl> + <dt><code>controller</code></dt> + <dd>The <code>controller</code> element has a mandatory <code>type</code> + attribute which takes the type of the controller, + e.g., <code>"scsi"</code>. <br/> + <code>name</code> specifies a symbolic identifier for the + controller by which it can be addressed. + + The address of the controller on the underlying bus is given + by the <code>address</code> subelement. The mandatory + element <code>type</code> specified the bus type for which + the address holds and must be the same as for the controller. + Currently, only pci is supported, which requires three + parameters to denote address: <code>bus</code>, <code>domain</code>, + and <code>device</code>. All must be given as hexadecimal numbers. + </dd> + </dl> <h4><a name="elementsGraphics">Graphical framebuffers</a></h4> -- 1.6.4

On Tue, Nov 17, 2009 at 12:53:31AM +0100, wolfgang.mauerer@siemens.com wrote:
Hi,
this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold:
1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks.
This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission.
What libvirt version / GIT changeset did you create these patches against ? The current libvirt QEMU driver code in GIT is quite different, so the patches here don't apply for me as is. 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 :|

Hi, Daniel P. Berrange wrote:
On Tue, Nov 17, 2009 at 12:53:31AM +0100, wolfgang.mauerer@siemens.com wrote:
Hi,
this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold:
1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks.
This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission.
What libvirt version / GIT changeset did you create these patches against ? The current libvirt QEMU driver code in GIT is quite different, so the patches here don't apply for me as is.
sorry for the late reply, I could not access my eMail during the last couple of days. Patches are on top of 790f0b3057787bb64, I did not realise that this one was only in the middle of qemu refactoring, not at the end :-( Do you plan any more refactorings to the qemu base in the near future, and if yes, are these already available somewhere? I'd like to avoid another useless rebase... Thanks, Wolfgang

On Mon, Nov 23, 2009 at 02:15:06PM +0100, Wolfgang Mauerer wrote:
Hi,
Daniel P. Berrange wrote:
On Tue, Nov 17, 2009 at 12:53:31AM +0100, wolfgang.mauerer@siemens.com wrote:
Hi,
this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold:
1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks.
This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission.
What libvirt version / GIT changeset did you create these patches against ? The current libvirt QEMU driver code in GIT is quite different, so the patches here don't apply for me as is.
sorry for the late reply, I could not access my eMail during the last couple of days. Patches are on top of 790f0b3057787bb64, I did not realise that this one was only in the middle of qemu refactoring, not at the end :-(
Do you plan any more refactorings to the qemu base in the near future, and if yes, are these already available somewhere? I'd like to avoid another useless rebase...
No, the monitor code for the QEMU driver is stable now. I'll only be adding extra functionality, not changing existing stuff, so it should be good to rebase against now. 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 :|

Hi, Daniel P. Berrange wrote:
On Mon, Nov 23, 2009 at 02:15:06PM +0100, Wolfgang Mauerer wrote:
On Tue, Nov 17, 2009 at 12:53:31AM +0100, wolfgang.mauerer@siemens.com wrote:
this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold:
1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks.
This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission. What libvirt version / GIT changeset did you create these patches against ? The current libvirt QEMU driver code in GIT is quite different, so the patches here don't apply for me as is. sorry for the late reply, I could not access my eMail during
Daniel P. Berrange wrote: the last couple of days. Patches are on top of 790f0b3057787bb64, I did not realise that this one was only in the middle of qemu refactoring, not at the end :-(
Do you plan any more refactorings to the qemu base in the near future, and if yes, are these already available somewhere? I'd like to avoid another useless rebase...
No, the monitor code for the QEMU driver is stable now. I'll only be adding extra functionality, not changing existing stuff, so it should be good to rebase against now.
okay, to avoid flooding the mailing list with nearly identical patches, I've put the rebased versions into a repository at git://git.kiszka.org/libvirt.git queue Best regards, Wolfgang ---------------------------------------------------- The following changes since commit 8f147d16f1ccdd1f2ce063823fcf961e7bf396a5: Daniel P. Berrange (1): Fix default disk type when parsing QEMU argv are available in the git repository at: git://git.kiszka.org/libvirt.git queue Wolfgang Mauerer (13): Fix help message Clarify documentation for private symbols Extend <disk> element with controller information Add new domain device: "controller" Add disk-based hotplugging for the qemu backend Drop qemudAttachPciDiskDevice Implement controller hotplugging Allow controller selection by ID Remove surprises in the semantics of disk-hotadd Factor out the method to get the PCI address of a controller for a given disk Extract qemu monitor parts for controller hotplug Extract monitor parts from the qemu interaction for disk hotplug Update documentation: Controller daemon/libvirtd.c | 2 +- docs/formatdomain.html.in | 38 +++++ docs/schemas/domain.rng | 160 ++++++++++++++++------ src/conf/domain_conf.c | 314 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 62 ++++++++- src/libvirt_private.syms | 7 +- src/qemu/qemu_driver.c | 194 ++++++++++++++++++++++++-- src/qemu/qemu_monitor_text.c | 199 ++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 9 ++ 9 files changed, 925 insertions(+), 60 deletions(-)

On Tue, Nov 24, 2009 at 02:15:58PM +0100, Wolfgang Mauerer wrote:
Hi,
Daniel P. Berrange wrote:
On Mon, Nov 23, 2009 at 02:15:06PM +0100, Wolfgang Mauerer wrote:
On Tue, Nov 17, 2009 at 12:53:31AM +0100, wolfgang.mauerer@siemens.com wrote:
this is the second revision of a patch series to improve disk hotadd support for libvirt. It focuses on the qemu backend, but is naturally designed to be compatible with other backends as well. The objective is two-fold:
1.) Split off controller from disk handling. This is done by introducing a new domain element <controller> that is used to describe disk controllers. Support for hotplugging such controllers is added. Support to reference controllers by name is also included. 2.) <disk>s can now be associated with a specific controller; this is done by means of a <controller> subelemnt for disks.
This patch addresses the questions that were raised during the review of the initial submission, massages the code by fixing some whitespace issues, gets static controller configurations to work, and adds documentation. Notice that in contrast to the first submission I did not include the patch that adds support for disk- and controller hot_remove_. Since the qemu codebase is still in bit of a flux wrt. the necessary patches required for this functionality, it will reappear some time later as a separate submission. What libvirt version / GIT changeset did you create these patches against ? The current libvirt QEMU driver code in GIT is quite different, so the patches here don't apply for me as is. sorry for the late reply, I could not access my eMail during
Daniel P. Berrange wrote: the last couple of days. Patches are on top of 790f0b3057787bb64, I did not realise that this one was only in the middle of qemu refactoring, not at the end :-(
Do you plan any more refactorings to the qemu base in the near future, and if yes, are these already available somewhere? I'd like to avoid another useless rebase...
No, the monitor code for the QEMU driver is stable now. I'll only be adding extra functionality, not changing existing stuff, so it should be good to rebase against now.
okay, to avoid flooding the mailing list with nearly identical patches, I've put the rebased versions into a repository at git://git.kiszka.org/libvirt.git queue
I've been taking a closer look at this, and I think the final state of patch series as a whole looks good. The minor issue is that the intermediate patches don't compile, since they rely on compile fixes at the end of the series. Rather than ask you to re-format the patches yet again, I'm going to just merge the patches into a smaller set myself. I've got just one question I'd like to understand before doing this. On the <disk> element's new <controller> element, you are allowing two mutually exclusive ways of specifying the controller <disk> ... <controller name="<identifier>" bus="<number>" unit="<number>"/> </disk> and <disk> ... <controller pci_addr="<addr>" bus="<number>" unit="<number>"/> </disk> I think it is going to cause unneccessary pain for applications to have to cope with two different ways of specifying the same relationship. So my intention is to remove the 'pci_addr' variant, since that information can easily be got directly from the separate top level <controller> device itself On a side note, once we've applied this initial series, I think we'll also need to be automatically adding a <controller> element in all guest domains which have IDE or SCSI controllers present by default. This is going to also force us to hook into QEMU's 'info pci' monitor command to figure out their PCI address. This is long overdue though and needed for NIC & Disk devices added at startup too, so this is good motivation :-) 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 :|
participants (3)
-
Daniel P. Berrange
-
Wolfgang Mauerer
-
wolfgang.mauerer@siemens.com