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

[John Ferlan]

On 3/21/19 3:58 PM, Eric Blake wrote:
> On 3/20/19 4:03 PM, Eric Blake wrote:
>> 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>
>>>> ---
>>
> 
>>>> +++ 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).
> 
> 
>>> 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.
> 
> I'm posting what I split out for the logic change (where I had fun
> writing the commit message);
> 
>>> 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>
> 
> and I'm going to be bold and apply your R-b to both halves of the split
> rather than make you re-read it in a v2.

That's fine.

John

[...]