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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sat, Jun 4, 2011 at 1:41 AM, Fam Zheng <famcool@gmail.com> wrote:
> +static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
> +{
> +    uint32_t magic;
> +    VMDK4Header header;
> +    BDRVVmdkState *s = bs->opaque;
> +    VmdkExtent *extent;
> +
> +    s->extents = qemu_mallocz(sizeof(VmdkExtent));
> +    s->num_extents = 1;
> +    if (bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header))
> +        != sizeof(header)) {
> +        goto fail;
> +    }
> +    extent = s->extents;
> +    extent->file = bs->file;
> +    extent->sectors = le64_to_cpu(header.capacity);
> +    extent->cluster_sectors = le64_to_cpu(header.granularity);
> +    extent->l2_size = le32_to_cpu(header.num_gtes_per_gte);
> +    extent->l1_entry_sectors = extent->l2_size * extent->cluster_sectors;
> +    if (extent->l1_entry_sectors <= 0) {
> +        goto fail;
> +    }
> +    extent->l1_size = (extent->sectors + extent->l1_entry_sectors - 1)
> +        / extent->l1_entry_sectors;
> +    extent->l1_table_offset = le64_to_cpu(header.rgd_offset) << 9;
> +    extent->l1_backup_table_offset = le64_to_cpu(header.gd_offset) << 9;
> +
> +    // try to open parent images, if exist
> +    if (vmdk_parent_open(bs) != 0) {
> +        goto fail;
> +    }
> +    // write the CID once after the image creation

This comment is from vmdk_create() and can be removed?

> +    extent->parent_cid = vmdk_read_cid(bs,1);
> +    bs->total_sectors = extent->sectors;
> +    if (vmdk_init_tables(bs, extent)) {
> +        goto fail;
>     }
> +    return 0;
> + fail:
> +    qemu_free(s->extents);
>     return -1;
>  }
>
> +static int vmdk_open(BlockDriverState *bs, int flags)
> +{
> +    uint32_t magic;
> +
> +    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> +        return -1;
> +    }
> +
> +    magic = be32_to_cpu(magic);
> +    if (magic == VMDK3_MAGIC) {
> +        return vmdk_open_vmdk3(bs, flags);
> +    } else if (magic == VMDK4_MAGIC) {
> +        return vmdk_open_vmdk4(bs, flags);
> +    } else {
> +        return -1;
> +    }

This is a good opportunity to use -errno instead of -1.  If you look
at qcow2 or qed they return appropriate error numbers and not just -1.
 (Also remember -1 is -EPERM so callers that interpret the return
value as an error will print junk error messages if -1 is returned.)

Stefan