On Mon, 2009-07-20 at 16:38 +0200, Daniel Veillard wrote:
On Mon, Jul 20, 2009 at 03:05:31PM +0100, Mark McLoughlin wrote:
On Mon, 2009-07-20 at 14:42 +0100, Daniel P. Berrange wrote:
On Mon, Jul 20, 2009 at 12:51:12PM +0100, Mark McLoughlin wrote:
When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts.
Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future.
* src/domain_conf.h: replace slotnum with pci_addr
diff --git a/src/domain_conf.h b/src/domain_conf.h index 6e111fa..1766b61 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -106,7 +106,7 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + char *pci_addr; /* for detach */ };
I think it'd be nicer to store the parsed address here as a nested struct with domain, bus, slot.
I understand dan 'here' as in the C struct not in the XML
It is not really saving us trouble by using a string, since most of the places using this field end up asprintf'ing it into another string, or even having to extract pieces out of it again.
It's saving us trouble because you don't have to code the equivalent of virDomainHostdevSubsysPciDefParseXML() and have e.g.
<state> <address domain="0" bus="0" slot="5"/> </state>
I just now started to do it and then realized how much extra hassle the XML parsing was going to be. All for some internal data that we use in textual format anyway. Are you sure? :-)
Well a single string in the XML is fine, but in the parsed Def let's keep the bits as fully parsed, i.e. the set of ints we extract in patch 12/14 Agreed with Dan, Agreed that separating them in the XML will make the code way more complex especially for error handling.
Okay, here's how that looks. Personally, I think even this much adds complexity (parsing the devaddr param from the XML, re-formatting it in several places, not being able to just check pci_addr != NULL) without much benefit. Cheers, Mark. From: Mark McLoughlin <markmc@redhat.com> Subject: [PATCH 02/14] Retain disk PCI address across libvirtd restarts When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts. Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future. * src/domain_conf.h: replace slotnum with pci_addr struct, add helper for testing whether the address is valid * src/domain_conf.c: handle formatting and parsing the address * src/qemu_driver.c: store the parsed slot number as a full PCI address, and use this address with the pci_del monitor command * src/vbox/vbox_tmpl.c: we're debug printing slotnum here even though it can never be set, just delete it --- src/domain_conf.c | 33 ++++++++++++++++++++++++++++++--- src/domain_conf.h | 12 +++++++++++- src/qemu_driver.c | 36 ++++++++++++++++++++++++------------ src/vbox/vbox_tmpl.c | 1 - 4 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 10e6ac6..91a6c6e 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -643,7 +643,7 @@ int virDomainDiskCompare(virDomainDiskDefPtr a, static virDomainDiskDefPtr virDomainDiskDefParseXML(virConnectPtr conn, xmlNodePtr node, - int flags ATTRIBUTE_UNUSED) { + int flags) { virDomainDiskDefPtr def; xmlNodePtr cur; char *type = NULL; @@ -654,6 +654,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *target = NULL; char *bus = NULL; char *cachetag = NULL; + char *devaddr = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -708,6 +709,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "state")) { + devaddr = virXMLPropString(cur, "devaddr"); } } cur = cur->next; @@ -807,6 +811,17 @@ virDomainDiskDefParseXML(virConnectPtr conn, goto error; } + if (devaddr && + sscanf(devaddr, "%x:%x:%x", + &def->pci_addr.domain, + &def->pci_addr.bus, + &def->pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse devaddr parameter '%s'"), + devaddr); + goto error; + } + def->src = source; source = NULL; def->dst = target; @@ -825,6 +840,7 @@ cleanup: VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); + VIR_FREE(devaddr); return def; @@ -3388,7 +3404,8 @@ virDomainLifecycleDefFormat(virConnectPtr conn, static int virDomainDiskDefFormat(virConnectPtr conn, virBufferPtr buf, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + int flags) { const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); @@ -3446,6 +3463,16 @@ virDomainDiskDefFormat(virConnectPtr conn, if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virBufferAddLit(buf, " <state"); + if (virDiskHasValidPciAddr(def)) + virBufferVSprintf(buf, " devaddr='%.4x:%.2x:%.2x'", + def->pci_addr.domain, + def->pci_addr.bus, + def->pci_addr.slot); + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " </disk>\n"); return 0; @@ -4048,7 +4075,7 @@ char *virDomainDefFormat(virConnectPtr conn, def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(conn, &buf, def->disks[n]) < 0) + if (virDomainDiskDefFormat(conn, &buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) diff --git a/src/domain_conf.h b/src/domain_conf.h index 69b665f..c0fdd65 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,9 +111,19 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; }; +static inline int +virDiskHasValidPciAddr(virDomainDiskDefPtr def) +{ + return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; +} + /* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 00dc6e5..a87ef70 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4390,6 +4390,8 @@ try_command: return -1; } + VIR_FREE(cmd); + DEBUG ("%s: pci_add reply: %s", vm->def->name, reply); /* If the command succeeds qemu prints: * OK bus 0, slot XXX... @@ -4399,22 +4401,25 @@ try_command: if ((s = strstr(reply, "OK ")) && (s = strstr(s, "slot "))) { char *dummy = s; + unsigned slot; + s += strlen("slot "); - if (virStrToLong_i ((const char*)s, &dummy, 10, &dev->data.disk->slotnum) == -1) + if (virStrToLong_ui((const char*)s, &dummy, 10, &slot) == -1) VIR_WARN("%s", _("Unable to parse slot number\n")); + /* XXX not neccessarily always going to end up in domain 0 / bus 0 :-( */ - /* XXX this slotnum is not persistant across restarts :-( */ + dev->data.disk->pci_addr.domain = 0; + dev->data.disk->pci_addr.bus = 0; + dev->data.disk->pci_addr.slot = slot; } else if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); - VIR_FREE(cmd); tryOldSyntax = 1; goto try_command; } else { qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("adding %s disk failed: %s"), type, reply); VIR_FREE(reply); - VIR_FREE(cmd); return -1; } @@ -4423,7 +4428,7 @@ try_command: virDomainDiskQSort); VIR_FREE(reply); - VIR_FREE(cmd); + return 0; } @@ -4660,21 +4665,24 @@ static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, goto cleanup; } - if (detach->slotnum < 1) { + if (!virDiskHasValidPciAddr(detach)) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - invalid slot number %d"), - detach->dst, detach->slotnum); + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); goto cleanup; } try_command: if (tryOldSyntax) { - if (virAsprintf(&cmd, "pci_del 0 %d", detach->slotnum) < 0) { + if (virAsprintf(&cmd, "pci_del 0 %.2x", detach->pci_addr.slot) < 0) { virReportOOMError(conn); goto cleanup; } } else { - if (virAsprintf(&cmd, "pci_del pci_addr=0:0:%d", detach->slotnum) < 0) { + if (virAsprintf(&cmd, "pci_del pci_addr=%.4x:%.2x:%.2x", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot) < 0) { virReportOOMError(conn); goto cleanup; } @@ -4698,8 +4706,12 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to detach disk %s: invalid slot %d: %s"), - detach->dst, detach->slotnum, reply); + _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->dst, + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); goto cleanup; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3208f03..a2b958a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2712,7 +2712,6 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { DEBUG("disk(%d) cachemode: %d", i, def->disks[i]->cachemode); DEBUG("disk(%d) readonly: %s", i, def->disks[i]->readonly ? "True" : "False"); DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared ? "True" : "False"); - DEBUG("disk(%d) slotnum: %d", i, def->disks[i]->slotnum); if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { -- 1.6.2.5