On Thu, Jun 9, 2011 at 3:58 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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);
>
This looks better, although num_extents is 1 at most here.
>> +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.
>
Parameter sector_num which I meant was relative to start_idx, so
caller passes a relative sector_num when calling with a non-0 index.
>> +
>> 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.
You are right. I did this because I wasn't considering multi extent
situations yet in this patch, and when introducing multi extents
support this problem is fixed.
--
Best regards!
Fam Zheng
