2:58 p.m.
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>
>> ---
>> +++ 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>
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.
From 1a54a57381704f09491c80a0ef54b43b7b582bb7 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake(a)redhat.com>
Date: Thu, 21 Mar 2019 12:16:03 -0500
Subject: [PATCH] snapshot: Rework parse logic during libvirt startup
Rework the logic in qemuDomainSnapshotLoad() to set
vm->current_snapshot only once at the end of the loop, rather than
repeatedly querying it during the loop, to make it easier for the next
patch to use accessor functions rather than direct manipulation of
vm->current_snapshot. When encountering multiple snapshots claiming
to be current (based on the presence of an <active>1</active> element
in the XML, which libvirt only outputs for internal use and not for
any public API), this changes behavior from warning only once and
running with no current snapshot, to instead warning on each duplicate
and selecting the last one encountered (which is arbitrary based on
readdir() ordering, but actually stands a fair chance of being the
most-recently created snapshot whether by timestamp or by the
propensity of humans to name things in ascending order).
Note that the code in question is only run by libvirtd when it first
starts, reading state from disk from the previous run into memory for
this run. Since the data resides somewhere that only libvirt should be
touching (typically /var/lib/libvirt/qemu/snapshot/*), it should be
clean. So in the common case, the code touched here is unreachable.
But if someone is actually messing with files behind libvirt's back,
they deserve the change in behavior.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 13a34ef860..e49924ce1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1,7 +1,7 @@
/*
* qemu_driver.c: core driver methods for managing qemu guests
*
- * Copyright (C) 2006-2016 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -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);
current = snap;
- if (!vm->current_snapshot)
- vm->current_snapshot = snap;
}
VIR_FREE(fullpath);
@@ -495,13 +497,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
_("Failed to fully read directory %s"),
snapDir);
- 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;
- }
-
+ vm->current_snapshot = current;
if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Snapshots have inconsistent relations for
domain %s"),
--
2.20.1
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org