From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjwov-0004G2-R3 for qemu-devel@nongnu.org; Mon, 12 May 2014 16:27:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wjwop-0006b0-Pq for qemu-devel@nongnu.org; Mon, 12 May 2014 16:27:33 -0400 Received: from mail-qc0-f180.google.com ([209.85.216.180]:58088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjwop-0006av-Lk for qemu-devel@nongnu.org; Mon, 12 May 2014 16:27:27 -0400 Received: by mail-qc0-f180.google.com with SMTP id i17so8655535qcy.11 for ; Mon, 12 May 2014 13:27:27 -0700 (PDT) From: Mike Day Date: Mon, 12 May 2014 16:27:18 -0400 Message-Id: <1399926438-32292-1-git-send-email-ncmike@ncultra.org> Subject: [Qemu-devel] [PATCH v2] 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. 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 Reviewed-by: Eric Blake --- 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