On Thu, Aug 4, 2011 at 3:35 AM, Fam Zheng <famc...@gmail.com> wrote:
> On Tue, Aug 2, 2011 at 6:32 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
>> On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famc...@gmail.com> wrote:
>>> Add twoGbMaxExtentSparse support. Only opening code is changed.
>>>
>>> Signed-off-by: Fam Zheng <famc...@gmail.com>
>>> ---
>>> block/vmdk.c | 124
>>> ++++++++++++++++++++++++++++++++++++----------------------
>>> 1 files changed, 77 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/block/vmdk.c b/block/vmdk.c
>>> index 9e6c67a..9d1ae32 100644
>>> --- a/block/vmdk.c
>>> +++ b/block/vmdk.c
>>> @@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs,
>>> VmdkExtent *extent)
>>> return ret;
>>> }
>>>
>>> -static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>> +static int vmdk_open_vmdk3(BlockDriverState *bs,
>>> + BlockDriverState *file,
>>> + int flags)
>>> {
>>> int ret;
>>> uint32_t magic;
>>> @@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int
>>> flags)
>>> BDRVVmdkState *s = bs->opaque;
>>> VmdkExtent *extent;
>>>
>>> - s->desc_offset = 0x200;
>>> - ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>>> + ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>> if (ret < 0) {
>>> - goto fail;
>>> + return ret;
>>> }
>>> extent = vmdk_add_extent(bs,
>>> bs->file, false,
>>> @@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int
>>> flags)
>>> le32_to_cpu(header.granularity));
>>> ret = vmdk_init_tables(bs, extent);
>>> if (ret) {
>>> - /* vmdk_init_tables cleans up on fail, so only free allocation of
>>> - * vmdk_add_extent here. */
>>> - goto fail;
>>> + /* free extent allocated by vmdk_add_extent */
>>> + s->num_extents--;
>>> + qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>>
>> The return value of qemu_realloc() is the new address of s->extents[].
>> You are ignoring the new address and continue to use the old one.
>>
>> Please add a vmdk_free_last_extent() function that encapsulates this.
>> The rest of the vmdk.c code should not manipulate
>> s->extents[]/s->num_extents. Use vmdk_add_extent(),
>> vmdk_free_last_extent(), vmdk_free_extents() instead of spreading the
>> extent management code in various places.
>>
>>> }
>>> - return 0;
>>> - fail:
>>> - vmdk_free_extents(bs);
>>> return ret;
>>> }
>>>
>>> -static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>>> +static int vmdk_open_vmdk4(BlockDriverState *bs,
>>> + BlockDriverState *file,
>>> + int flags)
>>> {
>>> int ret;
>>> uint32_t magic;
>>> @@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int
>>> flags)
>>> BDRVVmdkState *s = bs->opaque;
>>> VmdkExtent *extent;
>>>
>>> - s->desc_offset = 0x200;
>>> - ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>>> + ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>> if (ret < 0) {
>>> - goto fail;
>>> + return ret;
>>> }
>>> l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>>> * le64_to_cpu(header.granularity);
>>> + if (l1_entry_sectors <= 0) {
>>> + return -EINVAL;
>>> + }
>>> l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
>>> / l1_entry_sectors;
>>> - extent = vmdk_add_extent(bs, bs->file, false,
>>> + extent = vmdk_add_extent(bs, file, false,
>>> le64_to_cpu(header.capacity),
>>> le64_to_cpu(header.gd_offset) << 9,
>>> le64_to_cpu(header.rgd_offset) << 9,
>>> l1_size,
>>> le32_to_cpu(header.num_gtes_per_gte),
>>> le64_to_cpu(header.granularity));
>>> - if (extent->l1_entry_sectors <= 0) {
>>> - ret = -EINVAL;
>>> - goto fail;
>>> - }
>>> - /* try to open parent images, if exist */
>>> - ret = vmdk_parent_open(bs);
>>> - if (ret) {
>>> - goto fail;
>>> - }
>>> - s->parent_cid = vmdk_read_cid(bs, 1);
>>> ret = vmdk_init_tables(bs, extent);
>>> if (ret) {
>>> - goto fail;
>>> + /* free extent allocated by vmdk_add_extent */
>>> + s->num_extents--;
>>> + qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>>
>> Same bug.
>>
>>> }
>>> - return 0;
>>> - fail:
>>> - vmdk_free_extents(bs);
>>> return ret;
>>> }
>>>
>>> @@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc,
>>> const char *opt_name,
>>> return 0;
>>> }
>>>
>>> +/* Open an extent file and append to bs array */
>>> +static int vmdk_open_sparse(BlockDriverState *bs,
>>> + BlockDriverState *file,
>>> + int flags)
>>> +{
>>> + int ret;
>>> + uint32_t magic;
>>> +
>>> + if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>>> + return -EIO;
>>> + }
>>> +
>>> + magic = be32_to_cpu(magic);
>>> + if (magic == VMDK3_MAGIC) {
>>> + ret = vmdk_open_vmdk3(bs, file, flags);
>>> + if (ret) {
>>> + return ret;
>>> + }
>>> + } else if (magic == VMDK4_MAGIC) {
>>> + ret = vmdk_open_vmdk4(bs, file, flags);
>>> + if (ret) {
>>> + return ret;
>>> + }
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> + return 0;
>>
>> Shorter way of writing this (ret is no longer necessary):
>>
>> switch (magic) {
>> case VMDK3_MAGIC:
>> return vmdk_open_vmdk3(bs, file, flags);
>> case VMDK4_MAGIC:
>> return vmdk_open_vmdk4(bs, file, flags);
>> default:
>> return -EINVAL;
>> }
>>
>>> +}
>>> +
>>> static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>> const char *desc_file_path)
>>> {
>>> @@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc,
>>> BlockDriverState *bs,
>>> const char *p = desc;
>>> int64_t sectors = 0;
>>> int64_t flat_offset;
>>> + char extent_path[PATH_MAX];
>>> + BlockDriverState *extent_file;
>>>
>>> while (*p) {
>>> /* parse extent line:
>>> @@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc,
>>> BlockDriverState *bs,
>>> goto next_line;
>>> }
>>>
>>> + path_combine(extent_path, sizeof(extent_path),
>>> + desc_file_path, fname);
>>> + ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>>> + if (ret) {
>>> + return ret;
>>> + }
>>> +
>>> /* save to extents array */
>>> if (!strcmp(type, "FLAT")) {
>>> /* FLAT extent */
>>> - char extent_path[PATH_MAX];
>>> - BlockDriverState *extent_file;
>>> VmdkExtent *extent;
>>>
>>> - path_combine(extent_path, sizeof(extent_path),
>>> - desc_file_path, fname);
>>> - ret = bdrv_file_open(&extent_file, extent_path,
>>> bs->open_flags);
>>> - if (ret) {
>>> - return ret;
>>> - }
>>> extent = vmdk_add_extent(bs, extent_file, true, sectors,
>>> 0, 0, 0, 0, sectors);
>>> extent->flat_start_offset = flat_offset;
>>> + } else if (!strcmp(type, "SPARSE")) {
>>> + /* SPARSE extent */
>>> + ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
>>> + if (ret) {
>>> + bdrv_delete(extent_file);
>>> + return ret;
>>> + }
>>> } else {
>>> /* SPARSE extent, not supported for now */
>>
>> This comment is outdated.
>>
>>> fprintf(stderr,
>>> @@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs,
>>> int flags)
>>> return -EINVAL;
>>> }
>>> if (strcmp(ct, "monolithicFlat") &&
>>> + strcmp(ct, "twoGbMaxExtentSparse") &&
>>> strcmp(ct, "twoGbMaxExtentFlat")) {
>>> fprintf(stderr,
>>> "VMDK: Not supported image type \"%s\""".\n", ct);
>>> @@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs,
>>> int flags)
>>>
>>> static int vmdk_open(BlockDriverState *bs, int flags)
>>> {
>>> - uint32_t magic;
>>> -
>>> - if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>>> - return -EIO;
>>> - }
>>> + int ret;
>>> + BDRVVmdkState *s = bs->opaque;
>>>
>>> - 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);
>>> + if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
>>> + s->desc_offset = 0x200;
>>> + /* try to open parent images, if exist */
>>> + ret = vmdk_parent_open(bs);
>>> + if (ret) {
>>> + return ret;
>>
>> What needs to be freed here?
>>
>
> No resource allocation in vmdk_parent_open, so I think none.
But what about vmdk_open_sparse(), which succeeded? This function is
a .bdrv_open(). If you return an error then bs->file will be deleted
and bs->opaque is freed - but you need to clean up all image
format-specific resources *before* returning error. bdrv_close() will
not be called if bdrv_open() returns error.
Stefan
