Re: [Qemu-devel] [RFC PATCH] vpc: Ignore geometry for large images

[Charles Arnold]

 On 2/12/2015 at 12:05 PM, Peter Lieven p...@kamp.de wrote: 

 Am 12.02.2015 um 18:18 schrieb Charles Arnold carn...@suse.com:
 
 On 2/12/2015 at 03:23 AM, Kevin Wolf kw...@redhat.com wrote:
 Am 12.02.2015 um 11:09 hat Peter Lieven geschrieben:
 Am 12.02.2015 um 11:06 schrieb Kevin Wolf:
 Am 12.02.2015 um 11:02 hat Peter Lieven geschrieben:
 Am 12.02.2015 um 10:58 schrieb Kevin Wolf:
 Am 12.02.2015 um 10:23 hat Peter Lieven geschrieben:
 Am 10.02.2015 um 15:53 schrieb Kevin Wolf:
 Am 10.02.2015 um 15:00 hat Peter Lieven geschrieben:
 Am 10.02.2015 um 14:54 schrieb Kevin Wolf:
 Am 10.02.2015 um 14:42 hat Jeff Cody geschrieben:
 On Tue, Feb 10, 2015 at 02:34:14PM +0100, Kevin Wolf wrote:
 Am 10.02.2015 um 12:41 hat Peter Lieven geschrieben:
 Am 09.02.2015 um 17:09 schrieb Kevin Wolf:
 The CHS calculation as done per the VHD spec imposes a maximum
 image size of ~127 GB. Real VHD images exist that are larger 
 than
 that.
 
 Apparently there are two separate non-standard ways to achieve
 this: You could use more heads than the spec does - this is the
 option that qemu-img create chooses.
 
 However, other images exist where the geometry is set to the
 maximum (65536/16/255), but the actual image size is larger.
 Until now, such images are truncated at 127 GB when opening them
 with qemu.
 
 This patch changes the vpc driver to ignore geometry in this 
 case
 and only trust the size field in the header.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com ---
 
 Peter, I'm replacing some of your code in the hope that the new
 approach is more generally valid. Of course, I haven't tested if
 your case with disk2vhd is still covered. Could you check this,
 please?
 I checked this and found that disk2vhd always sets CHS to 
 65535ULL
 * 16 * 255 independed of the real size.
 
 But, as the conversion to CHS may have an error its maybe the 
 best
 solution to ignore CHS completely and always derive total_sectors
 from footer-size unconditionally.
 I had a look at what virtualbox does and they only rely on
 footer-size. If they alter the size or create an image the write
 the new size into the footer and recalculate CHS by the formula
 found in the appendix of the original spec.
 
 Check vhdCreateImage, vhdOpen in
 http://www.virtualbox.org/svn/vbox/trunk/src/VBox/Storage/VHD.cpp
 
 The original spec also says that CHS values purpose is the use in
 an ATA controller only.
 The problem with just using footer-size back then when I
 implemented this was that from the perspective of a VirtualPC 
 guest
 run in qemu, the size of its hard disk would change, which you 
 don't
 want either. Going from VPC to qemu would be ugly, but mostly
 harmless as the disk only grows. But if you use an image in qemu
 where the disk looks larger and then go back to VPC which respects
 geometry, your data may be truncated.
 I believe the vpc creator field is different if the image was
 created by Virtual PC, versus created by Hyper-V (vpc and win,
 respectively, I think).  Perhaps we could use that to infer a guest
 image came from VirtualPC, and thus not use footer-size in that
 scenario?
 Right, I think we discussed that before. Do you remember the 
 outcome of
 that discussion? I seem to remember that we had a conclusion, but
 apparently it was never actually implemented.
 
 Would your proposal be to special-case vpc to apply the geometry, 
 and
 everything else (including win, d2v and qemu) would use the 
 footer
 field?
 That sounds reasonable. In any case we have to fix qemu-img create
 to do not create out of spec geometry for images larger than 127G.
 It should set the correct footer-size and then calculate the 
 geometry.
 Do I understand correctly that you just volunteered to fix up that 
 whole
 thing? ;-)
 I knew that this would happen ;-)
 
 Regarding the C/H/S calculation. I was just wondering if we should
 not set this to maximum (=invalid?) for all newly created images.
 That is what disk2vhd does.
 CHS is what Virtual PC relies on. So I guess if you did that, you
 would render images unusable by it. Are you sure that disk2vhd does this
 always? I would have thought that it only does it for large images.
 At least 2.0.1 (latest available version) does this as well as the 
 version
 that I used when I added the hack for d2v creator.
 
 Virtual PC would not be able to use images we create with qemu-img create
 if we use footer-size (which I suppose to reanme to footer-cur_size, 
 btw)
 to calculate bs-total_sectors because we might write data to the end of
 the image which gets truncated in CHS format.
 These kinds of problems are why I'd like to keep CHS and size always
 consistent when creating an image with qemu-img.
 
 Okay, then I would vote for your RFC patch + fixing qemu-img create
 to not generate out of spec CHS values and just set maximum which
 then would make vpc_open use footer-size.
 
 Really the RFC patch or what we discussed above (vpc creator = CHS,
 everything else = footer-size)? Once I know what we prefer

Re: [Qemu-devel] [RFC PATCH] vpc: Ignore geometry for large images

[Charles Arnold]

 On 2/12/2015 at 03:23 AM, Kevin Wolf kw...@redhat.com wrote: 
 Am 12.02.2015 um 11:09 hat Peter Lieven geschrieben:
 Am 12.02.2015 um 11:06 schrieb Kevin Wolf:
 Am 12.02.2015 um 11:02 hat Peter Lieven geschrieben:
 Am 12.02.2015 um 10:58 schrieb Kevin Wolf:
 Am 12.02.2015 um 10:23 hat Peter Lieven geschrieben:
 Am 10.02.2015 um 15:53 schrieb Kevin Wolf:
 Am 10.02.2015 um 15:00 hat Peter Lieven geschrieben:
 Am 10.02.2015 um 14:54 schrieb Kevin Wolf:
 Am 10.02.2015 um 14:42 hat Jeff Cody geschrieben:
 On Tue, Feb 10, 2015 at 02:34:14PM +0100, Kevin Wolf wrote:
 Am 10.02.2015 um 12:41 hat Peter Lieven geschrieben:
 Am 09.02.2015 um 17:09 schrieb Kevin Wolf:
 The CHS calculation as done per the VHD spec imposes a maximum
 image size of ~127 GB. Real VHD images exist that are larger than
 that.
 
 Apparently there are two separate non-standard ways to achieve
 this: You could use more heads than the spec does - this is the
 option that qemu-img create chooses.
 
 However, other images exist where the geometry is set to the
 maximum (65536/16/255), but the actual image size is larger.
 Until now, such images are truncated at 127 GB when opening them
 with qemu.
 
 This patch changes the vpc driver to ignore geometry in this case
 and only trust the size field in the header.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com ---
 
 Peter, I'm replacing some of your code in the hope that the new
 approach is more generally valid. Of course, I haven't tested if
 your case with disk2vhd is still covered. Could you check this,
 please?
 I checked this and found that disk2vhd always sets CHS to 65535ULL
 * 16 * 255 independed of the real size.
 
 But, as the conversion to CHS may have an error its maybe the best
 solution to ignore CHS completely and always derive total_sectors
 from footer-size unconditionally.
 I had a look at what virtualbox does and they only rely on
 footer-size. If they alter the size or create an image the write
 the new size into the footer and recalculate CHS by the formula
 found in the appendix of the original spec.
 
 Check vhdCreateImage, vhdOpen in
 http://www.virtualbox.org/svn/vbox/trunk/src/VBox/Storage/VHD.cpp
 
 The original spec also says that CHS values purpose is the use in
 an ATA controller only.
 The problem with just using footer-size back then when I
 implemented this was that from the perspective of a VirtualPC guest
 run in qemu, the size of its hard disk would change, which you don't
 want either. Going from VPC to qemu would be ugly, but mostly
 harmless as the disk only grows. But if you use an image in qemu
 where the disk looks larger and then go back to VPC which respects
 geometry, your data may be truncated.
 I believe the vpc creator field is different if the image was
 created by Virtual PC, versus created by Hyper-V (vpc and win,
 respectively, I think).  Perhaps we could use that to infer a guest
 image came from VirtualPC, and thus not use footer-size in that
 scenario?
 Right, I think we discussed that before. Do you remember the outcome 
 of
 that discussion? I seem to remember that we had a conclusion, but
 apparently it was never actually implemented.
 
 Would your proposal be to special-case vpc to apply the geometry, 
 and
 everything else (including win, d2v and qemu) would use the 
 footer
 field?
 That sounds reasonable. In any case we have to fix qemu-img create
 to do not create out of spec geometry for images larger than 127G.
 It should set the correct footer-size and then calculate the geometry.
 Do I understand correctly that you just volunteered to fix up that whole
 thing? ;-)
 I knew that this would happen ;-)
 
 Regarding the C/H/S calculation. I was just wondering if we should
 not set this to maximum (=invalid?) for all newly created images.
 That is what disk2vhd does.
 CHS is what Virtual PC relies on. So I guess if you did that, you
 would render images unusable by it. Are you sure that disk2vhd does this
 always? I would have thought that it only does it for large images.
 At least 2.0.1 (latest available version) does this as well as the version
 that I used when I added the hack for d2v creator.
 
 Virtual PC would not be able to use images we create with qemu-img create
 if we use footer-size (which I suppose to reanme to footer-cur_size, btw)
 to calculate bs-total_sectors because we might write data to the end of
 the image which gets truncated in CHS format.
 These kinds of problems are why I'd like to keep CHS and size always
 consistent when creating an image with qemu-img.
 
 Okay, then I would vote for your RFC patch + fixing qemu-img create
 to not generate out of spec CHS values and just set maximum which
 then would make vpc_open use footer-size.
 
 Really the RFC patch or what we discussed above (vpc creator = CHS,
 everything else = footer-size)? Once I know what we prefer, I'll send
 the real patch.
 
 As for heads  16, that would essentially mean reverting 258d2edb.
 Should be easy to do, the harder part is probably the 

Re: [Qemu-devel] [PATCH] block: vpc support for ~2 TB disks

[Charles Arnold]

 On 11/14/2012 at 09:35 AM, in message 50a3c853.4010...@redhat.com, Paolo
Bonzini pbonz...@redhat.com wrote: 
 Il 14/11/2012 17:25, Thanos Makatos ha scritto:
 We don't use qemu's VHD driver in XenServer. Instead, we use blktap2
 to create a block device in dom0 serving the VHD file in question,
 and have qemu open that block device instead of the VHD file itself.
 
 Yes, the question is how you handle disks bigger than 127GB, so that
 QEMU can do the same.
 

In analyzing a 160 GB VHD fixed disk image created on Windows 2008 R2, it 
appears that
MS is also ignoring the CHS values in the footer geometry field in whatever
driver they use for accessing the image.  The CHS values are set at 65535,16,255
which obviously doesn't represent an image size of 160 GB.

This patch only extends the existing qemu driver to allow a larger image by 
allowing
more heads.  On real hardware, only 4 bits would be allowed for heads but we 
don't
have that restriction in qemu. 

- Charles

Re: [Qemu-devel] [PATCH] block: vpc support for ~2 TB disks

[Charles Arnold]

Ping?

Any thoughts on whether this is acceptable?

- Charles

 On 10/30/2012 at 08:59 PM, in message 50a0e561.5b74.009...@suse.com, 
 Charles
Arnold wrote: 
 The VHD specification allows for up to a 2 TB disk size. The current
 implementation in qemu emulates EIDE and ATA-2 hardware which only allows
 for up to 127 GB.  This disk size limitation can be overridden by allowing
 up to 255 heads instead of the normal 4 bit limitation of 16.  Doing so
 allows disk images to be created of up to nearly 2 TB.  This change does
 not violate the VHD format specification nor does it change how smaller
 disks (ie, =127GB) are defined. 
 
 Signed-off-by: Charles Arnold carn...@suse.com
 
 diff --git a/block/vpc.c b/block/vpc.c
 index b6bf52f..0c2eaf8 100644
 --- a/block/vpc.c
 +++ b/block/vpc.c
 @@ -198,7 +198,8 @@ static int vpc_open(BlockDriverState *bs, int flags)
  bs-total_sectors = (int64_t)
  be16_to_cpu(footer-cyls) * footer-heads * footer-secs_per_cyl;
  
 -if (bs-total_sectors = 65535 * 16 * 255) {
 +/* Allow a maximum disk size of approximately 2 TB */
 +if (bs-total_sectors = 65535LL * 255 * 255) {qemu-devel@nongnu.org
  err = -EFBIG;
  goto fail;
  }
 @@ -524,19 +525,27 @@ static coroutine_fn int vpc_co_write(BlockDriverState 
 *bs, int64_t sector_num,
   * Note that the geometry doesn't always exactly match total_sectors but
   * may round it down.
   *
 - * Returns 0 on success, -EFBIG if the size is larger than 127 GB
 + * Returns 0 on success, -EFBIG if the size is larger than ~2 TB. Override
 + * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB)
 + * and instead allow up to 255 heads.
   */
  static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
  uint8_t* heads, uint8_t* secs_per_cyl)
  {
  uint32_t cyls_times_heads;
  
 -if (total_sectors  65535 * 16 * 255)
 +/* Allow a maximum disk size of approximately 2 TB */
 +if (total_sectors  65535LL * 255 * 255) {
  return -EFBIG;
 +}
  
  if (total_sectors  65535 * 16 * 63) {
  *secs_per_cyl = 255;
 -*heads = 16;
 +if (total_sectors  65535 * 16 * 255) {
 +*heads = 255;
 +} else {
 +*heads = 16;
 +}
  cyls_times_heads = total_sectors / *secs_per_cyl;
  } else {
  *secs_per_cyl = 17;

Re: [Qemu-devel] [PATCH] block: vpc initialize the uuid footer field

[Charles Arnold]

Ping?

Is this ok?

- Charles

 On 11/2/2012 at 09:54 AM, in message 50a0e829.5b74.009...@suse.com, 
 Charles
Arnold wrote: 
 block/vpc: Initialize the uuid field in the footer with a generated uuid.
 
 Signed-off-by: Charles Arnold carn...@suse.com
 
 diff --git a/block/vpc.c b/block/vpc.c
 index b6bf52f..f14c6ae 100644
 --- a/block/vpc.c
 +++ b/block/vpc.c
 @@ -26,6 +26,9 @@
  #include block_int.h
  #include module.h
  #include migration.h
 +#if defined(CONFIG_UUID)
 +#include uuid/uuid.h
 +#endif
  
  /**/
  
 @@ -739,7 +742,9 @@ static int vpc_create(const char *filename, 
 QEMUOptionParameter *options)
  
  footer-type = be32_to_cpu(disk_type);
  
 -/* TODO uuid is missing */
 +#if defined(CONFIG_UUID)
 +uuid_generate(footer-uuid);
 +#endif
  
  footer-checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
  

[Qemu-devel] [PATCH] block: vpc initialize the uuid footer field

[Charles Arnold]

block/vpc: Initialize the uuid field in the footer with a generated uuid.

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c
index b6bf52f..f14c6ae 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -26,6 +26,9 @@
 #include block_int.h
 #include module.h
 #include migration.h
+#if defined(CONFIG_UUID)
+#include uuid/uuid.h
+#endif
 
 /**/
 
@@ -739,7 +742,9 @@ static int vpc_create(const char *filename, 
QEMUOptionParameter *options)
 
 footer-type = be32_to_cpu(disk_type);
 
-/* TODO uuid is missing */
+#if defined(CONFIG_UUID)
+uuid_generate(footer-uuid);
+#endif
 
 footer-checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
 

[Qemu-devel] [PATCH] block: vpc support for ~2 TB disks

[Charles Arnold]

The VHD specification allows for up to a 2 TB disk size. The current
implementation in qemu emulates EIDE and ATA-2 hardware which only allows
for up to 127 GB.  This disk size limitation can be overridden by allowing
up to 255 heads instead of the normal 4 bit limitation of 16.  Doing so
allows disk images to be created of up to nearly 2 TB.  This change does
not violate the VHD format specification nor does it change how smaller
disks (ie, =127GB) are defined. 

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c
index b6bf52f..0c2eaf8 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -198,7 +198,8 @@ static int vpc_open(BlockDriverState *bs, int flags)
 bs-total_sectors = (int64_t)
 be16_to_cpu(footer-cyls) * footer-heads * footer-secs_per_cyl;
 
-if (bs-total_sectors = 65535 * 16 * 255) {
+/* Allow a maximum disk size of approximately 2 TB */
+if (bs-total_sectors = 65535LL * 255 * 255) {
 err = -EFBIG;
 goto fail;
 }
@@ -524,19 +525,27 @@ static coroutine_fn int vpc_co_write(BlockDriverState 
*bs, int64_t sector_num,
  * Note that the geometry doesn't always exactly match total_sectors but
  * may round it down.
  *
- * Returns 0 on success, -EFBIG if the size is larger than 127 GB
+ * Returns 0 on success, -EFBIG if the size is larger than ~2 TB. Override
+ * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB)
+ * and instead allow up to 255 heads.
  */
 static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
 uint8_t* heads, uint8_t* secs_per_cyl)
 {
 uint32_t cyls_times_heads;
 
-if (total_sectors  65535 * 16 * 255)
+/* Allow a maximum disk size of approximately 2 TB */
+if (total_sectors  65535LL * 255 * 255) {
 return -EFBIG;
+}
 
 if (total_sectors  65535 * 16 * 63) {
 *secs_per_cyl = 255;
-*heads = 16;
+if (total_sectors  65535 * 16 * 255) {
+*heads = 255;
+} else {
+*heads = 16;
+}
 cyls_times_heads = total_sectors / *secs_per_cyl;
 } else {
 *secs_per_cyl = 17;

[Qemu-devel] [PATCH] qemu-img: Fix segmentation fault

[Charles Arnold]

The following command generates a segmentation fault.
qemu-img convert -O vpc -o ? test test2
This is because the 'goto out;' statement calls qemu_progress_end 
before qemu_progress_init is called resulting in a NULL pointer
invocation.

Signed-off-by: Charles Arnold carn...@suse.com
---

diff --git a/qemu-img.c b/qemu-img.c
index 0ae543c..113de93 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -712,6 +712,9 @@ static int img_convert(int argc, char **argv)
 
 out_filename = argv[argc - 1];
 
+/* Initialize before goto out */
+qemu_progress_init(progress, 2.0);
+
 if (options  !strcmp(options, ?)) {
 ret = print_block_option_help(out_filename, out_fmt);
 goto out;
@@ -724,7 +727,6 @@ static int img_convert(int argc, char **argv)
 goto out;
 }
 
-qemu_progress_init(progress, 2.0);
 qemu_progress_print(0, 100);
 
 bs = g_malloc0(bs_n * sizeof(BlockDriverState *));

Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

 On 2/6/2012 at 08:46 AM, in message 4f2ff5b9.9090...@redhat.com, Kevin 
 Wolf
kw...@redhat.com wrote: 
 
 Somehow you lost the ret = -EFBIG here.
 
 Otherwise the patch looks good enough for me.
 
 Kevin

Thanks Kevin.  Here is the revised patch with just this fix.
- Charles


The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing.  Qemu 
currently only supports Dynamic disks.  This patch adds support for
the Fixed Disk format.

Usage:
Example 1: qemu-img create -f vpc -o type=fixed filename [size]
Example 2: qemu-img convert -O vpc -o type=fixed input filename output 
filename

While it is also allowed to specify '-o type=dynamic', the default disk type 
remains Dynamic and is what is used when the type is left unspecified.

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..db6b14b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -161,13 +161,27 @@ static int vpc_open(BlockDriverState *bs, int flags)
 uint8_t buf[HEADER_SIZE];
 uint32_t checksum;
 int err = -1;
+int disk_type = VHD_DYNAMIC;
 
 if (bdrv_pread(bs-file, 0, s-footer_buf, HEADER_SIZE) != HEADER_SIZE)
 goto fail;
 
 footer = (struct vhd_footer*) s-footer_buf;
-if (strncmp(footer-creator, conectix, 8))
-goto fail;
+if (strncmp(footer-creator, conectix, 8)) {
+int64_t offset = bdrv_getlength(bs-file);
+if (offset  HEADER_SIZE) {
+goto fail;
+}
+/* If a fixed disk, the footer is found only at the end of the file */
+if (bdrv_pread(bs-file, offset-HEADER_SIZE, s-footer_buf, 
HEADER_SIZE)
+!= HEADER_SIZE) {
+goto fail;
+}
+if (strncmp(footer-creator, conectix, 8)) {
+goto fail;
+}
+disk_type = VHD_FIXED;
+}
 
 checksum = be32_to_cpu(footer-checksum);
 footer-checksum = 0;
@@ -186,49 +200,54 @@ static int vpc_open(BlockDriverState *bs, int flags)
 goto fail;
 }
 
-if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf, 
HEADER_SIZE)
-!= HEADER_SIZE)
-goto fail;
-
-dyndisk_header = (struct vhd_dyndisk_header*) buf;
+if (disk_type == VHD_DYNAMIC) {
+if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf,
+HEADER_SIZE) != HEADER_SIZE) {
+goto fail;
+}
 
-if (strncmp(dyndisk_header-magic, cxsparse, 8))
-goto fail;
+dyndisk_header = (struct vhd_dyndisk_header *) buf;
 
+if (strncmp(dyndisk_header-magic, cxsparse, 8)) {
+goto fail;
+}
 
-s-block_size = be32_to_cpu(dyndisk_header-block_size);
-s-bitmap_size = ((s-block_size / (8 * 512)) + 511)  ~511;
+s-block_size = be32_to_cpu(dyndisk_header-block_size);
+s-bitmap_size = ((s-block_size / (8 * 512)) + 511)  ~511;
 
-s-max_table_entries = be32_to_cpu(dyndisk_header-max_table_entries);
-s-pagetable = g_malloc(s-max_table_entries * 4);
+s-max_table_entries = be32_to_cpu(dyndisk_header-max_table_entries);
+s-pagetable = g_malloc(s-max_table_entries * 4);
 
-s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
-if (bdrv_pread(bs-file, s-bat_offset, s-pagetable,
-s-max_table_entries * 4) != s-max_table_entries * 4)
-   goto fail;
+s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
+if (bdrv_pread(bs-file, s-bat_offset, s-pagetable,
+s-max_table_entries * 4) != s-max_table_entries * 4) {
+goto fail;
+}
 
-s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
+s-free_data_block_offset =
+(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
 
-for (i = 0; i  s-max_table_entries; i++) {
-be32_to_cpus(s-pagetable[i]);
-if (s-pagetable[i] != 0x) {
-int64_t next = (512 * (int64_t) s-pagetable[i]) +
-s-bitmap_size + s-block_size;
+for (i = 0; i  s-max_table_entries; i++) {
+be32_to_cpus(s-pagetable[i]);
+if (s-pagetable[i] != 0x) {
+int64_t next = (512 * (int64_t) s-pagetable[i]) +
+s-bitmap_size + s-block_size;
 
-if (next s-free_data_block_offset)
-s-free_data_block_offset = next;
+if (next  s-free_data_block_offset) {
+s-free_data_block_offset = next;
+}
+}
 }
-}
 
-s-last_bitmap_offset = (int64_t) -1;
+s-last_bitmap_offset = (int64_t) -1;
 
 #ifdef CACHE
-s-pageentry_u8 = g_malloc(512);
-s-pageentry_u32 = s-pageentry_u8;
-s-pageentry_u16 = s-pageentry_u8;
-s-last_pagetable = -1;
+s-pageentry_u8 = g_malloc(512);
+s-pageentry_u32 = s-pageentry_u8;
+s-pageentry_u16 = s-pageentry_u8

Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

 On 2/6/2012 at 09:51 AM, in message 4f3004fc.7070...@redhat.com, Kevin 
 Wolf
kw...@redhat.com wrote: 
 Am 06.02.2012 17:22, schrieb Charles Arnold:
 On 2/6/2012 at 08:46 AM, in message 4f2ff5b9.9090...@redhat.com, Kevin 
 Wolf
 kw...@redhat.com wrote: 

 Somehow you lost the ret = -EFBIG here.

 Otherwise the patch looks good enough for me.

 Kevin
 
 Thanks Kevin.  Here is the revised patch with just this fix.
 - Charles
 
 Thanks, applied to the block branch.
 
 I have one question left, though:
 
 +total_sectors = total_size / BDRV_SECTOR_SIZE;
 +if (disk_type == VHD_DYNAMIC) {
 +/* Calculate matching total_size and geometry. Increase the number 
 of
 +   sectors requested until we get enough (or fail). */
 +for (i = 0; total_sectors  (int64_t)cyls * heads * secs_per_cyl;
 + i++) {
 +if (calculate_geometry(total_sectors + i,
 +   cyls, heads, secs_per_cyl)) {
 +ret = -EFBIG;
 +goto fail;
 +}
 +}
 +} else {
 +if (calculate_geometry(total_sectors, cyls, heads, 
 secs_per_cyl)) {
 +ret = -EFBIG;
 +goto fail;
 +}
 +}
 
 What's the reason that we need to do things differently here depending
 on the subformat? Dynamic disks round up the size so that during image
 conversion images won't be truncated. For fixed images,
 calculate_geometry can round down, so don't fixed image have the same
 problem?

Yes. In my testing I was simply creating a fixed disk that would appear exactly 
as 
it does when an equivalent size was created on windows.  I did not factor in 
the rounding needed on convert in order to get the right number of sectors
to cover the total (and somewhat arbitrary) size of the disk.
Combining them means that we will usually round up a little bit even on create 
which I suppose is fine. 

- Charles

Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

Next version of the patch with fixes, cleanups, and suggestions.
- Charles


The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing.  Qemu 
currently only supports Dynamic disks.  This patch adds support for
the Fixed Disk format.

Usage:
Example 1: qemu-img create -f vpc -o type=fixed filename [size]
Example 2: qemu-img convert -O vpc -o type=fixed input filename output 
filename

While it is also allowed to specify '-o type=dynamic', the default disk type 
remains Dynamic and is what is used when the type is left unspecified.

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..d9a1c44 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -161,13 +161,27 @@ static int vpc_open(BlockDriverState *bs, int flags)
 uint8_t buf[HEADER_SIZE];
 uint32_t checksum;
 int err = -1;
+int disk_type = VHD_DYNAMIC;
 
 if (bdrv_pread(bs-file, 0, s-footer_buf, HEADER_SIZE) != HEADER_SIZE)
 goto fail;
 
 footer = (struct vhd_footer*) s-footer_buf;
-if (strncmp(footer-creator, conectix, 8))
-goto fail;
+if (strncmp(footer-creator, conectix, 8)) {
+int64_t offset = bdrv_getlength(bs-file);
+if (offset  HEADER_SIZE) {
+goto fail;
+}
+/* If a fixed disk, the footer is found only at the end of the file */
+if (bdrv_pread(bs-file, offset-HEADER_SIZE, s-footer_buf, 
HEADER_SIZE)
+!= HEADER_SIZE) {
+goto fail;
+}
+if (strncmp(footer-creator, conectix, 8)) {
+goto fail;
+}
+disk_type = VHD_FIXED;
+}
 
 checksum = be32_to_cpu(footer-checksum);
 footer-checksum = 0;
@@ -186,49 +200,54 @@ static int vpc_open(BlockDriverState *bs, int flags)
 goto fail;
 }
 
-if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf, 
HEADER_SIZE)
-!= HEADER_SIZE)
-goto fail;
-
-dyndisk_header = (struct vhd_dyndisk_header*) buf;
+if (disk_type == VHD_DYNAMIC) {
+if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf,
+HEADER_SIZE) != HEADER_SIZE) {
+goto fail;
+}
 
-if (strncmp(dyndisk_header-magic, cxsparse, 8))
-goto fail;
+dyndisk_header = (struct vhd_dyndisk_header *) buf;
 
+if (strncmp(dyndisk_header-magic, cxsparse, 8)) {
+goto fail;
+}
 
-s-block_size = be32_to_cpu(dyndisk_header-block_size);
-s-bitmap_size = ((s-block_size / (8 * 512)) + 511)  ~511;
+s-block_size = be32_to_cpu(dyndisk_header-block_size);
+s-bitmap_size = ((s-block_size / (8 * 512)) + 511)  ~511;
 
-s-max_table_entries = be32_to_cpu(dyndisk_header-max_table_entries);
-s-pagetable = g_malloc(s-max_table_entries * 4);
+s-max_table_entries = be32_to_cpu(dyndisk_header-max_table_entries);
+s-pagetable = g_malloc(s-max_table_entries * 4);
 
-s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
-if (bdrv_pread(bs-file, s-bat_offset, s-pagetable,
-s-max_table_entries * 4) != s-max_table_entries * 4)
-   goto fail;
+s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
+if (bdrv_pread(bs-file, s-bat_offset, s-pagetable,
+s-max_table_entries * 4) != s-max_table_entries * 4) {
+goto fail;
+}
 
-s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
+s-free_data_block_offset =
+(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
 
-for (i = 0; i  s-max_table_entries; i++) {
-be32_to_cpus(s-pagetable[i]);
-if (s-pagetable[i] != 0x) {
-int64_t next = (512 * (int64_t) s-pagetable[i]) +
-s-bitmap_size + s-block_size;
+for (i = 0; i  s-max_table_entries; i++) {
+be32_to_cpus(s-pagetable[i]);
+if (s-pagetable[i] != 0x) {
+int64_t next = (512 * (int64_t) s-pagetable[i]) +
+s-bitmap_size + s-block_size;
 
-if (next s-free_data_block_offset)
-s-free_data_block_offset = next;
+if (next  s-free_data_block_offset) {
+s-free_data_block_offset = next;
+}
+}
 }
-}
 
-s-last_bitmap_offset = (int64_t) -1;
+s-last_bitmap_offset = (int64_t) -1;
 
 #ifdef CACHE
-s-pageentry_u8 = g_malloc(512);
-s-pageentry_u32 = s-pageentry_u8;
-s-pageentry_u16 = s-pageentry_u8;
-s-last_pagetable = -1;
+s-pageentry_u8 = g_malloc(512);
+s-pageentry_u32 = s-pageentry_u8;
+s-pageentry_u16 = s-pageentry_u8;
+s-last_pagetable = -1;
 #endif
+}
 
 qemu_co_mutex_init(s-lock);
 
@@ -395,7 +414,11 @@ static int vpc_read(BlockDriverState *bs, int64_t 
sector_num,
 int ret;
 int64_t offset

Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

 On 2/1/2012 at 05:15 AM, in message 4f292cd0.20...@redhat.com, Kevin Wolf
kw...@redhat.com wrote: 
 Am 01.02.2012 00:04, schrieb Charles Arnold:
 Thanks Andreas,
 
 The 'TODO uuid is missing' comment in the patch is from the 
 original sources (as well as many '//' comments).  The vhd footer 
 and header data structures contain a field for a UUID but no code 
 was ever developed to generate one.
 The revised patch is below after running scripts/checkpatch.pl and
 fixing the 32 bit issues.
 
 - Charles
 
 
 The Virtual Hard Disk Image Format Specification allows for three
 types of hard disk formats, Fixed, Dynamic, and Differencing.  Qemu 
 currently only supports Dynamic disks.  This patch adds support for
 the Fixed Disk format.
 
 Usage:
 Example 1: qemu-img create -f vpc -o type=fixed filename [size]
 Example 2: qemu-img convert -O vpc -o type=fixed input filename 
 output 
 filename
 
 While it is also allowed to specify '-o type=dynamic', the default disk type 
 remains Dynamic and is what is used when the type is left unspecified.
 
 Signed-off-by: Charles Arnold carn...@suse.com
 
 You have a lot of trailing whitespace in your patch, to the extent that
 the patch is corrupted:
 
 error: block/vpc.c   : does not exist in
 index
 
 Please consider using git send-email.

Sorry about that.

 
 
 diff --git a/block/vpc.c b/block/vpc.c  
 index 89a5ee2..04db372 100644   
 --- a/block/vpc.c   
 +++ b/block/vpc.c   
 @@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
  struct vhd_dyndisk_header* dyndisk_header;   
  uint8_t buf[HEADER_SIZE];
  uint32_t checksum;   
 +int disk_type = VHD_DYNAMIC; 
  int err = -1;
   
  if (bdrv_pread(bs-file, 0, s-footer_buf, HEADER_SIZE) != HEADER_SIZE)
  goto fail; 
 
  footer = (struct vhd_footer*) s-footer_buf;   
 -if (strncmp(footer-creator, conectix, 8))   
 -goto fail; 
 +if (strncmp(footer-creator, conectix, 8)) { 
 +int64_t offset = bdrv_getlength(bs-file); 
 
 bdrv_getlength can fail.

Ok, I'll fix this.

 
 +/* If a fixed disk, the footer is found only at the end of the file 
 */
 +if (bdrv_pread(bs-file, offset-HEADER_SIZE, s-footer_buf, 
 HEADER_SIZE)
 +!= HEADER_SIZE) {   
 
 +goto fail;  
 
 +}   
 
 +if (strncmp(footer-creator, conectix, 8)) {  
   
   
 +goto fail;  
 
 +}   
 
 +disk_type = VHD_FIXED;  
 
 +}   
 
  
 
  checksum = be32_to_cpu(footer-checksum);   
   
   
  footer-checksum = 0;   
   
   
 @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
  

  goto fail;  
 
  }   
 
  
 
 +/* The footer is all that is needed for fixed disks */  
 
 +if (disk_type == VHD_FIXED) {   
 
 +/* The fixed disk format doesn't use footer-data_offset but it 
   
   
 +   should be initialized */ 
 
 +footer-data_offset = be64_to_cpu(0xULL);   
   
  
 
 Why should it be changed? s-footer_buf is only used for updating the
 footer, so you will change the value that is in the image file.

The spec states the following about the data_offset field in the footer, 
This field is used for dynamic disks and differencing disks, 
but not fixed disks. For fixed disks, this field should be set to 0x

[Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing.  Qemu 
currently only supports Dynamic disks.  This patch adds support for
the Fixed Disk format.

Usage:
Example 1: qemu-img create -f vpc -o type=fixed filename [size]
Example 2: qemu-img convert -O vpc -o type=fixed input filename output 
filename

While it is also allowed to specify '-o type=dynamic', the default disk type 
remains Dynamic and is what is used when the type is left unspecified.

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..bfe5f7b 100644 
--- a/block/vpc.c 
+++ b/block/vpc.c 
@@ -160,6 +160,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
 struct vhd_dyndisk_header* dyndisk_header; 
 uint8_t buf[HEADER_SIZE];  
 uint32_t checksum; 
+int disk_type = VHD_DYNAMIC;   
 int err = -1;  

 if (bdrv_pread(bs-file, 0, s-footer_buf, HEADER_SIZE) != HEADER_SIZE)
@@ -167,7 +168,16 @@ static int vpc_open(BlockDriverState *bs, int flags)   

 footer = (struct vhd_footer*) s-footer_buf;   
 if (strncmp(footer-creator, conectix, 8))   
-goto fail; 
+{  
+int64_t offset = bdrv_getlength(bs-file); 
+// If a fixed disk, the footer is found only at the end of the file
+if (bdrv_pread(bs-file, offset-HEADER_SIZE, s-footer_buf, 
HEADER_SIZE)
+!= HEADER_SIZE)
 
+goto fail; 
 
+if (strncmp(footer-creator, conectix, 8))   
 
+goto fail; 
 
+disk_type = VHD_FIXED; 
 
+}  
 

 
 checksum = be32_to_cpu(footer-checksum);  
 
 footer-checksum = 0;  
 
@@ -186,6 +196,14 @@ static int vpc_open(BlockDriverState *bs, int flags)   
 
 goto fail; 
 
 }  
 

 
+// The footer is all that is needed for fixed disks.   
 
+if (disk_type == VHD_FIXED) {  
 
+// The fixed disk format doesn't use footer-data_offset but it
 
+// should be initialized   
 
+footer-data_offset = be64_to_cpu(0x); 
 
+return 0;  
 
+}  
 
+   
 
 if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf, 
HEADER_SIZE)
 != HEADER_SIZE)
 
 goto fail; 
 
@@ -533,7 +551,7 @@ static int calculate_geometry(int64_t total_sectors, 
uint16_t* cyls,
 return 0;  

 }  



-static int vpc_create(const char *filename, QEMUOptionParameter *options)  

+static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)   

 {  

 uint8_t buf[1024]; 

 struct vhd_footer* footer = (struct vhd_footer*) buf;  

@@ -544,13 +562,9 @@ static int vpc_create(const char *filename, 
QEMUOptionParameter *options)
 uint8_t heads = 0

Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Charles Arnold]

Thanks Andreas,

The 'TODO uuid is missing' comment in the patch is from the 
original sources (as well as many '//' comments).  The vhd footer 
and header data structures contain a field for a UUID but no code 
was ever developed to generate one.
The revised patch is below after running scripts/checkpatch.pl and
fixing the 32 bit issues.

- Charles


The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing.  Qemu 
currently only supports Dynamic disks.  This patch adds support for
the Fixed Disk format.

Usage:
Example 1: qemu-img create -f vpc -o type=fixed filename [size]
Example 2: qemu-img convert -O vpc -o type=fixed input filename output 
filename

While it is also allowed to specify '-o type=dynamic', the default disk type 
remains Dynamic and is what is used when the type is left unspecified.

Signed-off-by: Charles Arnold carn...@suse.com

diff --git a/block/vpc.c b/block/vpc.c  
index 89a5ee2..04db372 100644   
--- a/block/vpc.c   
+++ b/block/vpc.c   
@@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
 struct vhd_dyndisk_header* dyndisk_header;   
 uint8_t buf[HEADER_SIZE];
 uint32_t checksum;   
+int disk_type = VHD_DYNAMIC; 
 int err = -1;
  
 if (bdrv_pread(bs-file, 0, s-footer_buf, HEADER_SIZE) != HEADER_SIZE)
 goto fail; 

 footer = (struct vhd_footer*) s-footer_buf;   
-if (strncmp(footer-creator, conectix, 8))   
-goto fail; 
+if (strncmp(footer-creator, conectix, 8)) { 
+int64_t offset = bdrv_getlength(bs-file); 
+/* If a fixed disk, the footer is found only at the end of the file */
+if (bdrv_pread(bs-file, offset-HEADER_SIZE, s-footer_buf, 
HEADER_SIZE)
+!= HEADER_SIZE) {  
 
+goto fail; 
 
+}  
 
+if (strncmp(footer-creator, conectix, 8)) { 
 
+goto fail; 
 
+}  
 
+disk_type = VHD_FIXED; 
 
+}  
 

 
 checksum = be32_to_cpu(footer-checksum);  
 
 footer-checksum = 0;  
 
@@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)   
 
 goto fail; 
 
 }  
 

 
+/* The footer is all that is needed for fixed disks */ 
 
+if (disk_type == VHD_FIXED) {  
 
+/* The fixed disk format doesn't use footer-data_offset but it
 
+   should be initialized */
 
+footer-data_offset = be64_to_cpu(0xULL);  

+return 0;  
 
+}  
 
+   
 
 if (bdrv_pread(bs-file, be64_to_cpu(footer-data_offset), buf, 
HEADER_SIZE)
 != HEADER_SIZE)
 
 goto fail; 
 
@@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors, 
uint16_t* cyls,
 return 0;  
  
 }  
  

  
-static int vpc_create(const char

Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header

[Charles Arnold]

 On 11/9/2011 at 02:46 AM, in message 4eba4bec.5040...@redhat.com, Kevin 
 Wolf
kw...@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 carn...@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(0x);
 +dyndisk_header-data_offset = be64_to_cpu(0xULL);
  dyndisk_header-table_offset = be64_to_cpu(3 * 512);
  dyndisk_header-version = be32_to_cpu(0x0001);
  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

Re: [Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header

[Charles Arnold]

 On 11/9/2011 at 08:58 AM, in message 4ebaa30d.1090...@redhat.com, Kevin 
 Wolf
kw...@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(0x);
 +/* Note: The spec is actually wrong here, it says 0x, but
 MS tools expect all 64 bits to be set */
 +dyndisk_header-data_offset = be64_to_cpu(0xULL);
 
 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 carn...@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(0x);
+/*
+ * Note: The spec is actually wrong here for data_offset, it says
+ * 0x, but MS tools expect all 64 bits to be set.
+ */
+dyndisk_header-data_offset = be64_to_cpu(0xULL);
 dyndisk_header-table_offset = be64_to_cpu(3 * 512);
 dyndisk_header-version = be32_to_cpu(0x0001);
 dyndisk_header-block_size = be32_to_cpu(block_size);

[Qemu-devel] [PATCH] block: Fix vpc initialization of the Dynamic Disk Header

[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 carn...@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(0x);
+dyndisk_header-data_offset = be64_to_cpu(0xULL);
 dyndisk_header-table_offset = be64_to_cpu(3 * 512);
 dyndisk_header-version = be32_to_cpu(0x0001);
 dyndisk_header-block_size = be32_to_cpu(block_size);