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

[Kevin Wolf <kwolf@suse.de>]

Laurent Vivier schrieb:
> Le mercredi 06 août 2008 à 16:20 +0200, Kevin Wolf a écrit :
>> 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).
> 
> It frees the cluster if it is found except if it has the flag
> QCOW_OFLAG_COPIED. To do that it needs to load the L2 table.
> 
> But I'm open to a new name, I don't like this one too.

Well, yes and no. Of course, it needs to have the L2 table to free the
cluster. But it does not only load that table for local usage but
returns it to the caller using the parameters passed by reference. So
this loading doesn't only happen internally but it belongs to the interface.

Actually, I'm also having a hard time finding a fitting name for the
function. Maybe that's an indication that the function is doing two
things which don't really belong together. I'm not sure if it makes
sense, but you could do the L2 table loading in another function which
is called before free_used_clusters() and pass the already loaded table
into free_used_clusters().

>> 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.
> 
> Yes, but this is the original behavior and I don't want to modify it.
> 
>> Maybe you meant to return cluster_offset instead of 0 at the end of the
> 
> No, at the end of function the cluster has been freed and the offset is
> not valid anymore.

Yes, you have a point there...

>> 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.
> 
> You're right. But to return the offset with the flag allows to
> differentiate the case returning 0 from an offset equal to 0.
> If offset is 0, it returns (0 | QCOW_OFLAG_COPIED), but I agree to say
> that offset cannot be 0, but again I keep the original behavior.

I fully agree if you want to keep the original behaviour in the external
interface. However, free_used_clusters() is only used in your own
functions, so you could actually make that change without a too big risk.

But the most important part is really the documentation. You see how I
misunderstood what you were trying to achieve. This could be easily avoided.

Kevin