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

[Max Reitz]

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 <address@hidden>
> > ---
> > 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