| Message ID | 1311758866-30029-3-git-send-email-famcool@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote: > Add twoGbMaxExtentSparse support. Only opening code is changed. > > Signed-off-by: Fam Zheng <famcool@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? Stefan
On Tue, Aug 2, 2011 at 6:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote: >> Add twoGbMaxExtentSparse support. Only opening code is changed. >> >> Signed-off-by: Fam Zheng <famcool@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.
On Thu, Aug 4, 2011 at 3:35 AM, Fam Zheng <famcool@gmail.com> wrote: > On Tue, Aug 2, 2011 at 6:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote: >>> Add twoGbMaxExtentSparse support. Only opening code is changed. >>> >>> Signed-off-by: Fam Zheng <famcool@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
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)); } - 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)); } - 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; +} + 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 */ 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; + } + s->parent_cid = vmdk_read_cid(bs, 1); + return 0; } else { return vmdk_open_desc_file(bs, flags); }
Add twoGbMaxExtentSparse support. Only opening code is changed. Signed-off-by: Fam Zheng <famcool@gmail.com> --- block/vmdk.c | 124 ++++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 77 insertions(+), 47 deletions(-)