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

[Max Reitz]

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