Similarly to 510d154a0b41aa70aadabc0918d16dee22882394 we need to prevent
doing too deeply nested backing chains and reject them with a sane error
message.

Add a loop to go through the snapshots prior to attempting actually
creating them to prevent some possible inconsistent scenarios.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_driver.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2f66d7cd9a..4cebb54913 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14618,6 +14618,16 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
     if (!(created = virBitmapNew(snapdef->ndisks)))
         goto cleanup;

+    for (i = 0; i < snapdef->ndisks && !reuse; i++) {
+        snapdisk = &(snapdef->disks[i]);
+        defdisk = snapdef->parent.dom->disks[snapdisk->idx];
+        if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+            continue;
+
+        if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0)
+            return -1;
+    }
+
     /* If reuse is true, then qemuDomainSnapshotPrepare already
      * ensured that the new files exist, and it was up to the user to
      * create them correctly.  */
-- 
2.24.1

On Mon, Jan 20, 2020 at 16:12:06 +0100, Peter Krempa wrote:
> Similarly to 510d154a0b41aa70aadabc0918d16dee22882394 we need to prevent
> doing too deeply nested backing chains and reject them with a sane error
> message.
> 
> Add a loop to go through the snapshots prior to attempting actually
> creating them to prevent some possible inconsistent scenarios.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2f66d7cd9a..4cebb54913 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14618,6 +14618,16 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
>      if (!(created = virBitmapNew(snapdef->ndisks)))
>          goto cleanup;
> 
> +    for (i = 0; i < snapdef->ndisks && !reuse; i++) {
> +        snapdisk = &(snapdef->disks[i]);
> +        defdisk = snapdef->parent.dom->disks[snapdisk->idx];
> +        if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +            continue;
> +
> +        if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0)
> +            return -1;
> +    }
> +

I think this new loop should be put below the following comment and the
!reuse check should be moved out of the loop condition for better
visibility. And similar change should be made to the existing loop.

>      /* If reuse is true, then qemuDomainSnapshotPrepare already
>       * ensured that the new files exist, and it was up to the user to
>       * create them correctly.  */

In other words, the code following this comment would be something like
the following:

       if (!reuse) {
           for (i = 0; i < snapdef->ndisks; i++) {
               // check depth
           }

           for (i = 0; i < snapdef->ndisks; i++) {
               // create overlay images
           }
       }

Jirka