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

["Serge E. Hallyn" <serge@hallyn.com>]

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.