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

[Laurent Vivier]

Le mercredi 06 août 2008 à 16:56 +0200, Kevin Wolf a écrit :
> 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 <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.
> 
> 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.

I agree.

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

I agree.

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

I'll try. No promise...

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

OK.

Thank you for your comments,
Laurent
-- 
----------------- address@hidden  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry