[Qemu-devel] [PATCH v2] qemu-img fails to delete last snapshot

[Mike Day]

When deleting the last snapshot, copying the resulting snapshot table
currently fails, causing the delete operation to also fail. Fix the
failure by skipping the copy and just writing the snapshot header and
freeing the extra clusters.

There are two specific problems in the current code. First is a lack of
parenthesis in the calculation of the memmove size parameter:

s->nb_snapshots - snapshot_index - 1

When s->nb_snapshots is 0, snapshot_index is 1.

0 - 1 - 1 = 0xfffffffe

it should be:

0 - (1 - 1) = 0x00

The second problem is shifting the snapshot table to the left. After
removing the last snapshot there are no existing snapshots to be
shifted. All that needs to be done is to write the header and
unallocate the blocks.

Signed-off-by: Mike Day <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
---
v2: improved the git log entry
    added Eric Blake as a reviewer

 block/qcow2-snapshot.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0aa9def..c8b842c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -165,9 +165,11 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
     assert(offset <= INT_MAX);
     snapshots_size = offset;
-
     /* Allocate space for the new snapshot list */
-    snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
+    snapshots_offset = 0;
+    if (snapshots_size) {
+        snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
+    }
     offset = snapshots_offset;
     if (offset < 0) {
         ret = offset;
@@ -180,12 +182,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
     /* The snapshot list position has not yet been updated, so these clusters
      * must indeed be completely free */
-    ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size);
-    if (ret < 0) {
-        goto fail;
+    if (snapshots_size) {
+        ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
-
     /* Write all snapshots to the new list */
     for(i = 0; i < s->nb_snapshots; i++) {
         sn = s->snapshots + i;
@@ -590,12 +593,14 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
         return -ENOENT;
     }
     sn = s->snapshots[snapshot_index];
-
     /* Remove it from the snapshot list */
-    memmove(s->snapshots + snapshot_index,
-            s->snapshots + snapshot_index + 1,
-            (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
     s->nb_snapshots--;
+    if (s->nb_snapshots) {
+        memmove(s->snapshots + snapshot_index,
+                s->snapshots + snapshot_index + 1,
+                (s->nb_snapshots - (snapshot_index - 1)) * sizeof(sn));
+    }
+
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
         error_setg_errno(errp, -ret,
-- 
1.9.0