Series comparison

-[PATCH v2] vpc: Read images exported from Azure correctly
+[PATCH v3] vpc: Read images exported from Azure correctly
 ...
 we can see that Azure uses a different 'Creator application' --
 'wa\0\0' (offset 0x1c, likely reads as 'Windows Azure') and QEMU uses this
 field to determine how it can get image size. Apparently, Azure uses 'new'
 method, just like Hyper-V.
+Overall, it seems that only VPC and old QEMUs need to be ignored as all new
+creator apps seem to have reliable current_size. Invert the logic and make
+'current_size' method the default to avoid adding every new creator app to
+the list.
 Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
 ---
-Alternatively, we can probably make 'current_size' the default and only use
+Changes since v1/v2: invert the logic and make 'vpc' and 'qemu' use CHS
-CHS for 'vpc '/'qemu'.
+while defaulting to current_size.
 ---
- block/vpc.c | 2 ++
+ block/vpc.c | 65 ++++++++++++++++++++++++++++-------------------------
-file changed, 2 insertions(+)
+file changed, 35 insertions(+), 30 deletions(-)
 diff --git a/block/vpc.c b/block/vpc.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/vpc.c
 +++ b/block/vpc.c
+@@ -XXX,XX +XXX,XX @@ static void vpc_parse_options(BlockDriverState *bs, QemuOpts *opts,
+     }
+ }
++/*
++ * Microsoft Virtual PC and Microsoft Hyper-V produce and read
++ * VHD image sizes differently.  VPC will rely on CHS geometry,
++ * while Hyper-V and disk2vhd use the size specified in the footer.
++ *
++ * We use a couple of approaches to try and determine the correct method:
++ * look at the Creator App field, and look for images that have CHS
++ * geometry that is the maximum value.
++ *
++ * If the CHS geometry is the maximum CHS geometry, then we assume that
++ * the size is the footer->current_size to avoid truncation.  Otherwise,
++ * we follow the table based on footer->creator_app:
++ *
++ *  Currently known creator apps:
++ *      'vpc '  :  CHS              Virtual PC (uses disk geometry)
++ *      'qemu'  :  CHS              QEMU (uses disk geometry)
++ *      'qem2'  :  current_size     QEMU (uses current_size)
++ *      'win '  :  current_size     Hyper-V
++ *      'd2v '  :  current_size     Disk2vhd
++ *      'tap\0' :  current_size     XenServer
++ *      'CTXS'  :  current_size     XenConverter
++ *      'wa\0\0':  current_size     Azure
++ *
++ *  The user can override the table values via drive options, however
++ *  even with an override we will still use current_size for images
++ *  that have CHS geometry of the maximum size.
++ */
++static bool vpc_ignore_current_size(VHDFooter *footer)
++{
++    return !strncmp(footer->creator_app, "vpc ", 4) ||
++        !strncmp(footer->creator_app, "qemu", 4);
++}
++
+ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
+ {
 @@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
-      *      'qemu'  :  CHS              QEMU (uses disk geometry)
+     bs->total_sectors = (int64_t)
-      *      'qem2'  :  current_size     QEMU (uses current_size)
+         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
-      *      'win '  :  current_size     Hyper-V
-+     *      'wa\0\0':  current_size     Azure
+-    /* Microsoft Virtual PC and Microsoft Hyper-V produce and read
-      *      'd2v '  :  current_size     Disk2vhd
+-     * VHD image sizes differently.  VPC will rely on CHS geometry,
-      *      'tap\0' :  current_size     XenServer
+-     * while Hyper-V and disk2vhd use the size specified in the footer.
-      *      'CTXS'  :  current_size     XenConverter
+-     *
-@@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
+-     * We use a couple of approaches to try and determine the correct method:
-      *  that have CHS geometry of the maximum size.
+-     * look at the Creator App field, and look for images that have CHS
-      */
+-     * geometry that is the maximum value.
-     use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
+-     *
-+               !!memcmp(footer->creator_app, "wa\0", 4) &&
+-     * If the CHS geometry is the maximum CHS geometry, then we assume that
-                !!strncmp(footer->creator_app, "qem2", 4) &&
+-     * the size is the footer->current_size to avoid truncation.  Otherwise,
-                !!strncmp(footer->creator_app, "d2v ", 4) &&
+-     * we follow the table based on footer->creator_app:
-                !!strncmp(footer->creator_app, "CTXS", 4) &&
+-     *
 -     *  Known creator apps:
 -     *      'vpc '  :  CHS              Virtual PC (uses disk geometry)
 -     *      'qemu'  :  CHS              QEMU (uses disk geometry)
 -     *      'qem2'  :  current_size     QEMU (uses current_size)
 -     *      'win '  :  current_size     Hyper-V
 -     *      'd2v '  :  current_size     Disk2vhd
 -     *      'tap\0' :  current_size     XenServer
 -     *      'CTXS'  :  current_size     XenConverter
 -     *
 -     *  The user can override the table values via drive options, however
 -     *  even with an override we will still use current_size for images
 -     *  that have CHS geometry of the maximum size.
 -     */
 -    use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
 -               !!strncmp(footer->creator_app, "qem2", 4) &&
 -               !!strncmp(footer->creator_app, "d2v ", 4) &&
 -               !!strncmp(footer->creator_app, "CTXS", 4) &&
 -               !!memcmp(footer->creator_app, "tap", 4)) || s->force_use_chs;
 +    /* Use CHS or current_size to determine the image size */
 +    use_chs = vpc_ignore_current_size(footer) || s->force_use_chs;
      if (!use_chs || bs->total_sectors == VHD_MAX_GEOMETRY || s->force_use_sz) {
          bs->total_sectors = be64_to_cpu(footer->current_size) /
 --
 .47.0

It was found that 'qemu-nbd' is not able to work with some disk images
exported from Azure. Looking at the 512b footer (which contains VPC
metadata):

00000000  63 6f 6e 65 63 74 69 78  00 00 00 02 00 01 00 00  |conectix........|
00000010  ff ff ff ff ff ff ff ff  2e c7 9b 96 77 61 00 00  |............wa..|
00000020  00 07 00 00 57 69 32 6b  00 00 00 01 40 00 00 00  |....Wi2k....@...|
00000030  00 00 00 01 40 00 00 00  28 a2 10 3f 00 00 00 02  |....@...(..?....|
00000040  ff ff e7 47 8c 54 df 94  bd 35 71 4c 94 5f e5 44  |...G.T...5qL._.D|
00000050  44 53 92 1a 00 00 00 00  00 00 00 00 00 00 00 00  |DS..............|
00000060  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

we can see that Azure uses a different 'Creator application' --
'wa\0\0' (offset 0x1c, likely reads as 'Windows Azure') and QEMU uses this
field to determine how it can get image size. Apparently, Azure uses 'new'
method, just like Hyper-V.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Alternatively, we can probably make 'current_size' the default and only use
CHS for 'vpc '/'qemu'.
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
      *      'qemu'  :  CHS              QEMU (uses disk geometry)
      *      'qem2'  :  current_size     QEMU (uses current_size)
      *      'win '  :  current_size     Hyper-V
+     *      'wa\0\0':  current_size     Azure
      *      'd2v '  :  current_size     Disk2vhd
      *      'tap\0' :  current_size     XenServer
      *      'CTXS'  :  current_size     XenConverter
@@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
      *  that have CHS geometry of the maximum size.
      */
     use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
+               !!memcmp(footer->creator_app, "wa\0", 4) &&
                !!strncmp(footer->creator_app, "qem2", 4) &&
                !!strncmp(footer->creator_app, "d2v ", 4) &&
                !!strncmp(footer->creator_app, "CTXS", 4) &&
-- 
2.47.0

It was found that 'qemu-nbd' is not able to work with some disk images
exported from Azure. Looking at the 512b footer (which contains VPC
metadata):

Overall, it seems that only VPC and old QEMUs need to be ignored as all new
creator apps seem to have reliable current_size. Invert the logic and make
'current_size' method the default to avoid adding every new creator app to
the list.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since v1/v2: invert the logic and make 'vpc' and 'qemu' use CHS
while defaulting to current_size.
---
 block/vpc.c | 65 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -XXX,XX +XXX,XX @@ static void vpc_parse_options(BlockDriverState *bs, QemuOpts *opts,
     }
 }
 
+/*
+ * Microsoft Virtual PC and Microsoft Hyper-V produce and read
+ * VHD image sizes differently.  VPC will rely on CHS geometry,
+ * while Hyper-V and disk2vhd use the size specified in the footer.
+ *
+ * We use a couple of approaches to try and determine the correct method:
+ * look at the Creator App field, and look for images that have CHS
+ * geometry that is the maximum value.
+ *
+ * If the CHS geometry is the maximum CHS geometry, then we assume that
+ * the size is the footer->current_size to avoid truncation.  Otherwise,
+ * we follow the table based on footer->creator_app:
+ *
+ *  Currently known creator apps:
+ *      'vpc '  :  CHS              Virtual PC (uses disk geometry)
+ *      'qemu'  :  CHS              QEMU (uses disk geometry)
+ *      'qem2'  :  current_size     QEMU (uses current_size)
+ *      'win '  :  current_size     Hyper-V
+ *      'd2v '  :  current_size     Disk2vhd
+ *      'tap\0' :  current_size     XenServer
+ *      'CTXS'  :  current_size     XenConverter
+ *      'wa\0\0':  current_size     Azure
+ *
+ *  The user can override the table values via drive options, however
+ *  even with an override we will still use current_size for images
+ *  that have CHS geometry of the maximum size.
+ */
+static bool vpc_ignore_current_size(VHDFooter *footer)
+{
+    return !strncmp(footer->creator_app, "vpc ", 4) ||
+        !strncmp(footer->creator_app, "qemu", 4);
+}
+
 static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = (int64_t)
         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
 
-    /* Microsoft Virtual PC and Microsoft Hyper-V produce and read
-     * VHD image sizes differently.  VPC will rely on CHS geometry,
-     * while Hyper-V and disk2vhd use the size specified in the footer.
-     *
-     * We use a couple of approaches to try and determine the correct method:
-     * look at the Creator App field, and look for images that have CHS
-     * geometry that is the maximum value.
-     *
-     * If the CHS geometry is the maximum CHS geometry, then we assume that
-     * the size is the footer->current_size to avoid truncation.  Otherwise,
-     * we follow the table based on footer->creator_app:
-     *
-     *  Known creator apps:
-     *      'vpc '  :  CHS              Virtual PC (uses disk geometry)
-     *      'qemu'  :  CHS              QEMU (uses disk geometry)
-     *      'qem2'  :  current_size     QEMU (uses current_size)
-     *      'win '  :  current_size     Hyper-V
-     *      'd2v '  :  current_size     Disk2vhd
-     *      'tap\0' :  current_size     XenServer
-     *      'CTXS'  :  current_size     XenConverter
-     *
-     *  The user can override the table values via drive options, however
-     *  even with an override we will still use current_size for images
-     *  that have CHS geometry of the maximum size.
-     */
-    use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
-               !!strncmp(footer->creator_app, "qem2", 4) &&
-               !!strncmp(footer->creator_app, "d2v ", 4) &&
-               !!strncmp(footer->creator_app, "CTXS", 4) &&
-               !!memcmp(footer->creator_app, "tap", 4)) || s->force_use_chs;
+    /* Use CHS or current_size to determine the image size */
+    use_chs = vpc_ignore_current_size(footer) || s->force_use_chs;
 
     if (!use_chs || bs->total_sectors == VHD_MAX_GEOMETRY || s->force_use_sz) {
         bs->total_sectors = be64_to_cpu(footer->current_size) /
-- 
2.47.0