| Message ID | 20110725183435.GA26649@hallyn.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
============================================================= 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; } /*