Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation

[Max Reitz <mreitz@redhat.com>]

On 2014-11-20 at 22:43, Eric Blake wrote:
> On 11/20/2014 10:06 AM, Max Reitz wrote:
>> Add a helper function for reallocating a refcount array, independent of
>> the refcount order. The newly allocated space is zeroed and the function
>> handles failed reallocations gracefully.
>>
>> The helper function will always align the buffer size to a cluster
>> boundary; if storing the refcounts in such an array in big endian byte
>> order, this makes it possible to write parts of the array directly as
>> refcount blocks into the image file.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Perhaps the changes since v2 warranted removing my earlier R-b to make
> sure I review closely?

Well, normally I would not have taken the R-b. But you explicitly wrote:

> Code looks correct as written, whether or not you also add more
> comments, asserts, and/or shortcuts for no-op situations.

So I took that to mean "You may change the commit message as proposed 
(s/independently/independent/ and add a note about the cluster 
alignment), add a comment to realloc_refcount_array() about its calling 
contract, an assert(new_byte_size > 0) and/or an early-out for when the 
byte size of the refcount array does not increase", which is pretty 
broad, but that's what you wrote. That's why I kept the R-b in.

>> ---
>>   block/qcow2-refcount.c | 135 +++++++++++++++++++++++++++++++------------------
>>   1 file changed, 86 insertions(+), 49 deletions(-)
>>
>> +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
>> +                                  int64_t *size, int64_t new_size)
>> +{
>> +    /* Round to clusters so the array can be directly written to disk */
>> +    size_t old_byte_size = ROUND_UP(refcount_array_byte_size(s, *size),
>> +                                    s->cluster_size);
>> +    size_t new_byte_size = ROUND_UP(refcount_array_byte_size(s, new_size),
>> +                                    s->cluster_size);
>> +    uint16_t *new_ptr;
>> +
>> +    if (new_byte_size <= old_byte_size) {
>> +        *size = new_size;
>> +        return 0;
>> +    }
>> +
>> +    assert(new_byte_size > 0);
> Should this assert be moved before the early exit?

Could do, but it should not matter. It's important that new_byte_size > 
0 after the early-out; the caller should not use new_size == 0 at all, 
but I'd rather not add an assertion against a contract breach somewhere 
where the code can handle that breach actually just fine.

> Hmm, that's my only finding, and you did incorporate improvements mostly
> to comments or asserts as compared to v2.  Moving the assert is small
> enough, and not a show-stopper if you leave it in place, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Phew :-)

Thanks!

Max