| Message ID | 20110726202638.GA11351@hallyn.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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)