If a disk is unplugged and then the user tries to delete a checkpoint
the code would try to use NULL node name as it was not checked.

Fix this by fetching the whole disk definition object and verifying it
was found.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 03a8321135..326707e098 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -125,12 +125,15 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,

     for (i = 0; i < chkdef->ndisks; i++) {
         virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i];
-        const char *node;
+        virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name);
+
+        /* domdisk can be missing e.g. when it was unplugged */
+        if (!domdisk)
+            continue;

         if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
             continue;

-        node = qemuDomainDiskNodeFormatLookup(vm, chkdisk->name);
         /* If any ancestor checkpoint has a bitmap for the same
          * disk, then this bitmap must be merged to the
          * ancestor. */
@@ -153,20 +156,28 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
                 if (!(arr = virJSONValueNewArray()))
                     return -1;

-                if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, chkdisk->bitmap) < 0)
+                if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr,
+                                                                     domdisk->src->nodeformat,
+                                                                     chkdisk->bitmap) < 0)
                     return -1;

                 if (chkcurrent) {
-                    if (qemuMonitorTransactionBitmapEnable(actions, node, disk2->bitmap) < 0)
+                    if (qemuMonitorTransactionBitmapEnable(actions,
+                                                           domdisk->src->nodeformat,
+                                                           disk2->bitmap) < 0)
                         return -1;
                 }

-                if (qemuMonitorTransactionBitmapMerge(actions, node, disk2->bitmap, &arr) < 0)
+                if (qemuMonitorTransactionBitmapMerge(actions,
+                                                      domdisk->src->nodeformat,
+                                                      disk2->bitmap, &arr) < 0)
                     return -1;
             }
         }

-        if (qemuMonitorTransactionBitmapRemove(actions, node, chkdisk->bitmap) < 0)
+        if (qemuMonitorTransactionBitmapRemove(actions,
+                                               domdisk->src->nodeformat,
+                                               chkdisk->bitmap) < 0)
             return -1;
     }

-- 
2.24.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 1/9/20 12:21 PM, Peter Krempa wrote:
> If a disk is unplugged and then the user tries to delete a checkpoint
> the code would try to use NULL node name as it was not checked.
> 
> Fix this by fetching the whole disk definition object and verifying it
> was found.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

But it also makes me wonder if the act of hot-unplug should update the 
definition of existing checkpoints. (Doesn't stop this patch from being 
useful as-is, but may point to further design work and future patches)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Fri, Jan 24, 2020 at 12:47:41 -0600, Eric Blake wrote:
> On 1/9/20 12:21 PM, Peter Krempa wrote:
> > If a disk is unplugged and then the user tries to delete a checkpoint
> > the code would try to use NULL node name as it was not checked.
> > 
> > Fix this by fetching the whole disk definition object and verifying it
> > was found.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >   src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------
> >   1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> But it also makes me wonder if the act of hot-unplug should update the
> definition of existing checkpoints. (Doesn't stop this patch from being
> useful as-is, but may point to further design work and future patches)

It can't since we chose to not version checkpoints on snapshots. Thus if
you've taken a checkpoint and then a snapshot and want to detach the
disk, reverting to the checkpoint will add the disk back and the
checkpoint must stay valid. As the checkpoint is not versioned we
wouldn't have the data to add it back.