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

[Qemu-devel] [PATCH] 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.

Signed-off-by: Mike Day <ncmike@ncultra.org>
---

There are two specific problems in the curent code. First is a lack of
parenthesis in the calculation of a memmove 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.

 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On 05/09/2014 09:02 AM, Mike Day wrote:
> 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.
> 
> Signed-off-by: Mike Day <ncmike@ncultra.org>
> ---
> 
> There are two specific problems in the curent code. First is a lack of

s/curent/current/

> parenthesis in the calculation of a memmove 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.

This information is actually quite useful in understanding the patch,
and I would have included it prior to the --- for inclusion in git,
rather than in the reviewer-only commentary that gets stripped during
'git am'.

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

I'd suggest posting a v2 with a better commit message; but the code
itself seems fine, so you can add:

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Mike Day]

On Mon, May 12, 2014 at 12:39 PM, Eric Blake <eblake@redhat.com> wrote:
> This information is actually quite useful in understanding the patch,
> and I would have included it prior to the --- for inclusion in git,
> rather than in the reviewer-only commentary that gets stripped during
> 'git am'.
>
>>
>>  block/qcow2-snapshot.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> I'd suggest posting a v2 with a better commit message; but the code
> itself seems fine, so you can add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Got it, thanks!

Mike