[libvirt] [PATCH] metadata: track title edits across libvirtd restart
https://bugzilla.redhat.com/show_bug.cgi?id=1122205 Although the edits were changing in-memory XML, it was not flushed to disk; so unless some other action changes XML, a libvirtd restart would lose the changed information. * src/conf/domain_conf.c (virDomainObjSetMetadata): Add parameter, to save live status across restarts. (virDomainSaveXML): Allow for test driver. * src/conf/domain_conf.h (virDomainObjSetMetadata): Adjust signature. * src/bhyve/bhyve_driver.c (bhyveDomainSetMetadata): Adjust caller. * src/lxc/lxc_driver.c (lxcDomainSetMetadata): Likewise. * src/qemu/qemu_driver.c (qemuDomainSetMetadata): Likewise. * src/test/test_driver.c (testDomainSetMetadata): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/bhyve/bhyve_driver.c | 3 ++- src/conf/domain_conf.c | 9 ++++++++- src/conf/domain_conf.h | 1 + src/lxc/lxc_driver.c | 3 ++- src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 2 +- 6 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 9a13076..135cb24 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1046,7 +1046,8 @@ bhyveDomainSetMetadata(virDomainPtr dom, goto cleanup; ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps, - privconn->xmlopt, BHYVE_CONFIG_DIR, flags); + privconn->xmlopt, BHYVE_STATE_DIR, + BHYVE_CONFIG_DIR, flags); cleanup: virObjectUnref(caps); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4b00280..6ed6155 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18111,6 +18111,9 @@ virDomainSaveXML(const char *configDir, char *configFile = NULL; int ret = -1; + if (!configDir) + return 0; + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup; @@ -19741,6 +19744,7 @@ virDomainObjSetMetadata(virDomainObjPtr vm, const char *uri, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + const char *stateDir, const char *configDir, unsigned int flags) { @@ -19753,9 +19757,12 @@ virDomainObjSetMetadata(virDomainObjPtr vm, &persistentDef) < 0) return -1; - if (flags & VIR_DOMAIN_AFFECT_LIVE) + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (virDomainDefSetMetadata(vm->def, type, metadata, key, uri) < 0) return -1; + if (virDomainSaveStatus(xmlopt, stateDir, vm) < 0) + return -1; + } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (virDomainDefSetMetadata(persistentDef, type, metadata, key, uri) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f5eb031..d89bbe7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2715,6 +2715,7 @@ int virDomainObjSetMetadata(virDomainObjPtr vm, const char *uri, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + const char *stateDir, const char *configDir, unsigned int flags); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b7b4b02..855222f 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -5600,7 +5600,8 @@ lxcDomainSetMetadata(virDomainPtr dom, goto cleanup; ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps, - driver->xmlopt, cfg->configDir, flags); + driver->xmlopt, cfg->stateDir, + cfg->configDir, flags); cleanup: virObjectUnlock(vm); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9d23f6d..72e4a91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16128,7 +16128,8 @@ qemuDomainSetMetadata(virDomainPtr dom, goto cleanup; ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps, - driver->xmlopt, cfg->configDir, flags); + driver->xmlopt, cfg->stateDir, + cfg->configDir, flags); cleanup: virObjectUnlock(vm); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 67f637d..3b22cf6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3042,7 +3042,7 @@ static int testDomainSetMetadata(virDomainPtr dom, ret = virDomainObjSetMetadata(privdom, type, metadata, key, uri, privconn->caps, privconn->xmlopt, - NULL, flags); + NULL, NULL, flags); cleanup: if (privdom) -- 1.9.3
On Tue, Jul 22, 2014 at 01:11:03PM -0600, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1122205
Although the edits were changing in-memory XML, it was not flushed to disk; so unless some other action changes XML, a libvirtd restart would lose the changed information.
And there are more places like that. Take a qemuDomainSetNumaParameters() for example. Would it be possible (as in "not devastatingly hard" to create a syntax-check rule for *_driver.c files (only those that are applicable, mostly stateful) that would check if virCheckFlags is called with VIR_DOMAIN_AFFECT_LIVE and if yes, then that function would have to call virDomainSaveStatus() as well? [...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4b00280..6ed6155 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -19753,9 +19757,12 @@ virDomainObjSetMetadata(virDomainObjPtr vm, &persistentDef) < 0) return -1;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (virDomainDefSetMetadata(vm->def, type, metadata, key, uri) < 0) return -1;
One empty line right here would perfectly go with the rest of the function ;) ACK with or without this change, Martin
On 07/22/2014 11:09 PM, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 01:11:03PM -0600, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1122205
Although the edits were changing in-memory XML, it was not flushed to disk; so unless some other action changes XML, a libvirtd restart would lose the changed information.
And there are more places like that. Take a qemuDomainSetNumaParameters() for example.
That's what I was afraid of - that I'd be stuck with auditing other uses all because I stumbled on this one while testing a similar fix to blockcopy. :/
Would it be possible (as in "not devastatingly hard" to create a syntax-check rule for *_driver.c files (only those that are applicable, mostly stateful) that would check if virCheckFlags is called with VIR_DOMAIN_AFFECT_LIVE and if yes, then that function would have to call virDomainSaveStatus() as well?
Via a .pl script maybe, but even that is hard, because sometimes (as in this patch) the call to virDomainSaveStatus() is delegated to a helper function in another file than where the API call checks flags. It's not just AFFECT_LIVE, but any time where we modify a virDomainDefPtr (whether live or config), then those modifications have to be written back to disk before ending the API.
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4b00280..6ed6155 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -19753,9 +19757,12 @@ virDomainObjSetMetadata(virDomainObjPtr vm, &persistentDef) < 0) return -1;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (virDomainDefSetMetadata(vm->def, type, metadata, key, uri) < 0) return -1;
One empty line right here would perfectly go with the rest of the function ;)
ACK with or without this change,
Martin
Thanks, will push shortly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jul 23, 2014 at 06:46:24AM -0600, Eric Blake wrote:
On 07/22/2014 11:09 PM, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 01:11:03PM -0600, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1122205
Although the edits were changing in-memory XML, it was not flushed to disk; so unless some other action changes XML, a libvirtd restart would lose the changed information.
And there are more places like that. Take a qemuDomainSetNumaParameters() for example.
That's what I was afraid of - that I'd be stuck with auditing other uses all because I stumbled on this one while testing a similar fix to blockcopy. :/
Would it be possible (as in "not devastatingly hard" to create a syntax-check rule for *_driver.c files (only those that are applicable, mostly stateful) that would check if virCheckFlags is called with VIR_DOMAIN_AFFECT_LIVE and if yes, then that function would have to call virDomainSaveStatus() as well?
Via a .pl script maybe, but even that is hard, because sometimes (as in this patch) the call to virDomainSaveStatus() is delegated to a helper function in another file than where the API call checks flags. It's not just AFFECT_LIVE, but any time where we modify a virDomainDefPtr (whether live or config), then those modifications have to be written back to disk before ending the API.
Or maybe CIL or something similar can be used for that. Having syntax-check work with the intermediate language would have both pros and cons, I guess. Martin
participants (2)
-
Eric Blake -
Martin Kletzander