| Message ID | 20241118143646.33377-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] vpc: Read images exported from Azure correctly | expand |
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 <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.
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) &&
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(+)