[Qemu-devel] VMDK: footer must take precedence over header when present

[Qemu-devel] VMDK: footer must take precedence over header when present

[B Gordon]

VMDK: footer must take precedence over header when present

In e.g. streamOptimized VMDKs from VSphere 4 with this flag set the
header l1_table is bogus and only the footer l1_table can be used to
correctly read extents.

Also reverts recent change to VMDK4Header so order of rgd_ and
gd_offset matches the VMDK spec.

https://bugs.launchpad.net/qemu/+bug/907063

Signed-off-by: B.B. Gordon <bbgordonn@fastmail.fm>

diff --git a/block/vmdk.c b/block/vmdk.c
index 5623ac1..77ff9e1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -35,6 +35,7 @@
 #define VMDK4_FLAG_RGD (1 << 1)
 #define VMDK4_FLAG_COMPRESS (1 << 16)
 #define VMDK4_FLAG_MARKER (1 << 17)
+#define VMDK4_HEADER_AT_END 0xffffffffffffffffULL
 
 typedef struct {
     uint32_t version;
@@ -57,8 +58,8 @@ typedef struct {
     int64_t desc_offset;
     int64_t desc_size;
     int32_t num_gtes_per_gte;
-    int64_t gd_offset;
     int64_t rgd_offset;
+    int64_t gd_offset;
     int64_t grain_offset;
     char filler[1];
     char check_bytes[4];
@@ -443,11 +445,27 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     VMDK4Header header;
     VmdkExtent *extent;
     int64_t l1_backup_offset = 0;
+    struct stat sb;
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
         return ret;
     }
+    if (le64_to_cpu(header.gd_offset) == VMDK4_HEADER_AT_END) {
+        uint64_t filesize;
+        if (stat(bs->filename, &sb) == -1) {
+            fprintf(stderr, "Unable to get file size.\n");
+            return -1;
+        }
+        filesize = (((uint64_t)sb.st_size + (512 - 1)) &
~(uint64_t)(512 - 1));
+        ret = bdrv_pread(bs->file,
+                         (filesize - 1024 + sizeof(magic)),
+                         &header,
+                         sizeof(header));
+        if (ret != sizeof(header)) {
+            return ret;
+        }
+    }
     if (header.capacity == 0 && header.desc_offset) {
         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
     }

-- 
http://www.fastmail.fm - Choose from over 50 domains or use your own

Re: [Qemu-devel] VMDK: footer must take precedence over header when present

[Stefan Hajnoczi]

On Wed, Dec 21, 2011 at 11:34 PM, B Gordon <bbgordonn@fastmail.fm> wrote:
> VMDK: footer must take precedence over header when present
>
> In e.g. streamOptimized VMDKs from VSphere 4 with this flag set the
> header l1_table is bogus and only the footer l1_table can be used to
> correctly read extents.
>
> Also reverts recent change to VMDK4Header so order of rgd_ and
> gd_offset matches the VMDK spec.

Hi Fam,
In commit bb45ded93115ad4303471c9a492579dc36716547 you moved the
rgd_offset field after gd_offset.  The VMDK spec has the rgd_offset
field before gd_offset.

Could you please review B.B. Gordon's patch which reverts this change
and implements the header/footer precedence described in the "Header
and Footer" section for Stream-Optimized Compressed Sparse Extents?
If you still have the .vmdk files you tested against it would be
interesting to verify that applying this new patch doesn't break them.

http://patchwork.ozlabs.org/patch/132758/

Thanks,
Stefan

Re: [Qemu-devel] VMDK: footer must take precedence over header when present

[Fam Zheng]

On Thu, Dec 22, 2011 at 7:34 AM, B Gordon <bbgordonn@fastmail.fm> wrote:
> VMDK: footer must take precedence over header when present
>
> In e.g. streamOptimized VMDKs from VSphere 4 with this flag set the
> header l1_table is bogus and only the footer l1_table can be used to
> correctly read extents.
>
> Also reverts recent change to VMDK4Header so order of rgd_ and
> gd_offset matches the VMDK spec.
>
> https://bugs.launchpad.net/qemu/+bug/907063
>
> Signed-off-by: B.B. Gordon <bbgordonn@fastmail.fm>
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 5623ac1..77ff9e1 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -35,6 +35,7 @@
>  #define VMDK4_FLAG_RGD (1 << 1)
>  #define VMDK4_FLAG_COMPRESS (1 << 16)
>  #define VMDK4_FLAG_MARKER (1 << 17)
> +#define VMDK4_HEADER_AT_END 0xffffffffffffffffULL
>
>  typedef struct {
>     uint32_t version;
> @@ -57,8 +58,8 @@ typedef struct {
>     int64_t desc_offset;
>     int64_t desc_size;
>     int32_t num_gtes_per_gte;
> -    int64_t gd_offset;
>     int64_t rgd_offset;
> +    int64_t gd_offset;
>     int64_t grain_offset;
>     char filler[1];
>     char check_bytes[4];
> @@ -443,11 +445,27 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>     VMDK4Header header;
>     VmdkExtent *extent;
>     int64_t l1_backup_offset = 0;
> +    struct stat sb;

Please use `bdrv_getlength' to get file sizes, rather than using
`stat' which is system dependent.

>
>     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>     if (ret < 0) {
>         return ret;
>     }
> +    if (le64_to_cpu(header.gd_offset) == VMDK4_HEADER_AT_END) {
> +        uint64_t filesize;
> +        if (stat(bs->filename, &sb) == -1) {
> +            fprintf(stderr, "Unable to get file size.\n");
> +            return -1;
> +        }
> +        filesize = (((uint64_t)sb.st_size + (512 - 1)) &
> ~(uint64_t)(512 - 1));
> +        ret = bdrv_pread(bs->file,
> +                         (filesize - 1024 + sizeof(magic)),
> +                         &header,
> +                         sizeof(header));
> +        if (ret != sizeof(header)) {
> +            return ret;

if ret is positive and less than sizeof(header), the return value is
meaningless. Also makes problem when ret == 0.

> +        }
> +    }
>     if (header.capacity == 0 && header.desc_offset) {
>         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
>     }
>