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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-28 at 11:46, Stefan Hajnoczi wrote:
> On Thu, Nov 27, 2014 at 04:11:12PM +0100, Max Reitz wrote:
>> On 2014-11-27 at 16:09, Stefan Hajnoczi wrote:
>>> On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote:
>>>> +/**
>>>> + * Reallocates *array so that it can hold new_size entries. *size must contain
>>>> + * the current number of entries in *array. If the reallocation fails, *array
>>>> + * and *size will not be modified and -errno will be returned. If the
>>>> + * reallocation is successful, *array will be set to the new buffer and *size
>>>> + * will be set to new_size. The size of the reallocated refcount array buffer
>>>> + * will be aligned to a cluster boundary, and the newly allocated area will be
>>>> + * zeroed.
>>>> + */
>>>> +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;
>>>> +    }
>>> Why not realloc the array to the new smaller size? ...
>> Because such a call will actually never happen. I could replace this if ()
>> by assert(new_byte_size >= old_byte_size); if (new_byte_size ==
>> old_byte_size), but as I said before, I'm not a friend of assertions when
>> the code can deal perfectly well with the "unsupported" case.
> It is simpler to put an if statement around the memset.

Well, I personally find an assertion simpler; and I will not drop the if 
(new_byte_size == old_byte_size) because this is a very common case so I 
don't want to rely on g_realloc() to detect it. Also, Eric proposed it 
and I'd like to avoid a ping-pong discussion.

> Then the function actually frees unused memory

Which will never happen, though, because new_byte_size will never be 
less than old_byte_size.

> and readers don't wonder why you decided not to shrink the array.

An assertion will clear up things as well.

> Less code and slightly better behavior.

Well, an assert() is one line, while an if () takes two (if () { and }); 
the behavior will (hopefully) not change either.

But anyway, I'll go with your proposition because, as I said, I don't 
like assertions if the code can deal perfectly fine with the bad cases. 
Therefore, if at some point in time we decide to use 
realloc_refcount_array() to shrink a refcount array, it's better to have 
planned for that case.

Max