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