Re: [Qemu-devel] [PATCH v6 04/12] VMDK: separate vmdk_open by format version
Am 01.07.2011 15:06, schrieb Fam Zheng:
>>
>> bdrv_pread only ever returns 0 for success or -errno for errors. So you
>> can simplify the code like this:
>>
>> ret = bdrv_pread(...);
>> if (ret < 0) {
>>goto fail_l1;
>> }
>>
>> You have the same pattern in other places, too.
>
> I think bdrv_pead do return the read bytes, did you mean bdrv_read here? :)
Yes, you're right, it returns the read bytes. But it's always -errno or
the full byte count, there are no short reads. So my explanation wasn't
quite right, but the suggestion stays the same. :-)
Kevin
Re: [Qemu-devel] [PATCH v6 04/12] VMDK: separate vmdk_open by format version
>
> bdrv_pread only ever returns 0 for success or -errno for errors. So you
> can simplify the code like this:
>
> ret = bdrv_pread(...);
> if (ret < 0) {
> goto fail_l1;
> }
>
> You have the same pattern in other places, too.
>
I think bdrv_pead do return the read bytes, did you mean bdrv_read here? :)
--
Best regards!
Fam Zheng
Re: [Qemu-devel] [PATCH v6 04/12] VMDK: separate vmdk_open by format version
Am 01.07.2011 06:55, schrieb Fam Zheng:
> Separate vmdk_open by subformats to:
> * vmdk_open_vmdk3
> * vmdk_open_vmdk4
>
> Signed-off-by: Fam Zheng
> ---
> block/vmdk.c | 197 ++---
> 1 files changed, 131 insertions(+), 66 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 4684670..03248f3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -456,74 +456,26 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
> return extent;
> }
>
> -
> -static int vmdk_open(BlockDriverState *bs, int flags)
> +static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
> {
> -BDRVVmdkState *s = bs->opaque;
> -uint32_t magic;
> -int i;
> -uint32_t l1_size, l1_entry_sectors;
> -VmdkExtent *extent = NULL;
> -
> -if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic))
> -goto fail;
> -
> -magic = be32_to_cpu(magic);
> -if (magic == VMDK3_MAGIC) {
> -VMDK3Header header;
> -if (bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header))
> -!= sizeof(header)) {
> -goto fail;
> -}
> -extent = vmdk_add_extent(bs, bs->file, false,
> - le32_to_cpu(header.disk_sectors),
> - le32_to_cpu(header.l1dir_offset) << 9, 0,
> - 1 << 6, 1 << 9,
> le32_to_cpu(header.granularity));
> -} else if (magic == VMDK4_MAGIC) {
> -VMDK4Header header;
> -if (bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header))
> -!= sizeof(header)) {
> -goto fail;
> -}
> -l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
> -* le64_to_cpu(header.granularity);
> -l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
> -/ l1_entry_sectors;
> -extent = vmdk_add_extent(bs, 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) {
> -goto fail;
> -}
> -// try to open parent images, if exist
> -if (vmdk_parent_open(bs) != 0)
> -goto fail;
> -// write the CID once after the image creation
> -s->parent_cid = vmdk_read_cid(bs,1);
> -} else {
> -goto fail;
> -}
> -
> -/* sum up the total sectors */
> -bs->total_sectors = 0;
> -for (i = 0; i < s->num_extents; i++) {
> -bs->total_sectors += s->extents[i].sectors;
> -}
> +int ret;
> +int l1_size, i;
>
> /* read the L1 table */
> l1_size = extent->l1_size * sizeof(uint32_t);
> extent->l1_table = qemu_malloc(l1_size);
> -if (bdrv_pread(bs->file,
> -extent->l1_table_offset,
> -extent->l1_table,
> -l1_size)
> -!= l1_size) {
> +if (!extent->l1_table) {
> +ret = -ENOMEM;
> goto fail;
> }
qemu_malloc() never fails, so you don't need this check.
> +ret = bdrv_pread(bs->file,
> +extent->l1_table_offset,
> +extent->l1_table,
> +l1_size);
> +if (ret != l1_size) {
> +ret = ret < 0 ? ret : -EIO;
> +goto fail_l1;
> +}
bdrv_pread only ever returns 0 for success or -errno for errors. So you
can simplify the code like this:
ret = bdrv_pread(...);
if (ret < 0) {
goto fail_l1;
}
You have the same pattern in other places, too.
> for (i = 0; i < extent->l1_size; i++) {
> le32_to_cpus(&extent->l1_table[i]);
> }
> @@ -538,16 +490,129 @@ static int vmdk_open(BlockDriverState *bs, int flags)
> goto fail;
> }
> for (i = 0; i < extent->l1_size; i++) {
> +if (!extent->l1_backup_table) {
> +ret = -ENOMEM;
> +goto fail_l1;
> +}
> +}
Hm, isn't this checking the same thing multiple times?
Anyway, qemu_malloc() still doesn't fail. ;-)
> +ret = bdrv_pread(bs->file,
> +extent->l1_backup_table_offset,
> +extent->l1_backup_table,
> +l1_size);
This duplicates a read from a few lines above. Mismerge?
> +if (ret != l1_size) {
> +ret = ret < 0 ? ret : -EIO;
> +goto fail;
> +}
> +for (i = 0; i < extent->l1_size; i++) {
> le32_to_cpus(&extent->l1_backup_table[i]);
> }
> }
>
> extent->l2_cache =
> qemu_malloc(extent->l2_size * L2_CACHE_SIZE * sizeof(uint32_t));
> +i
[Qemu-devel] [PATCH v6 04/12] VMDK: separate vmdk_open by format version
Separate vmdk_open by subformats to:
* vmdk_open_vmdk3
* vmdk_open_vmdk4
Signed-off-by: Fam Zheng
---
block/vmdk.c | 197 ++---
1 files changed, 131 insertions(+), 66 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 4684670..03248f3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -456,74 +456,26 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
return extent;
}
-
-static int vmdk_open(BlockDriverState *bs, int flags)
+static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
{
-BDRVVmdkState *s = bs->opaque;
-uint32_t magic;
-int i;
-uint32_t l1_size, l1_entry_sectors;
-VmdkExtent *extent = NULL;
-
-if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic))
-goto fail;
-
-magic = be32_to_cpu(magic);
-if (magic == VMDK3_MAGIC) {
-VMDK3Header header;
-if (bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header))
-!= sizeof(header)) {
-goto fail;
-}
-extent = vmdk_add_extent(bs, bs->file, false,
- le32_to_cpu(header.disk_sectors),
- le32_to_cpu(header.l1dir_offset) << 9, 0,
- 1 << 6, 1 << 9, le32_to_cpu(header.granularity));
-} else if (magic == VMDK4_MAGIC) {
-VMDK4Header header;
-if (bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header))
-!= sizeof(header)) {
-goto fail;
-}
-l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
-* le64_to_cpu(header.granularity);
-l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
-/ l1_entry_sectors;
-extent = vmdk_add_extent(bs, 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) {
-goto fail;
-}
-// try to open parent images, if exist
-if (vmdk_parent_open(bs) != 0)
-goto fail;
-// write the CID once after the image creation
-s->parent_cid = vmdk_read_cid(bs,1);
-} else {
-goto fail;
-}
-
-/* sum up the total sectors */
-bs->total_sectors = 0;
-for (i = 0; i < s->num_extents; i++) {
-bs->total_sectors += s->extents[i].sectors;
-}
+int ret;
+int l1_size, i;
/* read the L1 table */
l1_size = extent->l1_size * sizeof(uint32_t);
extent->l1_table = qemu_malloc(l1_size);
-if (bdrv_pread(bs->file,
-extent->l1_table_offset,
-extent->l1_table,
-l1_size)
-!= l1_size) {
+if (!extent->l1_table) {
+ret = -ENOMEM;
goto fail;
}
+ret = bdrv_pread(bs->file,
+extent->l1_table_offset,
+extent->l1_table,
+l1_size);
+if (ret != l1_size) {
+ret = ret < 0 ? ret : -EIO;
+goto fail_l1;
+}
for (i = 0; i < extent->l1_size; i++) {
le32_to_cpus(&extent->l1_table[i]);
}
@@ -538,16 +490,129 @@ static int vmdk_open(BlockDriverState *bs, int flags)
goto fail;
}
for (i = 0; i < extent->l1_size; i++) {
+if (!extent->l1_backup_table) {
+ret = -ENOMEM;
+goto fail_l1;
+}
+}
+ret = bdrv_pread(bs->file,
+extent->l1_backup_table_offset,
+extent->l1_backup_table,
+l1_size);
+if (ret != l1_size) {
+ret = ret < 0 ? ret : -EIO;
+goto fail;
+}
+for (i = 0; i < extent->l1_size; i++) {
le32_to_cpus(&extent->l1_backup_table[i]);
}
}
extent->l2_cache =
qemu_malloc(extent->l2_size * L2_CACHE_SIZE * sizeof(uint32_t));
+if (!extent->l2_cache) {
+ret = -ENOMEM;
+goto fail_l1b;
+}
return 0;
+ fail_l1b:
+qemu_free(extent->l1_backup_table);
+ fail_l1:
+qemu_free(extent->l1_table);
fail:
-vmdk_free_extents(bs);
-return -1;
+return ret;
+}
+
+static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
+{
+int ret;
+uint32_t magic;
+VMDK3Header header;
+VmdkExtent *extent;
+BDRVVmdkState *s = bs->opaque;
+
+ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+if (ret != sizeof(header)) {
+ret = ret < 0 ? ret : -EIO;
+goto fail;
+}
+extent = vmdk_add_extent(bs,
+ bs->file, false,
+
