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

[Max Reitz]

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.

Max