Re: [Qemu-devel] [PATCH v7 10/13] qcow2: Rebuild refcount structure during check

[Max Reitz <mreitz@redhat.com>]

On 2014-10-22 at 11:14, Kevin Wolf wrote:
> Am 22.10.2014 um 10:08 hat Max Reitz geschrieben:
>> The previous commit introduced the "rebuild" variable to qcow2's
>> implementation of the image consistency check. Now make use of this by
>> adding a function which creates a completely new refcount structure
>> based solely on the in-memory information gathered before.
>>
>> The old refcount structure will be leaked, however. This leak will be
>> dealt with in a follow-up commit.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 310 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 304 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 101d7bb..8ca2324 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1640,6 +1640,284 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>   }
>>   
>>   /*
>> + * Allocates clusters using an in-memory refcount table (IMRT) in contrast to
>> + * the on-disk refcount structures.
>> + *
>> + * On input, *first_free_cluster tells where to start looking, and need not
>> + * actually be a free cluster; the returned offset will not be before that
>> + * cluster.  On output, *first_free_cluster points to the first gap found, even
>> + * if that gap was too small to be used as the returned offset.
>> + *
>> + * Note that *first_free_cluster is a cluster index whereas the return value is
>> + * an offset.
>> + */
>> +static int64_t alloc_clusters_imrt(BlockDriverState *bs,
>> +                                   int cluster_count,
>> +                                   uint16_t **refcount_table,
>> +                                   int64_t *imrt_nb_clusters,
>> +                                   int64_t *first_free_cluster)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    int64_t cluster = *first_free_cluster, i;
>> +    bool first_gap = true;
>> +    int contiguous_free_clusters;
>> +
>> +    /* Starting at *first_free_cluster, find a range of at least cluster_count
>> +     * continuously free clusters */
>> +    for (contiguous_free_clusters = 0;
>> +         cluster < *imrt_nb_clusters &&
>> +         contiguous_free_clusters < cluster_count;
>> +         cluster++)
>> +    {
>> +        if (!(*refcount_table)[cluster]) {
>> +            contiguous_free_clusters++;
>> +            if (first_gap) {
>> +                /* If this is the first free cluster found, update
>> +                 * *first_free_cluster accordingly */
>> +                *first_free_cluster = cluster;
>> +                first_gap = false;
>> +            }
>> +        } else if (contiguous_free_clusters) {
>> +            contiguous_free_clusters = 0;
>> +        }
>> +    }
>> +
>> +    /* If contiguous_free_clusters is greater than zero, it contains the number
>> +     * of continuously free clusters until the current cluster; the first free
>> +     * cluster in the current "gap" is therefore
>> +     * cluster - contiguous_free_clusters */
>> +
>> +    /* If no such range could be found, grow the in-memory refcount table
>> +     * accordingly to append free clusters at the end of the image */
>> +    if (contiguous_free_clusters < cluster_count) {
>> +        int64_t old_imrt_nb_clusters = *imrt_nb_clusters;
>> +        uint16_t *new_refcount_table;
>> +
>> +        /* contiguous_free_clusters clusters are already empty at the image end;
>> +         * we need cluster_count clusters; therefore, we have to allocate
>> +         * cluster_count - contiguous_free_clusters new clusters at the end of
>> +         * the image (which is the current value of cluster; note that cluster
>> +         * may exceed old_imrt_nb_clusters if *first_free_cluster pointed beyond
>> +         * the image end) */
>> +        *imrt_nb_clusters = cluster + cluster_count - contiguous_free_clusters;
>> +        new_refcount_table = g_try_realloc(*refcount_table,
>> +                                           *imrt_nb_clusters *
>> +                                           sizeof(**refcount_table));
>> +        if (!new_refcount_table) {
>> +            return -ENOMEM;
> Now the old refcount table is preserved, but nb_clusters has already
> been updated. Smells like the next thing will be a buffer overflow,
> doesn't it?

If the caller continues after ENOMEM, well, yes. I'd call it his own 
fault, but I'll fix it of course.

Max

>> +        }
>> +        *refcount_table = new_refcount_table;
>> +
>> +        memset(*refcount_table + old_imrt_nb_clusters, 0,
>> +               (*imrt_nb_clusters - old_imrt_nb_clusters) *
>> +               sizeof(**refcount_table));
>> +    }
> Kevin