[PATCH v4 0/2] vpc: Read images exported from Azure correctly

[PATCH v4 0/2] vpc: Read images exported from Azure correctly

[Vitaly Kuznetsov]

Changes since v3:
- Split the change into two patches [Philippe Mathieu-Daude].

It was found that 'qemu-nbd' is not able to work with some disk images
exported from Azure as it uses a currently unknown 'wa\0\0' 'creator app'
signature. QEMU currently supports two methods for determining the image
size: CHS and 'current_size' and the list of known 'creator app's is used
to decide between the two. Invert the logic in QEMU and make 'current_size'
the default as it seems that VPC and old QEMU are the only two legacy apps
where preferring CHS makes sense.

Vitaly Kuznetsov (2):
  vpc: Split off vpc_ignore_current_size() helper
  vpc: Read images exported from Azure correctly

 block/vpc.c | 65 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

-- 
2.47.0

[PATCH v4 1/2] vpc: Split off vpc_ignore_current_size() helper

[Vitaly Kuznetsov]

In preparation to making changes to the logic deciding whether CHS or
'current_size' need to be used in determining the image size, split off
vpc_ignore_current_size() helper.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 block/vpc.c | 67 +++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index d95a204612b7..fb0ded1c4344 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -216,6 +216,41 @@ 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:
+ *
+ *  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.
+ */
+static bool vpc_ignore_current_size(VHDFooter *footer)
+{
+    return !!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));
+}
+
 static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -304,36 +339,8 @@ 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

Re: [PATCH v4 1/2] vpc: Split off vpc_ignore_current_size() helper

[Philippe Mathieu-Daudé]

On 12/12/24 14:45, Vitaly Kuznetsov wrote:
> In preparation to making changes to the logic deciding whether CHS or
> 'current_size' need to be used in determining the image size, split off
> vpc_ignore_current_size() helper.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>   block/vpc.c | 67 +++++++++++++++++++++++++++++------------------------
>   1 file changed, 37 insertions(+), 30 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v4 2/2] vpc: Read images exported from Azure correctly

[Vitaly Kuznetsov]

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.

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>
---
 block/vpc.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index fb0ded1c4344..a5f626baf04a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -237,6 +237,7 @@ static void vpc_parse_options(BlockDriverState *bs, QemuOpts *opts,
  *      '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
@@ -244,11 +245,8 @@ static void vpc_parse_options(BlockDriverState *bs, QemuOpts *opts,
  */
 static bool vpc_ignore_current_size(VHDFooter *footer)
 {
-    return !!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));
+    return !strncmp(footer->creator_app, "vpc ", 4) ||
+           !strncmp(footer->creator_app, "qemu", 4);
 }
 
 static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
-- 
2.47.0

Re: [PATCH v4 0/2] vpc: Read images exported from Azure correctly

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Changes since v3:
> - Split the change into two patches [Philippe Mathieu-Daude].
>
> It was found that 'qemu-nbd' is not able to work with some disk images
> exported from Azure as it uses a currently unknown 'wa\0\0' 'creator app'
> signature. QEMU currently supports two methods for determining the image
> size: CHS and 'current_size' and the list of known 'creator app's is used
> to decide between the two. Invert the logic in QEMU and make 'current_size'
> the default as it seems that VPC and old QEMU are the only two legacy apps
> where preferring CHS makes sense.
>
> Vitaly Kuznetsov (2):
>   vpc: Split off vpc_ignore_current_size() helper
>   vpc: Read images exported from Azure correctly

Ping?

-- 
Vitaly

Re: [PATCH v4 0/2] vpc: Read images exported from Azure correctly

[Kevin Wolf]

Am 12.12.2024 um 14:45 hat Vitaly Kuznetsov geschrieben:
> Changes since v3:
> - Split the change into two patches [Philippe Mathieu-Daude].
> 
> It was found that 'qemu-nbd' is not able to work with some disk images
> exported from Azure as it uses a currently unknown 'wa\0\0' 'creator app'
> signature. QEMU currently supports two methods for determining the image
> size: CHS and 'current_size' and the list of known 'creator app's is used
> to decide between the two. Invert the logic in QEMU and make 'current_size'
> the default as it seems that VPC and old QEMU are the only two legacy apps
> where preferring CHS makes sense.
> 
> Vitaly Kuznetsov (2):
>   vpc: Split off vpc_ignore_current_size() helper
>   vpc: Read images exported from Azure correctly

Thanks, applied to the block branch.

Kevin