On 3/20/19 2:57 PM, John Ferlan wrote:
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> ---
+ * 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...
Good call. I'm squashing this in: diff --git i/src/conf/snapshot_conf.c w/src/conf/snapshot_conf.c index bf5fdc0647..65094766f0 100644 --- i/src/conf/snapshot_conf.c +++ w/src/conf/snapshot_conf.c @@ -347,6 +347,11 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { + if (!current) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("internal parse requested with NULL current")); + goto cleanup; + } if (virXPathInt("string(./active)", ctxt, &active) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find 'active' element"));
}
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.
Yeah, and the next few patches clear it up by getting rid of current_snapshot altogether :) (well, moving it into ObjList). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org