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

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

[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,

=============================================================
root@ip-10-38-123-242:~/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
root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
root@ip-10-38-123-242:~/qemu-fixed# echo $?
0
root@ip-10-38-123-242:~/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:

=============================================================
root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
---
 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;
+    }
+
     if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
             != HEADER_SIZE)
         goto fail;
@@ -222,7 +228,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
 
     return 0;
  fail:
-    return -1;
+    return err;
 }
 
 /*
-- 
1.7.5.4

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

[Kevin Wolf]

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,
> 
> =============================================================
> root@ip-10-38-123-242:~/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
> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> root@ip-10-38-123-242:~/qemu-fixed# echo $?
> 0
> root@ip-10-38-123-242:~/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:
> 
> =============================================================
> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
> ---
>  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
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.

What is the CHS geometry of this 140 GB image?

Kevin

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

[Serge E. Hallyn]

Quoting Kevin Wolf (kwolf@redhat.com):
> 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,
> > 
> > =============================================================
> > root@ip-10-38-123-242:~/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
> > root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> > root@ip-10-38-123-242:~/qemu-fixed# echo $?
> > 0
> > root@ip-10-38-123-242:~/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:
> > 
> > =============================================================
> > root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> > qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> > root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
> > ---
> >  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.

> 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.

thanks,
-serge

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

[Kevin Wolf]

Am 26.07.2011 18:08, schrieb Serge E. Hallyn:
> Quoting Kevin Wolf (kwolf@redhat.com):
>> 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,
>>>
>>> =============================================================
>>> root@ip-10-38-123-242:~/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
>>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
>>> root@ip-10-38-123-242:~/qemu-fixed# echo $?
>>> 0
>>> root@ip-10-38-123-242:~/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:
>>>
>>> =============================================================
>>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
>>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
>>> root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
>>> ---
>>>  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.

>> 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
}

That is, if the image is small enough that CHS can exactly represent it,
we use the CHS even if it differs from the real size (because this is
the same as Virtual PC does), but if it's bigger let's forget about CHS
switch to the real size.

Probably needs a comment, too.

Kevin

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

[Serge E. Hallyn]

Quoting Kevin Wolf (kwolf@redhat.com):
> Am 26.07.2011 18:08, schrieb Serge E. Hallyn:
> > Quoting Kevin Wolf (kwolf@redhat.com):
> >> 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,
> >>>
> >>> =============================================================
> >>> root@ip-10-38-123-242:~/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
> >>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> >>> root@ip-10-38-123-242:~/qemu-fixed# echo $?
> >>> 0
> >>> root@ip-10-38-123-242:~/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:
> >>>
> >>> =============================================================
> >>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> >>> root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
> >>> ---
> >>>  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 <serge.hallyn@canonical.com>
---
 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

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

[Kevin Wolf]

Am 26.07.2011 22:26, schrieb Serge E. Hallyn:
> Quoting Kevin Wolf (kwolf@redhat.com):
>> Am 26.07.2011 18:08, schrieb Serge E. Hallyn:
>>> Quoting Kevin Wolf (kwolf@redhat.com):
>>>> 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,
>>>>>
>>>>> =============================================================
>>>>> root@ip-10-38-123-242:~/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
>>>>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
>>>>> root@ip-10-38-123-242:~/qemu-fixed# echo $?
>>>>> 0
>>>>> root@ip-10-38-123-242:~/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:
>>>>>
>>>>> =============================================================
>>>>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
>>>>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
>>>>> root@ip-10-38-123-242:~/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 <serge.hallyn@canonical.com>
>>>>> ---
>>>>>  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?

Double size sounds really weird. 'qemu-img create' uses the size in
bytes for it. Is that wrong?

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

Hm, any idea where the -EPERM (-1) comes from? Maybe wrong total_sectors
number you're calculating?

> 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 <serge.hallyn@canonical.com>
> ---
>  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;

If footer->size is the double byte count, then it would be:;

+    if (footer->size / 2 > (uint64_t) 65536 * 16 * 255 * 512) {
+        bs->total_sectors = 512 * footer->size / 2;

Your calculation would assume that footer->size uses units of 256 bytes,
which appears rather unlikely.

Kevin

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

[Kevin Wolf]

Am 26.07.2011 22:26, schrieb Serge E. Hallyn:
> Quoting Kevin Wolf (kwolf@redhat.com):
>> 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?

Just looked it up in the spec:

"Current Size
This field stores the current size of the hard disk, in bytes, from the
perspective of the virtual machine.
This value is same as the original size when the hard disk is created.
This value can change depending on whether the hard disk is expanded."

And for the part about using CHS:

"When a hard disk is configured as an ATA hard disk, the CHS values
(that is, Cylinder, Heads, Sectors per track) are used by the ATA
controller to determine the size of the disk. When the user creates a
hard disk of a certain size, the size of the hard disk image in the
virtual machine is smaller than that created by the user. This is
because CHS value calculated from the hard disk size is rounded down."

Kevin

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

[Serge E. Hallyn]

Quoting Kevin Wolf (kwolf@redhat.com):
> > The footer->size appears to be double the 'real' size.  So I'm actually doing
> > the blow.  Does this seem sensible?
> 
> Double size sounds really weird. 'qemu-img create' uses the size in
> bytes for it. Is that wrong?
> 
> > Doing it this way, trying to convert the image gives me '-EPERM' rather than
> > -EFBIG.  Not sure where we are best off catching that.
> 
> Hm, any idea where the -EPERM (-1) comes from? Maybe wrong total_sectors
> number you're calculating?

No, vpc.c in some places just returns 0 or -1, while that return value is
taken to be a more meaningful errno by the caller.  -1 is EPERM.  That's
why my original patch had to return -EFBIG.

> > 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 <serge.hallyn@canonical.com>
> > ---
> >  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;
> 
> If footer->size is the double byte count, then it would be:;
> 
> +    if (footer->size / 2 > (uint64_t) 65536 * 16 * 255 * 512) {
> +        bs->total_sectors = 512 * footer->size / 2;

I'm just reporting the facts :)  The patch I sent ended up returning
the right size for 140G dynamic VHD image.

So given our fuzziness on the actual meanings, I think the right thing
to do is just:

	if (footer->size > 65536 * 16 * 255 * 2)
		return -EFBIG;

> Your calculation would assume that footer->size uses units of 256 bytes,
> which appears rather unlikely.