[libvirt] [PATCH 0/2] Fix migration to older versions of libvirt
This series deals with migrating to older versions of libvirt broken by unconditionally added USB controller into domain XML. Jiri Denemark (2): qemu: Don't use virDomainDefFormat* directly qemu: Emit compatible XML when migrating a domain src/qemu/qemu_domain.c | 88 ++++++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 15 ++++++-- src/qemu/qemu_driver.c | 26 ++++++++------ src/qemu/qemu_migration.c | 30 +++++++++------ src/qemu/qemu_process.c | 8 ++-- 5 files changed, 128 insertions(+), 39 deletions(-) -- 1.7.8.6
Always use appropriate qemuDomain{,Def}Format wrapper since it may do some additional magic based on the flags. --- src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 9 +++++---- src/qemu/qemu_migration.c | 22 +++++++++++++--------- src/qemu/qemu_process.c | 8 ++++---- 5 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4dda2e0..b105644 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1223,11 +1223,13 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, } -char *qemuDomainDefFormatXML(struct qemud_driver *driver, - virDomainDefPtr def, - unsigned int flags) +int +qemuDomainDefFormatBuf(struct qemud_driver *driver, + virDomainDefPtr def, + unsigned int flags, + virBuffer *buf) { - char *ret = NULL; + int ret = -1; virCPUDefPtr cpu = NULL; virCPUDefPtr def_cpu = def->cpu; @@ -1247,7 +1249,7 @@ char *qemuDomainDefFormatXML(struct qemud_driver *driver, def->cpu = cpu; } - ret = virDomainDefFormat(def, flags); + ret = virDomainDefFormatInternal(def, flags, buf); cleanup: def->cpu = def_cpu; @@ -1255,6 +1257,26 @@ cleanup: return ret; } +char *qemuDomainDefFormatXML(struct qemud_driver *driver, + virDomainDefPtr def, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (qemuDomainDefFormatBuf(driver, def, flags, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferError(&buf)) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, unsigned int flags) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ce52569..0be99b1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -249,6 +249,11 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuDomainDefFormatBuf(struct qemud_driver *driver, + virDomainDefPtr vm, + unsigned int flags, + virBuffer *buf); + char *qemuDomainDefFormatXML(struct qemud_driver *driver, virDomainDefPtr vm, unsigned int flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c100a1a..a207e33 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4551,7 +4551,7 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, goto cleanup; } - xml = virDomainDefFormat(def, VIR_DOMAIN_XML_INACTIVE); + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE); cleanup: virDomainDefFree(def); @@ -10972,9 +10972,10 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, snap->def->current = true; if (snap->def->dom) { char *xml; - if (!(xml = virDomainDefFormat(snap->def->dom, - (VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE)))) + if (!(xml = qemuDomainDefFormatXML(driver, + snap->def->dom, + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE))) goto cleanup; config = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a420be..d03b1c6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -390,7 +390,8 @@ static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, static int -qemuMigrationCookieXMLFormat(virBufferPtr buf, +qemuMigrationCookieXMLFormat(struct qemud_driver *driver, + virBufferPtr buf, qemuMigrationCookiePtr mig) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -428,10 +429,11 @@ qemuMigrationCookieXMLFormat(virBufferPtr buf, if ((mig->flags & QEMU_MIGRATION_COOKIE_PERSISTENT) && mig->persistent) { virBufferAdjustIndent(buf, 2); - if (virDomainDefFormatInternal(mig->persistent, - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE, - buf) < 0) + if (qemuDomainDefFormatBuf(driver, + mig->persistent, + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE, + buf) < 0) return -1; virBufferAdjustIndent(buf, -2); } @@ -441,11 +443,12 @@ qemuMigrationCookieXMLFormat(virBufferPtr buf, } -static char *qemuMigrationCookieXMLFormatStr(qemuMigrationCookiePtr mig) +static char *qemuMigrationCookieXMLFormatStr(struct qemud_driver *driver, + qemuMigrationCookiePtr mig) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuMigrationCookieXMLFormat(&buf, mig) < 0) { + if (qemuMigrationCookieXMLFormat(driver, &buf, mig) < 0) { virBufferFreeAndReset(&buf); return NULL; } @@ -717,7 +720,7 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, qemuMigrationCookieAddPersistent(mig, dom) < 0) return -1; - if (!(*cookieout = qemuMigrationCookieXMLFormatStr(mig))) + if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) return -1; *cookieoutlen = strlen(*cookieout) + 1; @@ -1235,7 +1238,8 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, char *xml; int hookret; - if (!(xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE))) + if (!(xml = qemuDomainDefFormatXML(driver, def, + VIR_DOMAIN_XML_SECURE))) goto cleanup; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, def->name, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f1401e1..b8e0f5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3322,7 +3322,7 @@ int qemuProcessStart(virConnectPtr conn, /* Run an early hook to set-up missing devices */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = virDomainDefFormat(vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3516,7 +3516,7 @@ int qemuProcessStart(virConnectPtr conn, /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = virDomainDefFormat(vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3956,7 +3956,7 @@ void qemuProcessStop(struct qemud_driver *driver, /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = virDomainDefFormat(vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -4049,7 +4049,7 @@ retry: /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = virDomainDefFormat(vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, -- 1.7.8.6
On 05/04/2012 04:40 PM, Jiri Denemark wrote:
Always use appropriate qemuDomain{,Def}Format wrapper since it may do some additional magic based on the flags. --- src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 9 +++++---- src/qemu/qemu_migration.c | 22 +++++++++++++--------- src/qemu/qemu_process.c | 8 ++++---- 5 files changed, 54 insertions(+), 22 deletions(-)
ACK. Looks like a straightforward refactoring to make the next patch possible. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When we added the default USB controller into domain XML, we efficiently broke migration to older versions of libvirt that didn't support USB controllers at all (0.9.4 and earlier) even for domains that doesn't use anything that the older libvirt can't provide. We still want to present the default USB controller in any XML seen by a user/app but we can safely remove it from the domain XML used during migration. If we are migrating to a new enough libvirt, it will add the controller XML back, while older libvirt won't be confused with it although it will still tell qemu to create the controller. Similar approach can be used in the future whenever we find out we always enabled some kind of device without properly advertising it in domain XML. --- src/qemu/qemu_domain.c | 60 ++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_domain.h | 10 +++++-- src/qemu/qemu_driver.c | 21 +++++++++------ src/qemu/qemu_migration.c | 10 ++++--- src/qemu/qemu_process.c | 8 +++--- 5 files changed, 83 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b105644..3752ddf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1227,11 +1227,14 @@ int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr def, unsigned int flags, + bool compatible, virBuffer *buf) { int ret = -1; virCPUDefPtr cpu = NULL; virCPUDefPtr def_cpu = def->cpu; + virDomainControllerDefPtr *controllers = NULL; + int ncontrollers = 0; /* Update guest CPU requirements according to host CPU */ if ((flags & VIR_DOMAIN_XML_UPDATE_CPU) && @@ -1249,21 +1252,64 @@ qemuDomainDefFormatBuf(struct qemud_driver *driver, def->cpu = cpu; } + if (compatible) { + int i; + virDomainControllerDefPtr usb = NULL; + + /* If only the default USB controller is present, we can remove it + * and make the XML compatible with older versions of libvirt which + * didn't support USB controllers in the XML but always added the + * default one to qemu anyway. + */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { + if (usb) { + usb = NULL; + break; + } + usb = def->controllers[i]; + } + } + if (usb && usb->idx == 0 && usb->model == -1) { + VIR_DEBUG("Removing default USB controller from domain '%s'" + " for migration compatibility", def->name); + controllers = def->controllers; + ncontrollers = def->ncontrollers; + if (VIR_ALLOC_N(def->controllers, ncontrollers - 1) < 0) { + controllers = NULL; + virReportOOMError(); + goto cleanup; + } + + def->ncontrollers = 0; + for (i = 0; i < ncontrollers; i++) { + if (controllers[i] != usb) + def->controllers[def->ncontrollers++] = controllers[i]; + } + } + } + ret = virDomainDefFormatInternal(def, flags, buf); cleanup: def->cpu = def_cpu; virCPUDefFree(cpu); + if (controllers) { + VIR_FREE(def->controllers); + def->controllers = controllers; + def->ncontrollers = ncontrollers; + } return ret; } char *qemuDomainDefFormatXML(struct qemud_driver *driver, virDomainDefPtr def, - unsigned int flags) + unsigned int flags, + bool compatible) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuDomainDefFormatBuf(driver, def, flags, &buf) < 0) { + if (qemuDomainDefFormatBuf(driver, def, flags, compatible, &buf) < 0) { virBufferFreeAndReset(&buf); return NULL; } @@ -1279,7 +1325,8 @@ char *qemuDomainDefFormatXML(struct qemud_driver *driver, char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, - unsigned int flags) + unsigned int flags, + bool compatible) { virDomainDefPtr def; @@ -1288,20 +1335,21 @@ char *qemuDomainFormatXML(struct qemud_driver *driver, else def = vm->def; - return qemuDomainDefFormatXML(driver, def, flags); + return qemuDomainDefFormatXML(driver, def, flags, compatible); } char * qemuDomainDefFormatLive(struct qemud_driver *driver, virDomainDefPtr def, - bool inactive) + bool inactive, + bool compatible) { unsigned int flags = QEMU_DOMAIN_FORMAT_LIVE_FLAGS; if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; - return qemuDomainDefFormatXML(driver, def, flags); + return qemuDomainDefFormatXML(driver, def, flags, compatible); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0be99b1..1256c4e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -252,19 +252,23 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr vm, unsigned int flags, + bool compatible, virBuffer *buf); char *qemuDomainDefFormatXML(struct qemud_driver *driver, virDomainDefPtr vm, - unsigned int flags); + unsigned int flags, + bool compatible); char *qemuDomainFormatXML(struct qemud_driver *driver, virDomainObjPtr vm, - unsigned int flags); + unsigned int flags, + bool compatible); char *qemuDomainDefFormatLive(struct qemud_driver *driver, virDomainDefPtr def, - bool inactive); + bool inactive, + bool compatible); void qemuDomainObjTaint(struct qemud_driver *driver, virDomainObjPtr obj, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a207e33..2bec617 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2630,9 +2630,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainDefFree(def); goto endjob; } - xml = qemuDomainDefFormatLive(driver, def, true); + xml = qemuDomainDefFormatLive(driver, def, true, true); } else { - xml = qemuDomainDefFormatLive(driver, vm->def, true); + xml = qemuDomainDefFormatLive(driver, vm->def, true, true); } if (!xml) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -4321,7 +4321,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, if (fd < 0) goto cleanup; - ret = qemuDomainDefFormatXML(driver, def, flags); + ret = qemuDomainDefFormatXML(driver, def, flags, false); cleanup: virDomainDefFree(def); @@ -4363,8 +4363,10 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, goto cleanup; } - xml = qemuDomainDefFormatXML(driver, def, (VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE)); + xml = qemuDomainDefFormatXML(driver, def, + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE, + true); if (!xml) goto cleanup; len = strlen(xml) + 1; @@ -4511,7 +4513,7 @@ endjob: } } - ret = qemuDomainFormatXML(driver, vm, flags); + ret = qemuDomainFormatXML(driver, vm, flags, false); cleanup: if (vm) @@ -4551,7 +4553,7 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, goto cleanup; } - xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE); + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_INACTIVE, false); cleanup: virDomainDefFree(def); @@ -10406,7 +10408,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true)) || + if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, false)) || !(def->dom = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE))) @@ -10975,7 +10977,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!(xml = qemuDomainDefFormatXML(driver, snap->def->dom, VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE))) + VIR_DOMAIN_XML_SECURE, + false))) goto cleanup; config = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d03b1c6..6f42823 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -433,6 +433,7 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, mig->persistent, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE, + true, buf) < 0) return -1; virBufferAdjustIndent(buf, -2); @@ -1158,9 +1159,9 @@ char *qemuMigrationBegin(struct qemud_driver *driver, if (!virDomainDefCheckABIStability(vm->def, def)) goto cleanup; - rv = qemuDomainDefFormatLive(driver, def, false); + rv = qemuDomainDefFormatLive(driver, def, false, true); } else { - rv = qemuDomainDefFormatLive(driver, vm->def, false); + rv = qemuDomainDefFormatLive(driver, vm->def, false, true); } cleanup: @@ -1239,7 +1240,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, int hookret; if (!(xml = qemuDomainDefFormatXML(driver, def, - VIR_DOMAIN_XML_SECURE))) + VIR_DOMAIN_XML_SECURE, false))) goto cleanup; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, def->name, @@ -2187,7 +2188,8 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, */ if (!(dom_xml = qemuDomainFormatXML(driver, vm, VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_UPDATE_CPU))) + VIR_DOMAIN_XML_UPDATE_CPU, + true))) return -1; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b8e0f5f..0355f8f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3322,7 +3322,7 @@ int qemuProcessStart(virConnectPtr conn, /* Run an early hook to set-up missing devices */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3516,7 +3516,7 @@ int qemuProcessStart(virConnectPtr conn, /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -3956,7 +3956,7 @@ void qemuProcessStop(struct qemud_driver *driver, /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -4049,7 +4049,7 @@ retry: /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, vm->def, 0, false); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, -- 1.7.8.6
On 05/04/2012 04:40 PM, Jiri Denemark wrote:
When we added the default USB controller into domain XML, we efficiently broke migration to older versions of libvirt that didn't support USB controllers at all (0.9.4 and earlier) even for domains that doesn't use
s/doesn't/don't/
anything that the older libvirt can't provide. We still want to present the default USB controller in any XML seen by a user/app but we can safely remove it from the domain XML used during migration. If we are migrating to a new enough libvirt, it will add the controller XML back, while older libvirt won't be confused with it although it will still tell qemu to create the controller.
Similar approach can be used in the future whenever we find out we always enabled some kind of device without properly advertising it in domain XML.
Memballoon would be such a device, if we care about migration to even older libvirt.
--- src/qemu/qemu_domain.c | 60 ++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_domain.h | 10 +++++-- src/qemu/qemu_driver.c | 21 +++++++++------ src/qemu/qemu_migration.c | 10 ++++--- src/qemu/qemu_process.c | 8 +++--- 5 files changed, 83 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b105644..3752ddf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1227,11 +1227,14 @@ int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr def, unsigned int flags, + bool compatible, virBuffer *buf)
Rather than adding a new 'compatible' flag and having to adjust _every_ caller, could we instead add a new VIR_DOMAIN_XML_INTERNAL flag to be or'd in to flags only for the callers that care? On the other hand, we'd have to filter that flag back out before calling into domain_conf, so I guess this approach is as good as any.
{ int ret = -1; virCPUDefPtr cpu = NULL; virCPUDefPtr def_cpu = def->cpu; + virDomainControllerDefPtr *controllers = NULL; + int ncontrollers = 0;
/* Update guest CPU requirements according to host CPU */ if ((flags & VIR_DOMAIN_XML_UPDATE_CPU) && @@ -1249,21 +1252,64 @@ qemuDomainDefFormatBuf(struct qemud_driver *driver, def->cpu = cpu; }
+ if (compatible) { + int i; + virDomainControllerDefPtr usb = NULL; + + /* If only the default USB controller is present, we can remove it + * and make the XML compatible with older versions of libvirt which + * didn't support USB controllers in the XML but always added the + * default one to qemu anyway. + */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { + if (usb) { + usb = NULL; + break; + } + usb = def->controllers[i]; + } + } + if (usb && usb->idx == 0 && usb->model == -1) {
Just like memballoon, if we ever want to explicitly represent an XML with no usb controller, we will have to provide <controller type='usb' mode='none'>; but there, the model would not be -1, so this would properly be migrated.
+ VIR_DEBUG("Removing default USB controller from domain '%s'" + " for migration compatibility", def->name); + controllers = def->controllers; + ncontrollers = def->ncontrollers; + if (VIR_ALLOC_N(def->controllers, ncontrollers - 1) < 0) { + controllers = NULL; + virReportOOMError(); + goto cleanup; + } + + def->ncontrollers = 0; + for (i = 0; i < ncontrollers; i++) { + if (controllers[i] != usb) + def->controllers[def->ncontrollers++] = controllers[i]; + } + } + } + ret = virDomainDefFormatInternal(def, flags, buf);
cleanup: def->cpu = def_cpu; virCPUDefFree(cpu); + if (controllers) { + VIR_FREE(def->controllers); + def->controllers = controllers; + def->ncontrollers = ncontrollers; + }
This looks like it will correctly trim _only_ the default usb controller from modern xml, and restore it after the migration has been prepared. On the receiving side, modern libvirt will know to reconstruct the default usb controller.
+++ b/src/qemu/qemu_driver.c @@ -2630,9 +2630,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainDefFree(def); goto endjob; } - xml = qemuDomainDefFormatLive(driver, def, true); + xml = qemuDomainDefFormatLive(driver, def, true, true); } else { - xml = qemuDomainDefFormatLive(driver, vm->def, true); + xml = qemuDomainDefFormatLive(driver, vm->def, true, true);
Good - save is indeed a migration case, since you can save on one machine and load on another.
@@ -10406,7 +10408,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true)) || + if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, false)) ||
I also agree with this one - although reverting to a snapshot is like loading a saved image, we don't currently migrate snapshots so they are only useful within the same host, so the revert should never be by an older libvirt that didn't recognize the usb controller.
@@ -433,6 +433,7 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver, mig->persistent, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE, + true, buf) < 0)
And of course, the main driver for this patch is migration. It looks to me like you passed the correct 'compatible' flag in all the callers, and that this does allow for cross-migration from newer to older xml. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 04, 2012 at 17:28:39 -0600, Eric Blake wrote:
On 05/04/2012 04:40 PM, Jiri Denemark wrote:
When we added the default USB controller into domain XML, we efficiently broke migration to older versions of libvirt that didn't support USB controllers at all (0.9.4 and earlier) even for domains that doesn't use
s/doesn't/don't/
anything that the older libvirt can't provide. We still want to present the default USB controller in any XML seen by a user/app but we can safely remove it from the domain XML used during migration. If we are migrating to a new enough libvirt, it will add the controller XML back, while older libvirt won't be confused with it although it will still tell qemu to create the controller.
Similar approach can be used in the future whenever we find out we always enabled some kind of device without properly advertising it in domain XML.
Memballoon would be such a device, if we care about migration to even older libvirt.
Right.
--- src/qemu/qemu_domain.c | 60 ++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_domain.h | 10 +++++-- src/qemu/qemu_driver.c | 21 +++++++++------ src/qemu/qemu_migration.c | 10 ++++--- src/qemu/qemu_process.c | 8 +++--- 5 files changed, 83 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b105644..3752ddf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1227,11 +1227,14 @@ int qemuDomainDefFormatBuf(struct qemud_driver *driver, virDomainDefPtr def, unsigned int flags, + bool compatible, virBuffer *buf)
Rather than adding a new 'compatible' flag and having to adjust _every_ caller, could we instead add a new VIR_DOMAIN_XML_INTERNAL flag to be or'd in to flags only for the callers that care? On the other hand, we'd have to filter that flag back out before calling into domain_conf, so I guess this approach is as good as any.
Yeah, we would need to filter it out and we would need to either move the internal flags enum to a more public place (it's internal to domain_conf.c now) or make another enum and make sure neither of them conflicts with the real flags enum :-) I decided to introduce new bool parameter since the compiler checked are places I need to change for me and it's more explicit so that any new caller of this api needs to explicitly think about the value to pass for compatibility.
+ if (usb && usb->idx == 0 && usb->model == -1) {
Just like memballoon, if we ever want to explicitly represent an XML with no usb controller, we will have to provide <controller type='usb' mode='none'>; but there, the model would not be -1, so this would properly be migrated.
Right and we already have a request to do just this. BTW, I don't explicitly check the PCI address assigned to the controller because the default automatically added USB1 controller must always have PCI address 0:0:1.2. When a domains is configured with different address assigned to the default USB controller, libvirt will refuse to start it.
ACK.
Thanks. I pushed this series. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark