[libvirt PATCH 21/30] qemu_snapshot: error out when deleting internal snapshot on non-active disk

[Peter Krempa]

On Thu, Dec 08, 2022 at 14:30:57 +0100, Pavel Hrdina wrote:
> Deleting internal snapshot when the currently active disk image is
> different then where the internal snapshot was taken doesn't work

than

> correctly.
> 
> This applies to a running VM only as we are using QMP command and
> talking to the QEMU process that is using different disk.
> 
> This works correctly when the VM is shut of as in this case we spawn
> qemu-img process to delete the snapshot.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index a4b45d3ba3..adcd4eb73a 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2495,9 +2495,30 @@ qemuSnapshotCountExternalInternal(void *payload,
>  
>  
>  static int
> -qemuSnapshotDeleteValidate(virDomainMomentObj *snap,
> +qemuSnapshotDeleteValidate(virDomainObj *vm,
> +                           virDomainMomentObj *snap,
>                             unsigned int flags)
>  {
> +    if (!virDomainSnapshotIsExternal(snap) &&
> +        virDomainObjIsActive(vm)) {
> +        ssize_t i;
> +        virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> +
> +        for (i = 0; i < snapdef->ndisks; i++) {
> +            virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> +            virDomainDiskDef *vmdisk = NULL;
> +            virDomainDiskDef *disk = NULL;
> +
> +            vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> +            disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
> +
> +            if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("deleting internal snapshot for non-active disk is not supported"));

This error is a bit misleading or at least doesn't explain very well
what the actual problem is. I think I'd go with:

"disk image for internal snapshot '%s' is not the same as when the
snapshot was taken"

Or something similar. This is such that in case when the user moved the
disk image and forgot to modify the snapshot XML the error is still
actionable.

With a better error:

Reviewed-by: Peter Krempa <pkrempa at redhat.com>