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

[Laurent Vivier <Laurent.Vivier@bull.net>]

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.

> 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.

> 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.

> 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.

I agree. Could you propose a name ?

Regards,
Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry