On 3/20/19 1:40 AM, Eric Blake wrote:
The only use for the 'current' member of virDomainSnapshotDef was with the PARSE/FORMAT_INTERNAL flag for controlling an internal-use <active> element marking whether a particular snapshot definition was current, and even then, only by the qemu driver on output, and by qemu and test driver on input. But this duplicates vm->snapshot_current, and gets in the way of potential simplifications to have qemu store a single file for all snapshots rather than one file per snapshot. Get rid of the member by adding a bool* parameter during parse (ignored if the PARSE_INTERNAL flag is not set), and by adding a new flag during format (if FORMAT_INTERNAL is set, the value printed in <active> depends on the new FORMAT_CURRENT). Then update the qemu driver accordingly.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 6 ++-- src/conf/snapshot_conf.c | 18 ++++++---- src/conf/virdomainsnapshotobjlist.c | 4 +-- src/esx/esx_driver.c | 2 +- src/qemu/qemu_domain.c | 18 +++++----- src/qemu/qemu_driver.c | 51 +++++++++++++++-------------- src/test/test_driver.c | 5 ++- src/vbox/vbox_common.c | 4 +-- src/vz/vz_driver.c | 3 +- tests/domainsnapshotxml2xmltest.c | 5 ++- 10 files changed, 66 insertions(+), 50 deletions(-)
[...]
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ffb1313c89..bf5fdc0647 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -184,12 +184,14 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
/* flags is bitwise-or of virDomainSnapshotParseFlags. * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then - * caps are ignored. + * caps are ignored. If flags does not include + * VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL, then current is ignored. */ static virDomainSnapshotDefPtr virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + bool *current, unsigned int flags) { virDomainSnapshotDefPtr def = NULL; @@ -350,7 +352,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, _("Could not find 'active' element")); goto cleanup; } - def->current = active != 0; + *current = active != 0;
Even though we've restricted usage via @flags where PARSE_INTERNAL is set, should this be prefaced by: if (current) guess I'm concerned with some future cut-copy-paste error...
}
if (!offline && virSaveCookieParse(ctxt, &def->cookie, saveCookie) < 0)
Reviewed-by: John Ferlan <jferlan@redhat.com> John The setting of ->current_snapshot in/around calls to qemuDomainSnapshotWriteMetadata was particularly "interesting" to follow, but looks fine. [...]