Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Wed, Apr 22, 2020 at 03:49:57PM +0200, Peter Krempa wrote: > On Thu, Apr 16, 2020 at 17:07:35 +0200, Pavel Mores wrote: > > On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote: > > > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > > > +} > > > > > + > > > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > > > +topPath = disk->src->path; > > > > > +else > > > > > +topPath = snapdisk->src->path; > > > > > > > > You must not use paths for doing this lookup. Paths at best work for > > > > local files only and this would make the code not future proof. > > > > > > > > Also you want to verify that: > > > > - images you want to merge are actually in the backing chain > > > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > > > and plug a different chain back) > > > > > > Let me check with you how exactly to perform this verification. > > > > > > To retrieve the names of the disks included in a snapshot, I can use its > > > virDomainSnapshotDef which contains an array of disks > > > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify > > > disks. > > > > > > To get the state of the chain at moment the snapshot was created, I can > > > use the > > > 'src' member and walk its 'backingStore' list to examine the whole chain. > > > > > > To get the currently running configuration, I assume I can use the names > > > of the > > > relevant disks (obtained from the snapshot as mentioned above) to get a > > > virDomainDiskDefPtr for each of them, whose 'src' member points to a > > > virStorageSource that represents the topmost image in the disk's chain. > > > From > > > there, I can walk the 'backingStore' list to get the other images in the > > > chain > > > all the way to the base image. > > > > > > The verification succeeds if the disk names and their chains in the > > > snapshot > > > and the running config correspond. Is the above correct? > > > > > > If so, I'm still not certain how to compare two virStorageSource's. How > > > is > > > identity of two different virStorageSource instances is established? > > > > After having a closer look into this I'm unfortunately even less clear as to > > how to perform the checks mentioned in the review. > > Well unfortunately developing this is the main part why deleting > snapshots is complicated. > > > In addition to the problem of establishing virStorageSource identity, a > > similar > > problem seems to come up with disks. They often seem to be identified and > > referred to by the target name, however what happens if a snapshot is taken > > for > > 'vda', then the disk's target is changed to 'vdb' and now the snapshot is > > to be > > In such case I'd consier it as being different. Similarly as we can't > really guarantee that the image described by a virStorageSource is the > same as it was when taking the snapshot. As long as the file is named > the same you can consider it identical IMO. > > > deleted? Is there a way to find out that the disk is still there, only > > under a > > different name? > > No and it doesn't seem to be worth doing that. > > > > > And as far as comparing chains goes - I know can retrieve the chain for a > > running disk and I can retrieve what the chain looked like at the point a > > snapshot was made. However, how do I establish they are equivalent? How > > can I > > What I meant is that you need to establish that the images you are going > to merge and the images you are going to merge into are the same > described by the snapshots and at the same time present in the current > backing chain of the disk. If they are not in the snapshot metadata > you'd potentially merge something unwanted, but this is covered by the > previous paragraphs. > > You need to also verify that they are currently opened by qemu as you > can't do blockjobs on images which are not part of the chain. > > > tell that snapshots in both chains compare identical in the first place? Is > > the snapshot name stable and persistent enough to be useful for this? Is it > > enough for chains to be equivalent that the chain at the point of snapshot > > creation be a superset of the currently running chain? Probably yes, > > provided > > snapshots can't be inserted in the middle of a chain - but could that ever > > happen? > > The main problem is that the configuration may change from the time when > the snapshot was taken. Either the XML or the on disk setup. In case a > user issues a blockjob manually which merges parts of the chain you > won't be able to delete the snapshot with data reorganisation via the > API. Similarly to when the VM config changes to point to other images > and or remove or add disks. The snapshot code should be able to at least > properly reject the operation if it's unclear whether we can do the >
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Thu, Apr 16, 2020 at 17:07:35 +0200, Pavel Mores wrote: > On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote: > > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > > +} > > > > + > > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > > +topPath = disk->src->path; > > > > +else > > > > +topPath = snapdisk->src->path; > > > > > > You must not use paths for doing this lookup. Paths at best work for > > > local files only and this would make the code not future proof. > > > > > > Also you want to verify that: > > > - images you want to merge are actually in the backing chain > > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > > and plug a different chain back) > > > > Let me check with you how exactly to perform this verification. > > > > To retrieve the names of the disks included in a snapshot, I can use its > > virDomainSnapshotDef which contains an array of disks > > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify > > disks. > > > > To get the state of the chain at moment the snapshot was created, I can use > > the > > 'src' member and walk its 'backingStore' list to examine the whole chain. > > > > To get the currently running configuration, I assume I can use the names of > > the > > relevant disks (obtained from the snapshot as mentioned above) to get a > > virDomainDiskDefPtr for each of them, whose 'src' member points to a > > virStorageSource that represents the topmost image in the disk's chain. > > From > > there, I can walk the 'backingStore' list to get the other images in the > > chain > > all the way to the base image. > > > > The verification succeeds if the disk names and their chains in the snapshot > > and the running config correspond. Is the above correct? > > > > If so, I'm still not certain how to compare two virStorageSource's. How is > > identity of two different virStorageSource instances is established? > > After having a closer look into this I'm unfortunately even less clear as to > how to perform the checks mentioned in the review. Well unfortunately developing this is the main part why deleting snapshots is complicated. > In addition to the problem of establishing virStorageSource identity, a > similar > problem seems to come up with disks. They often seem to be identified and > referred to by the target name, however what happens if a snapshot is taken > for > 'vda', then the disk's target is changed to 'vdb' and now the snapshot is to > be In such case I'd consier it as being different. Similarly as we can't really guarantee that the image described by a virStorageSource is the same as it was when taking the snapshot. As long as the file is named the same you can consider it identical IMO. > deleted? Is there a way to find out that the disk is still there, only under > a > different name? No and it doesn't seem to be worth doing that. > > And as far as comparing chains goes - I know can retrieve the chain for a > running disk and I can retrieve what the chain looked like at the point a > snapshot was made. However, how do I establish they are equivalent? How can > I What I meant is that you need to establish that the images you are going to merge and the images you are going to merge into are the same described by the snapshots and at the same time present in the current backing chain of the disk. If they are not in the snapshot metadata you'd potentially merge something unwanted, but this is covered by the previous paragraphs. You need to also verify that they are currently opened by qemu as you can't do blockjobs on images which are not part of the chain. > tell that snapshots in both chains compare identical in the first place? Is > the snapshot name stable and persistent enough to be useful for this? Is it > enough for chains to be equivalent that the chain at the point of snapshot > creation be a superset of the currently running chain? Probably yes, provided > snapshots can't be inserted in the middle of a chain - but could that ever > happen? The main problem is that the configuration may change from the time when the snapshot was taken. Either the XML or the on disk setup. In case a user issues a blockjob manually which merges parts of the chain you won't be able to delete the snapshot with data reorganisation via the API. Similarly to when the VM config changes to point to other images and or remove or add disks. The snapshot code should be able to at least properly reject the operation if it's unclear whether we can do the right thing. Unfortunately all the above need to be considered and thought out before because there isn't really any prior art in libvirt that could be copied. There are helpers to compare two virStorageSource structs, but that's far from all of the required logic. While the initial
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote: > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > +} > > > + > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > +topPath = disk->src->path; > > > +else > > > +topPath = snapdisk->src->path; > > > > You must not use paths for doing this lookup. Paths at best work for > > local files only and this would make the code not future proof. > > > > Also you want to verify that: > > - images you want to merge are actually in the backing chain > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > and plug a different chain back) > > Let me check with you how exactly to perform this verification. > > To retrieve the names of the disks included in a snapshot, I can use its > virDomainSnapshotDef which contains an array of disks > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify > disks. > > To get the state of the chain at moment the snapshot was created, I can use > the > 'src' member and walk its 'backingStore' list to examine the whole chain. > > To get the currently running configuration, I assume I can use the names of > the > relevant disks (obtained from the snapshot as mentioned above) to get a > virDomainDiskDefPtr for each of them, whose 'src' member points to a > virStorageSource that represents the topmost image in the disk's chain. From > there, I can walk the 'backingStore' list to get the other images in the chain > all the way to the base image. > > The verification succeeds if the disk names and their chains in the snapshot > and the running config correspond. Is the above correct? > > If so, I'm still not certain how to compare two virStorageSource's. How is > identity of two different virStorageSource instances is established? After having a closer look into this I'm unfortunately even less clear as to how to perform the checks mentioned in the review. In addition to the problem of establishing virStorageSource identity, a similar problem seems to come up with disks. They often seem to be identified and referred to by the target name, however what happens if a snapshot is taken for 'vda', then the disk's target is changed to 'vdb' and now the snapshot is to be deleted? Is there a way to find out that the disk is still there, only under a different name? And as far as comparing chains goes - I know can retrieve the chain for a running disk and I can retrieve what the chain looked like at the point a snapshot was made. However, how do I establish they are equivalent? How can I tell that snapshots in both chains compare identical in the first place? Is the snapshot name stable and persistent enough to be useful for this? Is it enough for chains to be equivalent that the chain at the point of snapshot creation be a superset of the currently running chain? Probably yes, provided snapshots can't be inserted in the middle of a chain - but could that ever happen? Thanks in advance for any help with this! pvl
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > +} > > + > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > +topPath = disk->src->path; > > +else > > +topPath = snapdisk->src->path; > > You must not use paths for doing this lookup. Paths at best work for > local files only and this would make the code not future proof. > > Also you want to verify that: > - images you want to merge are actually in the backing chain > - the backing chain looks as snapshots describe it (e.g you unplug vda > and plug a different chain back) Let me check with you how exactly to perform this verification. To retrieve the names of the disks included in a snapshot, I can use its virDomainSnapshotDef which contains an array of disks (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify disks. To get the state of the chain at moment the snapshot was created, I can use the 'src' member and walk its 'backingStore' list to examine the whole chain. To get the currently running configuration, I assume I can use the names of the relevant disks (obtained from the snapshot as mentioned above) to get a virDomainDiskDefPtr for each of them, whose 'src' member points to a virStorageSource that represents the topmost image in the disk's chain. From there, I can walk the 'backingStore' list to get the other images in the chain all the way to the base image. The verification succeeds if the disk names and their chains in the snapshot and the running config correspond. Is the above correct? If so, I'm still not certain how to compare two virStorageSource's. How is identity of two different virStorageSource instances is established? Thanks! pvl
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Fri, Apr 03, 2020 at 02:36:35PM +0200, Peter Krempa wrote: > On Fri, Apr 03, 2020 at 14:34:29 +0200, Pavel Mores wrote: > > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > > > > > +} > > > > + > > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > > +topPath = disk->src->path; > > > > +else > > > > +topPath = snapdisk->src->path; > > > > > > You must not use paths for doing this lookup. Paths at best work for > > > local files only and this would make the code not future proof. > > > > > > Also you want to verify that: > > > - images you want to merge are actually in the backing chain > > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > > and plug a different chain back) > > > > > > Doing the validation above will necessarily give you a > > > virStorageSourcePtr for the appropriate member of the backing chain and > > > that one should be used as argument for the commit operation. > > > > I'm afraid I'm not following this. qemuDomainBlockCommitImpl(), just like > > qemuDomainBlockCommit() take paths so that's what I'm passing them. What do > > you mean, I should use virStorageSourcePtr as argument for the commit > > operation? > > I'm saying you should not use paths at all. Use virStorageSource > directly. If you look inside qemuDomainBlockCommitImpl there are calls > which take path and look up the virStorageSource. > > Specifically e.g. for NBD disks path may be NULL, thus must not be used > if your code has to work for everything. It's okay only to use them when > passed by user, but not internally. Alright, so you mean a further refactor of qemuDomainBlockCommitImpl() is needed. Thanks, I'll look into it. pvl
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > +} > > + > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > +topPath = disk->src->path; > > +else > > +topPath = snapdisk->src->path; > > You must not use paths for doing this lookup. Paths at best work for > local files only and this would make the code not future proof. > > Also you want to verify that: > - images you want to merge are actually in the backing chain > - the backing chain looks as snapshots describe it (e.g you unplug vda > and plug a different chain back) > > Doing the validation above will necessarily give you a > virStorageSourcePtr for the appropriate member of the backing chain and > that one should be used as argument for the commit operation. I'm afraid I'm not following this. qemuDomainBlockCommitImpl(), just like qemuDomainBlockCommit() take paths so that's what I'm passing them. What do you mean, I should use virStorageSourcePtr as argument for the commit operation? pvl
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Fri, Apr 03, 2020 at 14:34:29 +0200, Pavel Mores wrote: > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > > > +} > > > + > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > +topPath = disk->src->path; > > > +else > > > +topPath = snapdisk->src->path; > > > > You must not use paths for doing this lookup. Paths at best work for > > local files only and this would make the code not future proof. > > > > Also you want to verify that: > > - images you want to merge are actually in the backing chain > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > and plug a different chain back) > > > > Doing the validation above will necessarily give you a > > virStorageSourcePtr for the appropriate member of the backing chain and > > that one should be used as argument for the commit operation. > > I'm afraid I'm not following this. qemuDomainBlockCommitImpl(), just like > qemuDomainBlockCommit() take paths so that's what I'm passing them. What do > you mean, I should use virStorageSourcePtr as argument for the commit > operation? I'm saying you should not use paths at all. Use virStorageSource directly. If you look inside qemuDomainBlockCommitImpl there are calls which take path and look up the virStorageSource. Specifically e.g. for NBD disks path may be NULL, thus must not be used if your code has to work for everything. It's okay only to use them when passed by user, but not internally.
Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> Works by blockcommitting the snapshot to be deleted into its parent. Handles
> both deleting children as well and deleting children only (commits the
> children into the snapshot referred to by snapshot-delete argument). If the
> necessary block commit operation turns out to be active (the snapshot referred
> to by snapshot-delete argument is current, or deleting children is requested)
> pivot is performed as well.
>
> This implemetation is limited to the straightforward case which assumes no
> branching snapshot chains below the snapshot to be deleted, and the current
> snapshot has to be the leaf of the (linear) chain starting at the snapshot to
> be deleted. These requirements are checked and enforced at the top of
> qemuDomainSnapshotDeleteExternal(). They might even be too restrictive for
> the case where deletion of children is not requested as in that case,
> requiring that the parent of the snapshot to be deleted not be a branching
> point in snapshot tree should be enough.
>
> The above limitations do not appear to be too severe under the current state
> of matters where a major source of branching snapshot structures,
> snapshot-revert, is not even implemented for external snapshots yet. The only
> other known cause of branching in external snapshots thus seems to be if a
> user creates branching by manually creating snapshot images and metadata and
> applies them using snapshot-create --redefine. At any rate, this work should
> be understood as just a first step to a full support of deleting external
> snapshots.
>
> Signed-off-by: Pavel Mores
> ---
> src/qemu/qemu_driver.c | 149 +++--
> 1 file changed, 145 insertions(+), 4 deletions(-)
Few quick comments:
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b981745ecf..4d4f3f069f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16707,6 +16707,149 @@ qemuDomainMomentReparentChildren(void *payload,
> }
>
>
> +/*
> + * We can't use virDomainMomentFindLeaf() as it takes a
> + * virDomainMomentObjListPtr which we don't have. It might be a good idea to
> + * move this function to a library of virDomainMomentObj helpers, then
> + * reimplement virDomainMomentFindLeaf() in terms of this function as it only
> + * uses its virDomainMomentObjListPtr parameter to fish a
> virDomainMomentObjPtr
> + * out of it. Then it just runs this function's algorithm on it.
Please do so ...
> + */
> +static virDomainMomentObjPtr
> +myDomainMomentFindLeaf(virDomainMomentObjPtr moment)
... since this does not conform to the naming scheme.
> +{
> +if (moment->nchildren != 1)
> +return NULL;
> +while (moment->nchildren == 1)
> +moment = moment->first_child;
> +if (moment->nchildren == 0)
> +return moment;
> +return NULL;
> +}
> +
> +
> +static int
> +qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm,
> + virQEMUDriverPtr driver,
> + virDomainMomentObjPtr snap,
> + unsigned int flags)
> +{
> +int ret = -1;
> +bool isActive;
> +virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap);
> +virDomainMomentObjPtr leaf = snap->nchildren ?
> myDomainMomentFindLeaf(snap) : snap;
Please refrain from using ternary operators here.
> +virDomainMomentObjPtr parent = snap->parent;
> +virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent);
> +size_t i;
> +
> +/* This function only works if the chain below 'snap' is linear. If
> + * there's no unique leaf it means the chain of 'snap's children
> + * branches at some point. Also, if there *is* a leaf but it's not
> + * the current snapshot, bail out as well. */
> +if (leaf == NULL) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + "%s", _("can't delete, snapshot chain branches"));
> +goto cleanup;
> +}
> +if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) {
Btw, vm->snapshots.base is the 'virDomainMomentObjListPtr' you were
looking for above.
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + "%s", _("can't delete, leaf snapshot is not
> current"));
> +goto cleanup;
> +}
Check that VM is active should be added here, so that we can bail out
sooner than when we try to actually commit.
> +
> +if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
> +isActive = true;
> +} else {
> +isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots);
> +}
> +
> +for (i = 0; i < snapdef->ndisks; i++) {
> +virDomainSnapshotDiskDefPtr snapdisk = &(snapdef->disks[i]);
> +const char *diskName = snapdisk->name;
> +const char *basePath = NULL;
> +const
[libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases
Works by blockcommitting the snapshot to be deleted into its parent. Handles
both deleting children as well and deleting children only (commits the
children into the snapshot referred to by snapshot-delete argument). If the
necessary block commit operation turns out to be active (the snapshot referred
to by snapshot-delete argument is current, or deleting children is requested)
pivot is performed as well.
This implemetation is limited to the straightforward case which assumes no
branching snapshot chains below the snapshot to be deleted, and the current
snapshot has to be the leaf of the (linear) chain starting at the snapshot to
be deleted. These requirements are checked and enforced at the top of
qemuDomainSnapshotDeleteExternal(). They might even be too restrictive for
the case where deletion of children is not requested as in that case,
requiring that the parent of the snapshot to be deleted not be a branching
point in snapshot tree should be enough.
The above limitations do not appear to be too severe under the current state
of matters where a major source of branching snapshot structures,
snapshot-revert, is not even implemented for external snapshots yet. The only
other known cause of branching in external snapshots thus seems to be if a
user creates branching by manually creating snapshot images and metadata and
applies them using snapshot-create --redefine. At any rate, this work should
be understood as just a first step to a full support of deleting external
snapshots.
Signed-off-by: Pavel Mores
---
src/qemu/qemu_driver.c | 149 +++--
1 file changed, 145 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b981745ecf..4d4f3f069f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16707,6 +16707,149 @@ qemuDomainMomentReparentChildren(void *payload,
}
+/*
+ * We can't use virDomainMomentFindLeaf() as it takes a
+ * virDomainMomentObjListPtr which we don't have. It might be a good idea to
+ * move this function to a library of virDomainMomentObj helpers, then
+ * reimplement virDomainMomentFindLeaf() in terms of this function as it only
+ * uses its virDomainMomentObjListPtr parameter to fish a virDomainMomentObjPtr
+ * out of it. Then it just runs this function's algorithm on it.
+ */
+static virDomainMomentObjPtr
+myDomainMomentFindLeaf(virDomainMomentObjPtr moment)
+{
+if (moment->nchildren != 1)
+return NULL;
+while (moment->nchildren == 1)
+moment = moment->first_child;
+if (moment->nchildren == 0)
+return moment;
+return NULL;
+}
+
+
+static int
+qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm,
+ virQEMUDriverPtr driver,
+ virDomainMomentObjPtr snap,
+ unsigned int flags)
+{
+int ret = -1;
+bool isActive;
+virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap);
+virDomainMomentObjPtr leaf = snap->nchildren ?
myDomainMomentFindLeaf(snap) : snap;
+virDomainMomentObjPtr parent = snap->parent;
+virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent);
+size_t i;
+
+/* This function only works if the chain below 'snap' is linear. If
+ * there's no unique leaf it means the chain of 'snap's children
+ * branches at some point. Also, if there *is* a leaf but it's not
+ * the current snapshot, bail out as well. */
+if (leaf == NULL) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ "%s", _("can't delete, snapshot chain branches"));
+goto cleanup;
+}
+if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ "%s", _("can't delete, leaf snapshot is not current"));
+goto cleanup;
+}
+
+if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
+ VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
+isActive = true;
+} else {
+isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots);
+}
+
+for (i = 0; i < snapdef->ndisks; i++) {
+virDomainSnapshotDiskDefPtr snapdisk = &(snapdef->disks[i]);
+const char *diskName = snapdisk->name;
+const char *basePath = NULL;
+const char *topPath = snapdisk->src->path;
+int blockCommitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE;
+virDomainDiskDefPtr disk;
+
+if (!(disk = qemuDomainDiskByName(vm->def, diskName)))
+goto cleanup;
+
+if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
+basePath = snapdisk->src->path;
+topPath = disk->src->path;
+} else {
+if (parent->nchildren == 1) {
+if (parentdef == NULL) {
+virStorageSourcePtr baseSrc =
virStorageFileChainLookup(disk->src, NULL, NULL, 0, NULL);
+if
