Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Laurent Vivier]

Le vendredi 07 novembre 2008 à 11:21 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
[...]
> >>>  
> >>> @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
> >>>                              int addend)
> >>>  {
> >>>      BDRVQcowState *s = bs->opaque;
> >>> -    int64_t start, last, cluster_offset;
> >>> +    int64_t start, last;
> >>>  
> >>>  #ifdef DEBUG_ALLOC2
> >>>      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> >>> @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
> >>>  #endif
> >>>      if (length <= 0)
> >>>          return;
> >>> -    start = offset & ~(s->cluster_size - 1);
> >>> -    last = (offset + length - 1) & ~(s->cluster_size - 1);
> >>> -    for(cluster_offset = start; cluster_offset <= last;
> >>> -        cluster_offset += s->cluster_size) {
> >>> -        update_cluster_refcount(bs, cluster_offset >> s->cluster_bits, 
> >>> addend);
> >>> -    }
> >>> +    start = offset >> s->cluster_bits;
> >>> +    last = (offset + length) >> s->cluster_bits;
> >> Off by one for length % cluster_size == 0?
> > 
> > Explain, please (but notice the "<=" in the "for (...)").
> 
> I didn't even look at the old code yesterday, but the difference is that
> you dropped the - 1 for last.
> 
> Let's do it by example: Assume cluster_size = 0x1000 for simplicity and
> we get offset = 0x1000 and length = 0x2000 (i.e. the last affected byte
> would be 0x2fff). The old code correctly produces start = 0x1000 and
> last = 0x2000 whereas you get start = 1 and last = 3.

Yes, "last" must be "(offset + length - 1) >> s->cluster_bits"

Thank you,
Laurent
-- 
------------------ address@hidden  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard