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@xxxxxxxxxx>
>>>> ---
>>
>
>>>> +++ 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@xxxxxxxxxx>
>
> 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
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
