[libvirt] [PATCH] Don't replace persistent domain config with migrated config
When a domain is defined on host1, migrated to host2 and then migrated back to host1, its current configuration would overwrite the libvirtd's in-memory copy of persistent configuration of that domain. This is not desired as we want to preserve the persistent configuration untouched. This patch introduces new 'live' parameter to virDomainAssignDef. Passing 'true' for 'live' means the configuration passed to virDomainAssignDef describes a configuration of live instance of the domain. This applies for saved domains which are being restored or for incoming domains during migration. All callers have been changed to pass the appropriate value. --- src/conf/domain_conf.c | 19 +++++++++++++------ src/conf/domain_conf.h | 5 ++++- src/lxc/lxc_driver.c | 4 ++-- src/opennebula/one_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/qemu/qemu_driver.c | 10 +++++----- src/test/test_driver.c | 10 +++++----- src/uml/uml_driver.c | 4 ++-- 8 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 22e1679..4cc27ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -749,18 +749,25 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps) virDomainObjPtr virDomainAssignDef(virCapsPtr caps, virDomainObjListPtr doms, - const virDomainDefPtr def) + const virDomainDefPtr def, + bool live) { virDomainObjPtr domain; char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((domain = virDomainFindByUUID(doms, def->uuid))) { if (!virDomainObjIsActive(domain)) { - virDomainDefFree(domain->def); - domain->def = def; + if (live) { + /* save current configuration to be restored on domain shutdown */ + if (!domain->newDef) + domain->newDef = domain->def; + domain->def = def; + } else { + virDomainDefFree(domain->def); + domain->def = def; + } } else { - if (domain->newDef) - virDomainDefFree(domain->newDef); + virDomainDefFree(domain->newDef); domain->newDef = def; } @@ -5780,7 +5787,7 @@ virDomainObjPtr virDomainLoadConfig(virCapsPtr caps, newVM = 0; } - if (!(dom = virDomainAssignDef(caps, doms, def))) + if (!(dom = virDomainAssignDef(caps, doms, def, false))) goto error; dom->autostart = autostart; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 44fff0c..9e6cb69 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -810,9 +810,12 @@ void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ int virDomainObjUnref(virDomainObjPtr vm); +/* live == true means def describes an active domain (being migrated or + * restored) as opposed to a new persistent configuration of the domain */ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, virDomainObjListPtr doms, - const virDomainDefPtr def); + const virDomainDefPtr def, + bool live); void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainObjPtr dom); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ba13065..9786cc0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -381,7 +381,7 @@ static virDomainPtr lxcDomainDefine(virConnectPtr conn, const char *xml) } if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, def))) + &driver->domains, def, false))) goto cleanup; def = NULL; vm->persistent = 1; @@ -1368,7 +1368,7 @@ lxcDomainCreateAndStart(virConnectPtr conn, if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, def))) + &driver->domains, def, false))) goto cleanup; def = NULL; diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index e1d1efc..f7fbd46 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -251,7 +251,7 @@ static virDomainPtr oneDomainDefine(virConnectPtr conn, const char *xml) goto return_point; if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, def))) { + &driver->domains, def, false))) { virDomainDefFree(def); goto return_point; } @@ -456,7 +456,7 @@ oneDomainCreateAndStart(virConnectPtr conn, } if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, def))) { + &driver->domains, def, false))) { virDomainDefFree(def); goto return_point; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 50aadfc..e0a0768 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -825,7 +825,7 @@ openvzDomainDefineXML(virConnectPtr conn, const char *xml) goto cleanup; } if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, vmdef))) + &driver->domains, vmdef, false))) goto cleanup; vmdef = NULL; vm->persistent = 1; @@ -905,7 +905,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, goto cleanup; } if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, vmdef))) + &driver->domains, vmdef, false))) goto cleanup; vmdef = NULL; /* All OpenVZ domains seem to be persistent - this is a bit of a violation diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 257f914..2c81d68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3563,7 +3563,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) + def, false))) goto cleanup; def = NULL; @@ -5220,7 +5220,7 @@ static int qemudDomainRestore(virConnectPtr conn, if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) { + def, true))) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to assign new VM")); goto cleanup; @@ -5761,7 +5761,7 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) { + def, false))) { goto cleanup; } def = NULL; @@ -8391,7 +8391,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) { + def, true))) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to assign new VM")); goto cleanup; @@ -8622,7 +8622,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) { + def, true))) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to assign new VM")); goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f54ebae..9a880f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -554,7 +554,7 @@ static int testOpenDefault(virConnectPtr conn) { if (testDomainGenerateIfnames(conn, domdef) < 0) goto error; if (!(domobj = virDomainAssignDef(privconn->caps, - &privconn->domains, domdef))) + &privconn->domains, domdef, false))) goto error; domdef = NULL; @@ -910,7 +910,7 @@ static int testOpenFromFile(virConnectPtr conn, if (testDomainGenerateIfnames(conn, def) < 0 || !(dom = virDomainAssignDef(privconn->caps, - &privconn->domains, def))) { + &privconn->domains, def, false))) { virDomainDefFree(def); goto error; } @@ -1308,7 +1308,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; if (!(dom = virDomainAssignDef(privconn->caps, - &privconn->domains, def))) + &privconn->domains, def, false))) goto cleanup; def = NULL; @@ -1853,7 +1853,7 @@ static int testDomainRestore(virConnectPtr conn, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; if (!(dom = virDomainAssignDef(privconn->caps, - &privconn->domains, def))) + &privconn->domains, def, true))) goto cleanup; def = NULL; @@ -2302,7 +2302,7 @@ static virDomainPtr testDomainDefineXML(virConnectPtr conn, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; if (!(dom = virDomainAssignDef(privconn->caps, - &privconn->domains, def))) + &privconn->domains, def, false))) goto cleanup; def = NULL; dom->persistent = 1; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bf06787..0c12469 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1283,7 +1283,7 @@ static virDomainPtr umlDomainCreate(virConnectPtr conn, const char *xml, if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) + def, false))) goto cleanup; def = NULL; @@ -1619,7 +1619,7 @@ static virDomainPtr umlDomainDefine(virConnectPtr conn, const char *xml) { if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def))) + def, false))) goto cleanup; def = NULL; vm->persistent = 1; -- 1.7.0.3
On 03/24/2010 08:52 AM, Jiri Denemark wrote:
When a domain is defined on host1, migrated to host2 and then migrated back to host1, its current configuration would overwrite the libvirtd's in-memory copy of persistent configuration of that domain. This is not desired as we want to preserve the persistent configuration untouched.
This patch introduces new 'live' parameter to virDomainAssignDef. Passing 'true' for 'live' means the configuration passed to virDomainAssignDef describes a configuration of live instance of the domain. This applies for saved domains which are being restored or for incoming domains during migration.
ACK.
virDomainObjPtr virDomainAssignDef(virCapsPtr caps,
...
} else { - if (domain->newDef) - virDomainDefFree(domain->newDef); + virDomainDefFree(domain->newDef);
And nice removal of useless-if-before-free. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/24/2010 08:52 AM, Jiri Denemark wrote: ...
virDomainObjPtr virDomainAssignDef(virCapsPtr caps, ...
} else { - if (domain->newDef) - virDomainDefFree(domain->newDef); + virDomainDefFree(domain->newDef);
And nice removal of useless-if-before-free.
Thanks for highlighting that. Our syntax-check rule should have caught it. With the following change, it does, along with one other: src/conf/domain_conf.c: if (domain->newDef) virDomainDefFree(domain->newDef) src/test/test_driver.c: if (def) virDomainDefFree(def) maint.mk: found useless "if" before "free" above make: *** [sc_avoid_if_before_free] Error 1 This also fixes the one not fixed by Jiri's change.
From 032b5a8b01ae00c00975ea32eb045dbee620331f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 25 Mar 2010 21:53:29 +0100 Subject: [PATCH] tests: teach syntax-check that virDomainDefFree has free-like semantics
* cfg.mk (useless_free_options): Add virDomainDefFree to the list of free-like functions. * src/test/test_driver.c (testDomainCreateXML): Remove useless-if- before-virDomainDefFree. --- cfg.mk | 1 + src/test/test_driver.c | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4302338..bf5eae3 100644 --- a/cfg.mk +++ b/cfg.mk @@ -64,6 +64,7 @@ useless_free_options = \ --name=VIR_FREE \ --name=xmlFree \ --name=xmlXPathFreeContext \ + --name=virDomainDefFree \ --name=xmlXPathFreeObject # Avoid uses of write(2). Either switch to streams (fwrite), or use diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f54ebae..fb5c3f6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1328,8 +1328,7 @@ cleanup: virDomainObjUnlock(dom); if (event) testDomainEventQueue(privconn, event); - if (def) - virDomainDefFree(def); + virDomainDefFree(def); testDriverUnlock(privconn); return ret; } -- 1.7.0.3.448.g82eeb
On 03/25/2010 02:56 PM, Jim Meyering wrote:
Thanks for highlighting that. Our syntax-check rule should have caught it. With the following change, it does, along with one other:
src/conf/domain_conf.c: if (domain->newDef) virDomainDefFree(domain->newDef) src/test/test_driver.c: if (def) virDomainDefFree(def) maint.mk: found useless "if" before "free" above make: *** [sc_avoid_if_before_free] Error 1
This also fixes the one not fixed by Jiri's change.
ACK -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/25/2010 02:56 PM, Jim Meyering wrote:
Thanks for highlighting that. Our syntax-check rule should have caught it. With the following change, it does, along with one other:
src/conf/domain_conf.c: if (domain->newDef) virDomainDefFree(domain->newDef) src/test/test_driver.c: if (def) virDomainDefFree(def) maint.mk: found useless "if" before "free" above make: *** [sc_avoid_if_before_free] Error 1
This also fixes the one not fixed by Jiri's change.
ACK
Thanks for the review.. I've gone ahead and fixed both violations, so I can push this right away without breaking syntax-check, rather than having to wait for Jiri's change to be committed. Besides, it is more appropriate to keep clean-up style changes separate from "real" (semantics-changing) ones.
From 156133597dee6e79768c267b2fb4b5ebff5998f0 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 25 Mar 2010 21:53:29 +0100 Subject: [PATCH] tests: teach syntax-check that virDomainDefFree has free-like semantics
* cfg.mk (useless_free_options): Add virDomainDefFree to the list of free-like functions. * src/test/test_driver.c (testDomainCreateXML): Remove useless-if- before-virDomainDefFree. * src/conf/domain_conf.c (virDomainAssignDef): Likewise --- cfg.mk | 1 + src/conf/domain_conf.c | 3 +-- src/test/test_driver.c | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4302338..bf5eae3 100644 --- a/cfg.mk +++ b/cfg.mk @@ -64,6 +64,7 @@ useless_free_options = \ --name=VIR_FREE \ --name=xmlFree \ --name=xmlXPathFreeContext \ + --name=virDomainDefFree \ --name=xmlXPathFreeObject # Avoid uses of write(2). Either switch to streams (fwrite), or use diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 22e1679..5519834 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -759,8 +759,7 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, virDomainDefFree(domain->def); domain->def = def; } else { - if (domain->newDef) - virDomainDefFree(domain->newDef); + virDomainDefFree(domain->newDef); domain->newDef = def; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f54ebae..fb5c3f6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1328,8 +1328,7 @@ cleanup: virDomainObjUnlock(dom); if (event) testDomainEventQueue(privconn, event); - if (def) - virDomainDefFree(def); + virDomainDefFree(def); testDriverUnlock(privconn); return ret; } -- 1.7.0.3.448.g82eeb
participants (3)
-
Eric Blake -
Jim Meyering -
Jiri Denemark