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

[Max Reitz]

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 <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> 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 <address@hidden>

Phew :-)

Thanks!

Max