[libvirt] [PATCH 06/16] snapshot: Track current snapshot in virDomainSnapshotObjList

[Eric Blake]

On 3/20/19 3:39 PM, John Ferlan wrote:
> 
> 
> On 3/20/19 1:40 AM, Eric Blake wrote:
>> It is easier to track the current snapshot as part of the list of
>> snapshots. In particular, doing so lets us guarantee that the current
>> snapshot is cleared if that snapshot is removed from the list (rather
>> than depending on the caller to do so, and risking a use-after-free
>> problem).  This requires the addition of several new accessor
>> functions.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---

>> +
>> +/* Remove snapshot from the list; return true if it was current */
>> +bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
>>                                      virDomainSnapshotObjPtr snapshot)
> 
> NIT: One of these things is not like the others ;-)
> 
> bool
> virDomain...

I need to quit sending patches at midnight.

> 
>>  {
>> +    bool ret = snapshots->current == snapshot;
>>      virHashRemoveEntry(snapshots->objs, snapshot->def->name);
>> +    if (ret)
>> +        snapshots->current = NULL;
> 
> Slick, this is how testDomainSnapshotDiscardAll can alter it's logic.
> Took me until the end of the patch to find ;-)... and coverity didn't
> whine about one function checking return while the others don't.
> 
>> +    return ret;

You noticed that this clears the current snapshot... [1]

>>  }
>>
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 7e0e76a31a..6c71382b93 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>>          if (snap == NULL) {
>>              virDomainSnapshotDefFree(def);
>>          } else if (cur) {
>> +            if (current)
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("Too many snapshots claiming to be current for domain %s"),
>> +                               vm->def->name);
> 
> Even though we generate this message we go ahead and update @current to
> @snap. Should this be an "if (current) ... else ... " ?
> 
> Additionally if someone's really AFU'd, they could get more than one
> message; whereas, previously they'd only get one such message.

It's a tough call. Anyone messing around with
/var/lib/libvrt/qemu/snapshot/$dom to trigger the problem in the first
place is already unsupported territory, where who knows what libvirt
will do with their garbage. Maybe I should just do a standalone patch
that quits trying to play nice (by merely setting no current snapshot
after a warning) and instead hard-fail the loading of any more
snapshots.  (After all, I have a patch in the pipeline that does a bulk
load, and THAT patch refuses to load ANY snapshots if the xml has been
modified incorrectly behind libvirt's back, rather than trying to play
nice and still load as many snapshots as possible).


>> -    if (vm->current_snapshot != current) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Too many snapshots claiming to be current for domain %s"),
>> -                       vm->def->name);
>> -        vm->current_snapshot = NULL;
> 
> Previously if this happened, then current was set to NULL, but the
> following will set it to the last snap declared to be current.
> 
> Is that expected?  If not, then perhaps the if (current) above needs to
> add a "current = NULL;" along with the error message. Of course that
> leads to the possibility of others declaring themselves current and
> possibly having multiple errors splatted.

And getting different behavior depending on whether the user had an even
or odd number of domains claiming to be current.

> Only seems to matter for
> someone running debug or looking at debug logs since we don't fail.
> 
> BTW: This is one of those current gray areas of making two changes in
> one patch.  One change being the usage of the accessors and the other
> being the alteration of when this message gets splatted.

Okay, you've convinced me to try and split it.


>> @@ -15927,7 +15923,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>>                             _("unable to save metadata for snapshot %s"),
>>                             snap->def->name);
>>              virDomainSnapshotObjListRemove(vm->snapshots, snap);
>> -            vm->current_snapshot = NULL;
> 
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> 
> right?

Not needed - virDomainSnapshotObjListRemove() takes care of it, back at [1].

> 
>>          } else {
>>              other = virDomainSnapshotFindByName(vm->snapshots,
>>                                                  snap->def->parent);
> 
> [...]
> 
>>
>> @@ -16459,7 +16453,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>       *
>>       * XXX Should domain snapshots track live xml rather
>>       * than inactive xml?  */
>> -    vm->current_snapshot = snap;
> 
> virDomainSnapshotSetCurrent(vm->snapshots, snap); ?

This one's trickier, but still, not needed: look at the cleanup: label,
which calls virDomainSnapshotSetCurrent(vm->snapshots, snap); on success.

> 
>>      if (snap->def->dom) {
>>          config = virDomainDefCopy(snap->def->dom, caps,
>>                                    driver->xmlopt, NULL, true);
> 
> [...]
> 
> The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
> can answer and things will be fine.
> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> John
> 

-- 
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/20190320/c2e1840f/attachment-0001.sig>