[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part from alloc_clu
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part from alloc_cluster_offset() |
Date: |
Wed, 06 Aug 2008 16:41:22 +0200 |
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 <address@hidden>
>
> 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
--
----------------- address@hidden ------------------
"La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry