Re: [Qemu-devel] [PATCH v2] qcow2: Rewrite qcow2_alloc_bytes()

[Max Reitz <mreitz@redhat.com>]

On 2015-02-06 at 09:08, Kevin Wolf wrote:
> Am 05.02.2015 um 16:58 hat Max Reitz geschrieben:
>> qcow2_alloc_bytes() is a function with insufficient error handling and
>> an unnecessary goto. This patch rewrites it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> v2:
>> - s/free_cluster_index/free_byte_index/ [Eric]
>> - added an assertion at the start of the function that
>>    s->free_byte_offset is either 0 or points to the tail of a cluster
>>    (but never to the start)
>> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
>> - added an assertion that s->free_byte_offset is set before using it
>>    [Eric]
>> ---
>>   block/qcow2-refcount.c | 77 +++++++++++++++++++++++++++++---------------------
>>   1 file changed, 45 insertions(+), 32 deletions(-)
>>

[snip]

> The patch looks correct to me. Let me know if you'd like to address the
> point I made above, or if I should apply it as it is.

Good question. On one hand, I like your suggestion because it would 
indeed make the code shorter. On the other hand, I somehow feel better 
using functions that are prefixed qcow2_* because I feel like it might 
make the code harder to read if sometimes we use qcow2_alloc_clusters() 
and alloc_clusters_noref() at other times; and sometimes we use 
qcow2_update_cluster_refcount() and update_refcount() at other times.

But then again, it might make sense to have this function mirror 
qcow2_alloc_clusters(), which does use alloc_clusters_noref() and 
update_refcount() (of course).

So I think I'll go with your suggestion, thanks!

Max