Re: [Qemu-devel] [PATCH v3 04/12] VMDK: separate vmdk_open by format version

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <famcool@gmail.com> wrote:
> -        extent = s->extents;
> -        extent->flat = false;
> -        extent->file = bs->file;
> -        extent->cluster_sectors = le32_to_cpu(header.granularity);
> -        extent->l2_size = 1 << 9;
> -        extent->l1_size = 1 << 6;
> -        extent->sectors = le32_to_cpu(header.disk_sectors);
> -        extent->end_sector = le32_to_cpu(header.disk_sectors);
> -        extent->l1_table_offset = le32_to_cpu(header.l1dir_offset) << 9;
> -        extent->l1_backup_table_offset = 0;
> -        extent->l1_entry_sectors = extent->l2_size * extent->cluster_sectors;

Please move vmdk_add_extent() to the patch where you added this code.
A nice patch series minimizes the amount of code that gets added
temporarily and then removed again.

> +/* Create and append extent to the entext array. Return the added VmdkExtent

s/entext/extent/

> + * address. return NULL if allocation failed. */
> +static int vmdk_add_extent(BlockDriverState *bs,
> +                           BlockDriverState *file, bool flat, int64_t sectors,
> +                           int64_t l1_offset, int64_t l1_backup_offset,
> +                           uint32_t l1_size,
> +                           int l2_size, unsigned int cluster_sectors,
> +                           VmdkExtent **new_extent)
> +{
> +    VmdkExtent *extent, *p;
> +    BDRVVmdkState *s = bs->opaque;
> +
> +    p = qemu_realloc(s->extents, (s->num_extents + 1) * sizeof(VmdkExtent));
> +    if (!p) {
> +        return -ENOMEM;
> +    }

qemu_realloc() never returns NULL.

> +    s->extents = p;
> +    extent = &s->extents[s->num_extents];
> +    s->num_extents++;
> +
> +    memset(extent, 0, sizeof(VmdkExtent));
> +    extent->file = file;
> +    extent->flat = flat;
> +    extent->sectors = sectors;
> +    extent->l1_table_offset = l1_offset;
> +    extent->l1_backup_table_offset = l1_backup_offset;
> +    extent->l1_size = l1_size;
> +    extent->l1_entry_sectors = l2_size * cluster_sectors;
> +    extent->l2_size = l2_size;
> +    extent->cluster_sectors = cluster_sectors;
> +
> +    if (s->num_extents > 1) {
> +        extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
> +    } else {
> +        extent->end_sector = extent->sectors;
> +    }
> +    bs->total_sectors = extent->end_sector;
> +    if (new_extent) {
> +        *new_extent = extent;
> +    }
> +    return 0;

Since this function cannot fail it would be simpler to make the return
value VmdkExtent *.

Stefan