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@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@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 [...]