On Fri, Sep 01, 2023 at 11:10:43AM +0200, Peter Krempa wrote:
On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote:
> We need to skip all disks that have snapshot type other than 'external'.
Since the commit message is vague on the specific problem details ...
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_snapshot.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index d943281e35..ff85d56572 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
> virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i];
> virDomainDiskDef *domdisk = domdef->disks[i];
>
> + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> + continue;
... I remember you talking about the need to create overlays also for
images which might have been excluded in given snapshot (the one you are
reverting to) , but externally snapshotted in a later (still existing)
one.
In such case not creating the overlay here would still invalidate the
overlay of the next snapshot. Is that right?
I also remember that you mentioned that the actual problem is with empty
cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?
That is done in virDomainSnapshotAlignDisks() called right before the
for loop in qemuSnapshotRevertExternalPrepare(), there we fill in the
temporary snapshot definition with new overlays. From that moment the
temporary snapshot definition contains all disks based on the VM
definition.
Looking at the code, when reverting to a snapshot we will always create
new overlays for all disks including readonly disks, not sure if that is
what we should do or no.
Pavel