Re: [Qemu-devel] [PATCH v2 07/21] qcow2: Helper function for refcount modification

[Max Reitz]

On 2014-11-15 at 18:02, Eric Blake wrote:
> On 11/14/2014 06:06 AM, Max Reitz wrote:
> > Since refcounts do not always have to be a uint16_t, all refcount blocks
> > and arrays in memory should not have a specific type (thus they become
> > pointers to void) and for accessing them, two helper functions are used
> > (a getter and a setter). Those functions are called indirectly through
> > function pointers in the BDRVQcowState so they may later be exchanged
> > for different refcount orders.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/qcow2-refcount.c | 128 ++++++++++++++++++++++++++++++-------------------
> >   block/qcow2.h          |   8 ++++
> >   2 files changed, 87 insertions(+), 49 deletions(-)
> >
> > @@ -1216,7 +1249,7 @@ enum {
> >    * error occurred.
> >    */
> >   static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
> > -    uint16_t **refcount_table, int64_t *refcount_table_size, int64_t l2_offset,
> > +    void **refcount_table, int64_t *refcount_table_size, int64_t l2_offset,
> >       int flags)
> >   {
> Might be worth fixing the indentation here in addition to all the other
> places you adjusted.  But that's minor.
>
> > @@ -1933,17 +1967,13 @@ write_refblocks:
> >               goto fail;
> >           }
> >   
> > -        on_disk_refblock = qemu_blockalign0(bs->file, s->cluster_size);
> > -        for (i = 0; i < s->refcount_block_size &&
> > -                    refblock_start + i < *nb_clusters; i++)
> > -        {
> > -            on_disk_refblock[i] =
> > -                cpu_to_be16((*refcount_table)[refblock_start + i]);
> > -        }
> > +        /* The size of *refcount_table is always cluster-aligned, therefore the
> > +         * write operation will not overflow */
> > +        on_disk_refblock = (void *)((uintptr_t)*refcount_table +
> > +                                    (refblock_index << 
> > s->refcount_block_bits));
> Here is where you are relying on the guarantee that you added in 6/21,
> which is why I ask for that one to mention it.
>
> Nice reduction of a bounce buffer, by the way :)  Worth mentioning in
> the commit message as an intentional part of this commit?

Why not.

> > @@ -2087,7 +2117,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> > BdrvCheckResult *res,
> >           /* Because the old reftable has been exchanged for a new one the
> >            * references have to be recalculated */
> >           rebuild = false;
> > -        memset(refcount_table, 0, nb_clusters * sizeof(uint16_t));
> > +        memset(refcount_table, 0, nb_clusters * s->refcount_bits / 8);
> Phew; we're safe that this won't overflow; and good that you do the *
> first (if you did the /8 first, it would fail for sub-byte refcounts).

Thanks for catching this, it is wrong (albeit it does the right thing). 
It should use refcount_array_byte_size(), which was in this version of 
the series introduced before this patch, so it's an artifact of swapping 
patch 6 and 7.

Max

> Reviewed-by: Eric Blake <address@hidden>