Re: [Qemu-devel] [PATCH 01/12] VMDK: Introduced VmdkExtent

[Stefan Hajnoczi]

On Sat, Jun 04, 2011 at 08:40:22AM +0800, Fam Zheng wrote:
>   fail:
> -    qemu_free(s->l1_backup_table);
> -    qemu_free(s->l1_table);
> -    qemu_free(s->l2_cache);
> +    if(s->extents) {
> +        qemu_free(s->extents[0].l1_backup_table);
> +        qemu_free(s->extents[0].l1_table);
> +        qemu_free(s->extents[0].l2_cache);
> +    }

for (i = 0; i < s->num_extents; i++) {
    qemu_free(s->extents[i].l1_backup_table);
    qemu_free(s->extents[i].l1_table);
    qemu_free(s->extents[i].l2_cache);
}
qemu_free(s->extents);

> +static int find_extent(BDRVVmdkState *s, int64_t sector_num, int start_idx)
> +{
> +    int ext_idx = start_idx;
> +    while (ext_idx < s->num_extents
> +            && sector_num >= s->extents[ext_idx].sectors) {
> +        sector_num -= s->extents[ext_idx].sectors;
> +        ext_idx++;
> +    }
> +    if (ext_idx == s->num_extents) return -1;
> +    return ext_idx;
> +}

Callers don't really need the index, they just want the extent and an
optimized way of repeated calls to avoid O(N^2) find_extent() times:

static VmdkExtent *find_extent(BDRVVmdkState *s, int64_t sector_num,
                               VmdkExtent *start_extent)
{
    VmdkExtent *extent = start_extent;

    if (!start_extent) {
        extent = &s->extent[0];
    }

    while (extent < &s->extents[s->num_extents]) {
        if (sector_num < extent->end) {
            return extent;
        }
        extent++;
    }
    return NULL;
}

I added the VmdkExtent.end field so that this function can be called
repeatedly for an increasing series of sector_num values.  It seems like
your code would fail when called with a non-0 index since sector_num -=
s->extents[ext_idx].sectors is incorrect when starting from an arbitrary
extent_idx.

> +
>  static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>  {
>      BDRVVmdkState *s = bs->opaque;
> -    int index_in_cluster, n;
> -    uint64_t cluster_offset;
> 
> -    cluster_offset = get_cluster_offset(bs, NULL, sector_num << 9, 0);
> -    index_in_cluster = sector_num % s->cluster_sectors;
> -    n = s->cluster_sectors - index_in_cluster;
> +    int index_in_cluster, n, ret;
> +    uint64_t offset;
> +    VmdkExtent *extent;
> +    int ext_idx;
> +
> +    ext_idx = find_extent(s, sector_num, 0);
> +    if (ext_idx == -1) return 0;
> +    extent = &s->extents[ext_idx];
> +    if (s->extents[ext_idx].flat) {
> +        n = extent->sectors - sector_num;

If I have two flat extents:
Extent A: 0     - 1.5MB
Extent B: 1.5MB - 2MB

And I call vmdk_is_allocated(sector_num=1.5MB), then n = 512KB - 1.5MB
which is negative!  Also n is an int but it should be an int64_t (or
uint64_t) which can hold sector units.

Stefan