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 <[email protected]> >> ---
>> + * 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 <[email protected]>
>
> 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
