[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2] qemu-img fails to delete last snapshot
Date: Thu, 15 May 2014 01:54:05 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

On 12.05.2014 22:27, 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.

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.

Before this patch is applied, s->nb_snapshots is only increased after the memmove(). Therefore, it can never be 0 – snapshot_index on the other hand needs to be 0, as find_snapshot_by_id_and_name() forces it to be less than s->nb_snapshots (to elaborate on Kevin's review).

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

I do agree that this code is rather ugly and I had problems with it on more than one occasion (which should be speaking for itself, considering that I have not worked that long on qemu). On the other hand it is always a nice test case whether one broke zero-size allocations, though (while I'm not sure whether these should be allowed in the first place, though…).

Considering that this code indeed does perform a zero-size allocation reproducably, I'm rather surprised that we actually do not have a test case yet for snapshot deletion, though (as far as I can see).

So, all in all, I am kind of in favor of making the deletion of the last snapshot a special case as this would probably greatly improve readability; but on the other hand, it actually is a good test as it is right now.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]