Re: [Qemu-devel] [PATCH 1/1] block/vpc.c: Detect too-large vpc file

[Serge E. Hallyn]

Quoting Kevin Wolf (address@hidden):
> Am 26.07.2011 18:08, schrieb Serge E. Hallyn:
> > Quoting Kevin Wolf (address@hidden):
> >> Am 25.07.2011 20:34, schrieb Serge E. Hallyn:
> >>> VHD files technically can be up to 2Tb, but virtual pc is limited
> >>> to 127G.  Currently qemu-img refused to create vpc files > 127G,
> >>> but it is failing to return error when converting from a non-vpc
> >>> VHD file which is >127G.  It returns success, but creates a truncated
> >>> converted image.  Also, qemu-img info claims the vpc file is 127G
> >>> (and clean).
> >>>
> >>> This patch detects a too-large vpc file and returns -EFBIG.  Without
> >>> this patch,
> >>>
> >>> =============================================================
> >>> address@hidden:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd 
> >>> image: /mnt/140g-dynamic.vhd
> >>> file format: vpc
> >>> virtual size: 127G (136899993600 bytes)
> >>> disk size: 284K
> >>> address@hidden:~/qemu-fixed# qemu-img convert -f vpc -O raw 
> >>> /mnt/140g-dynamic.vhd /mnt/y
> >>> address@hidden:~/qemu-fixed# echo $?
> >>> 0
> >>> address@hidden:~/qemu-fixed# qemu-img info /mnt/y
> >>> image: /mnt/y
> >>> file format: raw
> >>> virtual size: 127G (136899993600 bytes)
> >>> disk size: 0
> >>> =============================================================
> >>>
> >>> (The 140G image was truncated with no warning or error.)
> >>>
> >>> With the patch, I get:
> >>>
> >>> =============================================================
> >>> address@hidden:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> >>> address@hidden:~/qemu-fixed# ./qemu-img convert -f vpc -O raw 
> >>> /mnt/140g-dynamic.vhd /mnt/y
> >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd'
> >>> =============================================================
> >>>
> >>> See https://bugs.launchpad.net/qemu/+bug/814222 for details.
> >>>
> >>> Signed-off-by: Serge Hallyn <address@hidden>
> >>> ---
> >>>  block/vpc.c |    8 +++++++-
> >>>  1 files changed, 7 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/block/vpc.c b/block/vpc.c
> >>> index 56865da..fdd5236 100644
> >>> --- a/block/vpc.c
> >>> +++ b/block/vpc.c
> >>> @@ -156,6 +156,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
> >>>      struct vhd_dyndisk_header* dyndisk_header;
> >>>      uint8_t buf[HEADER_SIZE];
> >>>      uint32_t checksum;
> >>> +    int err = -1;
> >>>  
> >>>      if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != 
> >>> HEADER_SIZE)
> >>>          goto fail;
> >>> @@ -176,6 +177,11 @@ 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) {
> >>> +        err = -EFBIG;
> >>> +        goto fail;
> >>> +    }
> >>
> >> I wonder why this works. If bs->total_sectors was right, shouldn't it
> > 
> > bs->total_sectors is exactly 65535 * 16 * 255.  It never gets bigger.
> > So really the check could be
> > 
> >     if (bs->total_sectors == 65535 * 16 * 255) {
> > 
> > and it would still work.
> 
> Oh, now I understand. I think this is a bit too restrictive as it
> forbids the largest possible size.

Yeah, it does.

> >> have converted the full 140 GB? I can't see where else we would limit it
> >> to 127 GB, so what I had expected is that the CHS geometry stored in the
> >> image header is already too small.
> > 
> > If I understand you correctly, what you're saying is exactly what I'm
> > finding.  total_sectors is not reflective of the true size.
> > 
> > I assume the true size is somewhere to be found :)  But I haven't found a
> > nice spec for these.
> 
> I think that footer->size contains the real size. Maybe we should do
> something like this:
> if (footer->size > 65536 * 16 * 255 * 512) {
>     bs->total_sectors = footer->size / 512;
> } else {
>     bs->total_sectors = (int64_t)
>         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl
> }

The footer->size appears to be double the 'real' size.  So I'm actually doing
the blow.  Does this seem sensible?

Doing it this way, trying to convert the image gives me '-EPERM' rather than
-EFBIG.  Not sure where we are best off catching that.

Subject: [PATCH 1/1] vpc: accurately detect file size

VHD files technically can be up to 2Tb, but virtual pc uses CHS which
is limited to 127G.  Currently qemu-img refused to create vpc files > 127G,
but it reports any file >= 127G as being 127G.  If asked to convert such a
file, it creates a 127G (truncated) file without returning error.

This patch uses the size reported in the footer to detect whether the
size is > 127G, and reports the size stored in footer if so.

qemu-img info now reports the correct size.

qemu-img convert refuses, but returns -EPERM rather than -EFBIG.

See https://bugs.launchpad.net/qemu/+bug/814222 for details.

Changelog: follow Kevin Wolf's suggestion for detecting true file size.

Signed-off-by: Serge Hallyn <address@hidden>
---
 block/vpc.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 56865da..37eebc2 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -173,8 +173,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
     // The visible size of a image in Virtual PC depends on the geometry
     // rather than on the size stored in the footer (the size in the footer
     // is too large usually)
-    bs->total_sectors = (int64_t)
-        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
+    // If the size stored in the footer is larger than CHS can represent,
+    // then use the size stored in the footer.
+    if (footer->size > (uint64_t) 65536 * 16 * 255 * 2) {
+        bs->total_sectors = footer->size / 2;
+    } else {
+        bs->total_sectors = (int64_t)
+            be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
+    }
 
     if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, 
HEADER_SIZE)
             != HEADER_SIZE)
-- 
1.7.5.4


thanks,
-serge