[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4 24/51] snapshot: prevent stranding snapshot data on domain destruction



Hello Eric,

On Friday 02 September 2011 06:25:01 Eric Blake wrote:
> Just as leaving managed save metadata behind can cause problems
> when creating a new domain that happens to collide with the name
> of the just-deleted domain, the same is true of leaving any
> snapshot metadata behind.

I just noticed a problem with that patch 
282fe1f08c89189e36142fc2d12bae0175038bdd:

> index ac71b04..38a21db 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4852,6 +4853,14 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>          goto cleanup;
>      }
>
> +    if (!virDomainObjIsActive(vm) &&

This check restricts the test to only running domains, that is if you undefine 
a currently inactive domain, its snapshot metadata is left behind while the 
domain is deleted.
This behaviour is actually documented in 
<http://libvirt.org/html/libvirt-libvirt.html#virDomainUndefineFlags> (now 
that I know what I have to look at), but I was still surprised that 
virDomainUndefineFlags(0) returned success on my inactive domain with 
snapshots.

> +        (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                        _("cannot delete inactive domain with %d
> snapshots"), +                        nsnapshots);
> +        goto cleanup;
> +    }
> +
>      if (!vm->persistent) {
>          qemuReportError(VIR_ERR_OPERATION_INVALID,
>                          "%s", _("cannot undefine transient domain"));

Is this restriction to only delete the snapshots for active domains on purpose 
or am I missing something?
I would expect the undefine to be refused for all states, not just for active 
domains.
I would prefer the snapshots to be deleted for both active and inactive 
domains, since qemuDomainSnapshotDiscardAllMetadata() is not available 
externally. And iterating over all snapshots to just delete them seems to be 
wasteful, especially when you use qcow2 with its reference counting issues.

See the attached patch for my proposal.

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn univention de
Univention GmbH        be open.                       fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/
From 8f1e38f9a182a5baa0ef275976c5d35360b384a4 Mon Sep 17 00:00:00 2001
Message-Id: <8f1e38f9a182a5baa0ef275976c5d35360b384a4 1351266167 git hahn univention de>
In-Reply-To: <1314937528-8318-25-git-send-email-eblake redhat com>
References: <1314937528-8318-25-git-send-email-eblake redhat com>
From: Philipp Hahn <hahn univention de>
Date: Fri, 26 Oct 2012 16:43:10 +0200
Subject: [PATCH] Prevent undefine of QEMU domains with snapshots
Organization: Univention GmbH, Bremen, Germany
To: libvir-list redhat com

Commit 282fe1f08c89189e36142fc2d12bae0175038bdd added a test to prevent
un-defining domains with snapshots, which causes problems with QEMU when
a new domain re-uses the name of those domains.
The check is currently only performed for running domains, while still
leaving behind the data for inactive domains.

Remove the check for running domains and always refuse to undefine
a domain with snapshots unless forced by
VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA.

Signed-off-by: Philipp Hahn <hahn univention de>
---
 src/libvirt.c          |    6 +++---
 src/qemu/qemu_driver.c |    3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 7e79470..2a5cfe6 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -8039,11 +8039,11 @@ error:
  * then including VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in @flags will also remove
  * that file, and omitting the flag will cause the undefine process to fail.
  *
- * If the domain is inactive and has any snapshot metadata (see
+ * If the domain has any snapshot metadata (see
  * virDomainSnapshotNum()), then including
  * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove
- * that metadata.  Omitting the flag will cause the undefine of an
- * inactive domain to fail.  Active snapshots will retain snapshot
+ * that metadata.  Omitting the flag will cause the undefine of the
+ * domain to fail.  Active snapshots will retain snapshot
  * metadata until the (now-transient) domain halts, regardless of
  * whether this flag is present.  On hypervisors where snapshots do
  * not use libvirt metadata, this flag has no effect.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 18be7d9..a09376e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5736,8 +5736,7 @@ qemuDomainUndefineFlags(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (!virDomainObjIsActive(vm) &&
-        (nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) {
+    if ((nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) {
         if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) {
             virReportError(VIR_ERR_OPERATION_INVALID,
                            _("cannot delete inactive domain with %d "
-- 
1.7.1

Attachment: signature.asc
Description: This is a digitally signed message part.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]