[v2] vpc: Read images exported from Azure correctly

[PATCH v2] vpc: Read images exported from Azure correctly

Posted by Vitaly Kuznetsov 1 year, 2 months ago

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 d95a204612b7..b67798697c15 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -321,6 +321,7 @@ 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
@@ -330,6 +331,7 @@ 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

Re: [PATCH v2] vpc: Read images exported from Azure correctly

Posted by Eric Blake 1 year, 2 months ago

On Mon, Nov 18, 2024 at 03:36:46PM +0100, Vitaly Kuznetsov wrote:
> 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 d95a204612b7..b67798697c15 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -321,6 +321,7 @@ 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
> @@ -330,6 +331,7 @@ 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) &&

While this is literally correct (a string literal with 3 characters
spelled out includes the implicit NUL byte; sizeof("wa\0") == 4), it
is a bit odd to see a memcmp() of 4 bytes against a literal containing
only 3 characters, especially when the comments above spelled it out
with four characters.  For the sake of avoiding further confusion, it
might be nice to use memcmp() against explicit 4-byte patterns for all
of the strings (not just the Azure witness).

>                 !!strncmp(footer->creator_app, "qem2", 4) &&
>                 !!strncmp(footer->creator_app, "d2v ", 4) &&
>                 !!strncmp(footer->creator_app, "CTXS", 4) &&

I also don't know if it would be any easier to read by creating a
`static const char table[][4] = { "qemu", "qem2", "wa", ...}` (where
you don't have to write any explicit \0, because the compiler is
guaranteed to NUL-pad short strings into the char[4] table entry) and
then write a loop over each entry in the table, rather than having to
spell out a separate strncmp/memcmp line for each string in the table.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2] vpc: Read images exported from Azure correctly

Posted by Vitaly Kuznetsov 1 year, 2 months ago

Eric Blake <eblake@redhat.com> writes:

> On Mon, Nov 18, 2024 at 03:36:46PM +0100, Vitaly Kuznetsov wrote:
>> 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 d95a204612b7..b67798697c15 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -321,6 +321,7 @@ 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
>> @@ -330,6 +331,7 @@ 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) &&
>
> While this is literally correct (a string literal with 3 characters
> spelled out includes the implicit NUL byte; sizeof("wa\0") == 4), it
> is a bit odd to see a memcmp() of 4 bytes against a literal containing
> only 3 characters, especially when the comments above spelled it out
> with four characters.  For the sake of avoiding further confusion, it
> might be nice to use memcmp() against explicit 4-byte patterns for all
> of the strings (not just the Azure witness).

Yea, it's just that we already have

    !!memcmp(footer->creator_app, "tap", 4))

down below so I decided to stick to the style :-)

>
>>                 !!strncmp(footer->creator_app, "qem2", 4) &&
>>                 !!strncmp(footer->creator_app, "d2v ", 4) &&
>>                 !!strncmp(footer->creator_app, "CTXS", 4) &&
>
> I also don't know if it would be any easier to read by creating a
> `static const char table[][4] = { "qemu", "qem2", "wa", ...}` (where
> you don't have to write any explicit \0, because the compiler is
> guaranteed to NUL-pad short strings into the char[4] table entry) and
> then write a loop over each entry in the table, rather than having to
> spell out a separate strncmp/memcmp line for each string in the table.

I like the idea but I'm still trying to understand whether we need to
keep adding new entries there or just flip the default and say that only
'vpc ' and 'qemu' are legacy and deserve CHS.

-- 
Vitaly