[libvirt] [PATCH 05/16] snapshot: Drop virDomainSnapshotDef.current

[Eric Blake]

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 at 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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190321/9046ce2b/attachment-0001.sig>