From 0165410712e25fffa0585bf026762e3cdc623a9e Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Fri, 17 Jul 2009 22:08:33 +0100 Subject: [PATCH] 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 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 10e6ac6183..91a6c6e4c1 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, " \n"); + if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virBufferAddLit(buf, " pci_addr.domain, + def->pci_addr.bus, + def->pci_addr.slot); + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " \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 69b665f49f..c0fdd65520 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 d2db1a2425..dac5af2834 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 56595e8090..6ee3449103 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) {