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

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@redhat.com> --- 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); -- 1.9.3

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@redhat.com> --- 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@redhat.com> 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

On 10/27/2014 09:14 AM, Kashyap Chamarthy wrote:
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:
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.
FWIW, Tested-By: Kashyap Chamarthy <kchamart@redhat.com>
I used the same method as you, after applying the patch manually from the list (and made RPMs):
I've gone ahead and pushed this patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Kashyap Chamarthy