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. From 1a54a57381704f09491c80a0ef54b43b7b582bb7 Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake@xxxxxxxxxx> 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@xxxxxxxxxx> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> --- 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list