4:03 p.m.
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>
> ---
> +
> +/* 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(a)redhat.com>
John
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org