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 <ncm...@ncultra.org>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
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.
Max
