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

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(a)redhat.com&gt; >>> --- > >>> +++ 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(a)redhat.com&gt; 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.