Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part from alloc_cluster_offset()

[Kevin Wolf <kwolf@suse.de>]

Laurent Vivier schrieb:
> Divide alloc_cluster_offset() into alloc_cluster_offset() and
> alloc_compressed_cluster_offset(). Factorize code to free clusters into
> free_used_clusters().
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>

I think free_used_clusters() is misnamed. That it frees clusters is more
of an additional action it performs. What it really is doing is to load
the appropriate L2 table into memory (and to allocate it if needed).

Also, the current function name doesn't provide a hint how the return
value is to be interpreted. If I'm not mistaken, 0 could mean that
everything went fine and the cluster has been freed, or it could mean
that an error occurred.

Maybe you meant to return cluster_offset instead of 0 at the end of the
function (that you check for QCOW_OFLAG_COPIED which is _always_ set if
the function returns != 0 suggests this), but I can't tell for sure
because there is no documentation on the return value.

So I suggest that, besides renaming and possibly fixing, you add a
header comment to the function which describes the whole functionality
it performs (it's too much to fit in a function name) and what the
return value actually means.

Kevin