* [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header @ 2011-11-08 19:25 Charles Arnold 2011-11-08 20:16 ` Andreas Färber 2011-11-09 9:46 ` Kevin Wolf 0 siblings, 2 replies; 9+ messages in thread From: Charles Arnold @ 2011-11-08 19:25 UTC (permalink / raw) To: qemu-devel The Data Offset field in the Dynamic Disk Header is an 8 byte field. Although the specification (2006-10-11) gives an example of initializing only the first 4 bytes, images generated by Microsoft on Windows initialize all 8 bytes. Failure to initialize all 8 bytes results in errors from utilities that check specifically for the complete Data Offset field initialization. Signed-off-by: Charles Arnold <carnold@suse.com> diff --git a/block/vpc.c b/block/vpc.c index 416f489..35ac3fd 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) memcpy(dyndisk_header->magic, "cxsparse", 8); - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); dyndisk_header->table_offset = be64_to_cpu(3 * 512); dyndisk_header->version = be32_to_cpu(0x00010000); dyndisk_header->block_size = be32_to_cpu(block_size); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-08 19:25 [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header Charles Arnold @ 2011-11-08 20:16 ` Andreas Färber 2011-11-09 9:46 ` Kevin Wolf 1 sibling, 0 replies; 9+ messages in thread From: Andreas Färber @ 2011-11-08 20:16 UTC (permalink / raw) To: Kevin Wolf; +Cc: Charles Arnold, qemu-devel Am 08.11.2011 20:25, schrieb Charles Arnold: > The Data Offset field in the Dynamic Disk Header is an 8 byte field. > Although the specification (2006-10-11) gives an example of initializing > only the first 4 bytes, images generated by Microsoft on Windows initialize > all 8 bytes. > > Failure to initialize all 8 bytes results in errors from utilities > that check specifically for the complete Data Offset field initialization. > > Signed-off-by: Charles Arnold <carnold@suse.com> Reviewed-by: Andreas Färber <afaerber@suse.de> Kevin, can you please apply this for 1.0? Thanks! Andreas > > diff --git a/block/vpc.c b/block/vpc.c > index 416f489..35ac3fd 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) > > memcpy(dyndisk_header->magic, "cxsparse", 8); > > - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); > + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); > dyndisk_header->table_offset = be64_to_cpu(3 * 512); > dyndisk_header->version = be32_to_cpu(0x00010000); > dyndisk_header->block_size = be32_to_cpu(block_size); -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-08 19:25 [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header Charles Arnold 2011-11-08 20:16 ` Andreas Färber @ 2011-11-09 9:46 ` Kevin Wolf 2011-11-09 15:42 ` Charles Arnold 1 sibling, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2011-11-09 9:46 UTC (permalink / raw) To: Charles Arnold; +Cc: qemu-devel Am 08.11.2011 20:25, schrieb Charles Arnold: > The Data Offset field in the Dynamic Disk Header is an 8 byte field. > Although the specification (2006-10-11) gives an example of initializing > only the first 4 bytes, images generated by Microsoft on Windows initialize > all 8 bytes. > > Failure to initialize all 8 bytes results in errors from utilities > that check specifically for the complete Data Offset field initialization. > > Signed-off-by: Charles Arnold <carnold@suse.com> > > diff --git a/block/vpc.c b/block/vpc.c > index 416f489..35ac3fd 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) > > memcpy(dyndisk_header->magic, "cxsparse", 8); > > - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); > + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); > dyndisk_header->table_offset = be64_to_cpu(3 * 512); > dyndisk_header->version = be32_to_cpu(0x00010000); > dyndisk_header->block_size = be32_to_cpu(block_size); Can you please add a short comment explaining why we deviate from the specification in this point? If someone notices the discrepancy later, he shouldn't have to dig up the git commit message. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 9:46 ` Kevin Wolf @ 2011-11-09 15:42 ` Charles Arnold 2011-11-09 15:58 ` Kevin Wolf 2011-11-09 16:00 ` Andreas Färber 0 siblings, 2 replies; 9+ messages in thread From: Charles Arnold @ 2011-11-09 15:42 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel >>> On 11/9/2011 at 02:46 AM, in message <4EBA4BEC.5040401@redhat.com>, Kevin Wolf <kwolf@redhat.com> wrote: > Am 08.11.2011 20:25, schrieb Charles Arnold: >> The Data Offset field in the Dynamic Disk Header is an 8 byte field. >> Although the specification (2006-10-11) gives an example of initializing >> only the first 4 bytes, images generated by Microsoft on Windows initialize >> all 8 bytes. >> >> Failure to initialize all 8 bytes results in errors from utilities >> that check specifically for the complete Data Offset field initialization. >> >> Signed-off-by: Charles Arnold <carnold@suse.com> >> >> diff --git a/block/vpc.c b/block/vpc.c >> index 416f489..35ac3fd 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, > QEMUOptionParameter *options) >> >> memcpy(dyndisk_header->magic, "cxsparse", 8); >> >> - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); >> + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); >> dyndisk_header->table_offset = be64_to_cpu(3 * 512); >> dyndisk_header->version = be32_to_cpu(0x00010000); >> dyndisk_header->block_size = be32_to_cpu(block_size); > > Can you please add a short comment explaining why we deviate from the > specification in this point? If someone notices the discrepancy later, > he shouldn't have to dig up the git commit message. > > Kevin Using qemu-img to convert a raw image to a vhd image seems to work fine accept that only 4 bytes of the 8 byte Dynamic Disk Header Data Offset field are initialized. This seems correct according to the specification but tools like Citrix's vhd-util fail when querying the image due to the shortened initialization. Upon further analysis, it was determined that Microsoft also initializes all 8 bytes when generating a vhd image on windows. Initializing all 8 bytes seems more logical and may indicate that Microsoft needs to update the specification because the requirement for this field changed or that it is simply an oversight that has never been noticed. - Charles ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 15:42 ` Charles Arnold @ 2011-11-09 15:58 ` Kevin Wolf 2011-11-09 16:32 ` Charles Arnold 2011-11-09 16:00 ` Andreas Färber 1 sibling, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2011-11-09 15:58 UTC (permalink / raw) To: Charles Arnold; +Cc: qemu-devel Am 09.11.2011 16:42, schrieb Charles Arnold: >>>> On 11/9/2011 at 02:46 AM, in message <4EBA4BEC.5040401@redhat.com>, Kevin Wolf > <kwolf@redhat.com> wrote: >> Am 08.11.2011 20:25, schrieb Charles Arnold: >>> The Data Offset field in the Dynamic Disk Header is an 8 byte field. >>> Although the specification (2006-10-11) gives an example of initializing >>> only the first 4 bytes, images generated by Microsoft on Windows initialize >>> all 8 bytes. >>> >>> Failure to initialize all 8 bytes results in errors from utilities >>> that check specifically for the complete Data Offset field initialization. >>> >>> Signed-off-by: Charles Arnold <carnold@suse.com> >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 416f489..35ac3fd 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, >> QEMUOptionParameter *options) >>> >>> memcpy(dyndisk_header->magic, "cxsparse", 8); >>> >>> - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); >>> + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); >>> dyndisk_header->table_offset = be64_to_cpu(3 * 512); >>> dyndisk_header->version = be32_to_cpu(0x00010000); >>> dyndisk_header->block_size = be32_to_cpu(block_size); >> >> Can you please add a short comment explaining why we deviate from the >> specification in this point? If someone notices the discrepancy later, >> he shouldn't have to dig up the git commit message. >> >> Kevin > > Using qemu-img to convert a raw image to a vhd image seems to work fine accept that only 4 bytes of the 8 byte Dynamic Disk Header Data Offset field are initialized. This seems correct according to the specification but tools like Citrix's vhd-util fail when querying the image due to the shortened initialization. Upon further analysis, it was determined that Microsoft also initializes all 8 bytes when generating a vhd image on windows. Initializing all 8 bytes seems more logical and may indicate that Microsoft needs to update the specification because the requirement for this field changed or that it is simply an oversight that has never been noticed. Right, I'm not arguing that your change is wrong. What I'm asking for is just something like: - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); + /* Note: The spec is actually wrong here, it says 0xFFFFFFFF, but MS tools expect all 64 bits to be set */ + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 15:58 ` Kevin Wolf @ 2011-11-09 16:32 ` Charles Arnold 2011-11-09 16:38 ` Andreas Färber 2011-11-09 16:57 ` Kevin Wolf 0 siblings, 2 replies; 9+ messages in thread From: Charles Arnold @ 2011-11-09 16:32 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel >>> On 11/9/2011 at 08:58 AM, in message <4EBAA30D.1090209@redhat.com>, Kevin Wolf <kwolf@redhat.com> wrote: > > Right, I'm not arguing that your change is wrong. What I'm asking for is > just something like: > > - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); > + /* Note: The spec is actually wrong here, it says 0xFFFFFFFF, but > MS tools expect all 64 bits to be set */ > + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); > > Kevin Ok, I had the header comment on the brain, not a code comment. Here is the revised patch. - Charles The Data Offset field in the Dynamic Disk Header is an 8 byte field. Although the specification (2006-10-11) gives an example of initializing only the first 4 bytes, images generated by Microsoft on Windows initialize all 8 bytes. Failure to initialize all 8 bytes results in errors from utilities like Citrix's vhd-util which checks specifically for the proper Data Offset field initialization. Signed-off-by: Charles Arnold <carnold@suse.com> diff --git a/block/vpc.c b/block/vpc.c index 416f489..179c6ae 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -585,7 +585,11 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) memcpy(dyndisk_header->magic, "cxsparse", 8); - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); + /* + * Note: The spec is actually wrong here for data_offset, it says + * 0xFFFFFFFF, but MS tools expect all 64 bits to be set. + */ + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); dyndisk_header->table_offset = be64_to_cpu(3 * 512); dyndisk_header->version = be32_to_cpu(0x00010000); dyndisk_header->block_size = be32_to_cpu(block_size); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 16:32 ` Charles Arnold @ 2011-11-09 16:38 ` Andreas Färber 2011-11-09 16:57 ` Kevin Wolf 1 sibling, 0 replies; 9+ messages in thread From: Andreas Färber @ 2011-11-09 16:38 UTC (permalink / raw) To: Kevin Wolf; +Cc: Charles Arnold, qemu-devel Am 09.11.2011 17:32, schrieb Charles Arnold: >>>> On 11/9/2011 at 08:58 AM, in message <4EBAA30D.1090209@redhat.com>, Kevin Wolf > <kwolf@redhat.com> wrote: >> >> Right, I'm not arguing that your change is wrong. What I'm asking for is >> just something like: >> >> - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); >> + /* Note: The spec is actually wrong here, it says 0xFFFFFFFF, but >> MS tools expect all 64 bits to be set */ >> + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); >> >> Kevin > > Ok, I had the header comment on the brain, not a code comment. > Here is the revised patch. > > - Charles > > > The Data Offset field in the Dynamic Disk Header is an 8 byte field. > Although the specification (2006-10-11) gives an example of initializing > only the first 4 bytes, images generated by Microsoft on Windows initialize > all 8 bytes. > > Failure to initialize all 8 bytes results in errors from utilities > like Citrix's vhd-util which checks specifically for the proper Data > Offset field initialization. > > Signed-off-by: Charles Arnold <carnold@suse.com> Reviewed-by: Andreas Färber <afaerber@suse.de> Andreas > > diff --git a/block/vpc.c b/block/vpc.c > index 416f489..179c6ae 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -585,7 +585,11 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) > > memcpy(dyndisk_header->magic, "cxsparse", 8); > > - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); > + /* > + * Note: The spec is actually wrong here for data_offset, it says > + * 0xFFFFFFFF, but MS tools expect all 64 bits to be set. > + */ > + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); > dyndisk_header->table_offset = be64_to_cpu(3 * 512); > dyndisk_header->version = be32_to_cpu(0x00010000); > dyndisk_header->block_size = be32_to_cpu(block_size); -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 16:32 ` Charles Arnold 2011-11-09 16:38 ` Andreas Färber @ 2011-11-09 16:57 ` Kevin Wolf 1 sibling, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2011-11-09 16:57 UTC (permalink / raw) To: Charles Arnold; +Cc: qemu-devel Am 09.11.2011 17:32, schrieb Charles Arnold: >>>> On 11/9/2011 at 08:58 AM, in message <4EBAA30D.1090209@redhat.com>, Kevin Wolf > <kwolf@redhat.com> wrote: >> >> Right, I'm not arguing that your change is wrong. What I'm asking for is >> just something like: >> >> - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); >> + /* Note: The spec is actually wrong here, it says 0xFFFFFFFF, but >> MS tools expect all 64 bits to be set */ >> + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); >> >> Kevin > > Ok, I had the header comment on the brain, not a code comment. > Here is the revised patch. > > - Charles > > > The Data Offset field in the Dynamic Disk Header is an 8 byte field. > Although the specification (2006-10-11) gives an example of initializing > only the first 4 bytes, images generated by Microsoft on Windows initialize > all 8 bytes. > > Failure to initialize all 8 bytes results in errors from utilities > like Citrix's vhd-util which checks specifically for the proper Data > Offset field initialization. > > Signed-off-by: Charles Arnold <carnold@suse.com> Thanks, applied to the block-stable branch (for 1.0) Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header 2011-11-09 15:42 ` Charles Arnold 2011-11-09 15:58 ` Kevin Wolf @ 2011-11-09 16:00 ` Andreas Färber 1 sibling, 0 replies; 9+ messages in thread From: Andreas Färber @ 2011-11-09 16:00 UTC (permalink / raw) To: Charles Arnold; +Cc: Kevin Wolf, qemu-devel Am 09.11.2011 16:42, schrieb Charles Arnold: >>>> On 11/9/2011 at 02:46 AM, in message <4EBA4BEC.5040401@redhat.com>, Kevin Wolf > <kwolf@redhat.com> wrote: >> Am 08.11.2011 20:25, schrieb Charles Arnold: >>> The Data Offset field in the Dynamic Disk Header is an 8 byte field. >>> Although the specification (2006-10-11) gives an example of initializing >>> only the first 4 bytes, images generated by Microsoft on Windows initialize >>> all 8 bytes. >>> >>> Failure to initialize all 8 bytes results in errors from utilities >>> that check specifically for the complete Data Offset field initialization. >>> >>> Signed-off-by: Charles Arnold <carnold@suse.com> >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 416f489..35ac3fd 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -585,7 +585,7 @@ static int vpc_create(const char *filename, >> QEMUOptionParameter *options) >>> >>> memcpy(dyndisk_header->magic, "cxsparse", 8); >>> >>> - dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFF); >>> + dyndisk_header->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL); >>> dyndisk_header->table_offset = be64_to_cpu(3 * 512); >>> dyndisk_header->version = be32_to_cpu(0x00010000); >>> dyndisk_header->block_size = be32_to_cpu(block_size); >> >> Can you please add a short comment explaining why we deviate from the >> specification in this point? If someone notices the discrepancy later, >> he shouldn't have to dig up the git commit message. >> >> Kevin > > Using qemu-img to convert a raw image to a vhd image seems to work fine accept that only 4 bytes of the 8 byte Dynamic Disk Header Data Offset field are initialized. This seems correct according to the specification but tools like Citrix's vhd-util fail when querying the image due to the shortened initialization. Upon further analysis, it was determined that Microsoft also initializes all 8 bytes when generating a vhd image on windows. Initializing all 8 bytes seems more logical and may indicate that Microsoft needs to update the specification because the requirement for this field changed or that it is simply an oversight that has never been noticed. Charles, I think Kevin meant inserting a C comment above that line. :) Maybe: /* Microsoft and Citrix tools expect data_offset to be eight bytes 0xff although this does not match the spec. */ Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-11-09 16:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-08 19:25 [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header Charles Arnold 2011-11-08 20:16 ` Andreas Färber 2011-11-09 9:46 ` Kevin Wolf 2011-11-09 15:42 ` Charles Arnold 2011-11-09 15:58 ` Kevin Wolf 2011-11-09 16:32 ` Charles Arnold 2011-11-09 16:38 ` Andreas Färber 2011-11-09 16:57 ` Kevin Wolf 2011-11-09 16:00 ` Andreas Färber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).