Re: [Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Kevin Wolf]

Laurent Vivier schrieb:
> Le jeudi 06 novembre 2008 à 18:32 +0100, Kevin Wolf a écrit :
>> Laurent Vivier schrieb:
>>> pièce jointe document texte brut
>>> (0001-Clean-up-update_cluster_refcount.patch)
>>> Move some parts to alloc_refcount_block() and block cache
>>>  checking to load_refcount_block().
>>>
>>> Signed-off-by: Laurent Vivier <address@hidden>
>>> ---
>>>  block-qcow2.c |   71 
>>> +++++++++++++++++++++++++++++++++++-----------------------
>>>  1 file changed, 43 insertions(+), 28 deletions(-)
>>>
>>> Index: qemu/block-qcow2.c
>>> ===================================================================
>>> --- qemu.orig/block-qcow2.c 2008-11-06 16:34:46.000000000 +0100
>>> +++ qemu/block-qcow2.c      2008-11-06 16:40:27.000000000 +0100
>>> @@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
>>>  {
>>>      BDRVQcowState *s = bs->opaque;
>>>      int ret;
>>> +
>>> +    if (refcount_block_offset == s->refcount_block_cache_offset)
>>> +        return 0;
>>> +
>>>      ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
>>>                       s->cluster_size);
>>>      if (ret != s->cluster_size)
>>> @@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
>>>      refcount_block_offset = s->refcount_table[refcount_table_index];
>>>      if (!refcount_block_offset)
>>>          return 0;
>>> -    if (refcount_block_offset != s->refcount_block_cache_offset) {
>>> -        /* better than nothing: return allocated if read error */
>>> -        if (load_refcount_block(bs, refcount_block_offset) < 0)
>>> -            return 1;
>>> -    }
>>> +    if (load_refcount_block(bs, refcount_block_offset) < 0)
>>> +        return 1;
>> Please retain the comment saying that 1 is not the correct value but we
>> default to allocated for safety if the block could not be read. I don't
>> think this is obvious.
> 
> I totally agree with you, but it's just a code move, I don't want to
> change anything except the line numbers.
> 
> I think this kind of change must have its own patch.

No, it's a move which drops a comment and it shouldn't do that. ;-)

> -        /* better than nothing: return allocated if read error */

Kevin