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

[Kevin Wolf <kwolf@redhat.com>]

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