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

[Pavel Hrdina]

On Tue, Dec 13, 2022 at 03:26:52PM +0100, Peter Krempa wrote:
> 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.

I get the point but to me it feels it still doesn't describe the issue
properly.

Take this situation, you have VM, first snapshot is internal and second
one is external and active and the VM is running. In this situation you
are not able to delete the internal snapshot because the HMP commands
will not work properly but will not produce any error as well.

VM
  |
  +- snap1 (internal)
     |
     + snap2 (external)(active)

So the triggers only if the snapshot that we are trying to delete is
internal, the VM is running and the currently active disk in VM is
different than where the internal snapshot was taken.

How about:

"disk image '%s' for internal snapshot '%s' is not the same as disk
image currently used by VM"

Pavel

> With a better error:
> 
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20221214/e73ed7c7/attachment-0001.sig>