Re: [PATCH] qemu: forbid snapshot-delete --children-only on external snapshot

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

 



On Mon, Oct 27, 2014 at 05:44:29AM -0600, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=956506 documents that
> given a domain where an internal snapshot parent has an external
> snapshot child, we lacked a safety check when trying to use the
> --children-only option to snapshot-delete:
> 
> $ virsh start dom
> $ virsh snapshot-create-as dom internal
> $ virsh snapshot-create-as dom external --disk-only
> $ virsh snapshot-delete dom external
> error: Failed to delete snapshot external
> error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
> $ virsh snapshot-delete dom internal --children
> error: Failed to delete snapshot internal
> error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
> $ virsh snapshot-delete dom internal --children-only
> Domain snapshot internal children deleted
> 
> While I'd still like to see patches that actually do proper external
> snapshot deletion, we should at least fix the inconsistency in the
> meantime.  With this patch:
> 
> $ virsh snapshot-delete dom internal --children-only
> error: Failed to delete snapshot internal
> error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
> 
> * src/qemu/qemu_driver.c (qemuDomainSnapshotDelete): Fix condition.
> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cd8d3c7..5eccacc 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14812,7 +14812,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>          if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) &&
>              virDomainSnapshotIsExternal(snap))
>              external++;
> -        if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> +        if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> +                     VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))
>              virDomainSnapshotForEachDescendant(snap,
>                                                 qemuDomainSnapshotCountExternal,
>                                                 &external);

FWIW, Tested-By: Kashyap Chamarthy <kchamart@xxxxxxxxxx>

I used the same method as you, after applying the patch manually from
the list (and made RPMs):

  $ virsh start vm1
  Domain vm1 started
  
  $  virsh snapshot-create-as vm1 internal
  Domain snapshot internal created
  
  $ virsh snapshot-create-as vm1 external --disk-only
  Domain snapshot external created
  
  $ virsh snapshot-list vm1
   Name                 Creation Time             State
  ------------------------------------------------------------
   external             2014-10-27 16:05:51 +0100 disk-snapshot
   internal             2014-10-27 16:05:39 +0100 running
  
  $ virsh snapshot-delete vm1 external
  error: Failed to delete snapshot external
  error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
  
  $ virsh snapshot-delete vm1 internal --children
  error: Failed to delete snapshot internal
  error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
  
  $ virsh snapshot-delete vm1 internal --children-only
  error: Failed to delete snapshot internal
  error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
  
  $ virsh snapshot-list vm1
   Name                 Creation Time             State
  ------------------------------------------------------------
   external             2014-10-27 16:05:51 +0100 disk-snapshot
   internal             2014-10-27 16:05:39 +0100 running

-- 
/kashyap

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]