From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WimKQ-00059P-NW for qemu-devel@nongnu.org; Fri, 09 May 2014 11:03:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WimKK-0004Xv-Rr for qemu-devel@nongnu.org; Fri, 09 May 2014 11:03:14 -0400 Received: from mail-qc0-f182.google.com ([209.85.216.182]:41975) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WimKK-0004XT-Nz for qemu-devel@nongnu.org; Fri, 09 May 2014 11:03:08 -0400 Received: by mail-qc0-f182.google.com with SMTP id e16so4725276qcx.27 for ; Fri, 09 May 2014 08:03:07 -0700 (PDT) From: Mike Day Date: Fri, 9 May 2014 11:02:52 -0400 Message-Id: <1399647772-4402-1-git-send-email-ncmike@ncultra.org> Subject: [Qemu-devel] [PATCH] qemu-img fails to delete last snapshot List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, Mike Day , stefanha@redhat.com 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 --- 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