| Message ID | 1360265212-22037-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 07, 2013 at 08:26:52PM +0100, Stefan Weil wrote: > From: Stefan Weil <stefan@kiwi.(none)> Should be "From: Stefan Weil <sw@weilnetz.de>"? > The size calculated from the CHS values is not the real image (disk) size, > but usually a smaller value. This is caused by rounding effects. > > Only older operating systems use CHS. Such guests won't be able to use > the whole disk. All modern operating systems use the real size. > > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > This is a rebased extract from my patch series for block/vpc.c. > It's the minimum needed to fix the open bug for QEMU 1.4. > > The rest of the series can be discussed and applied after 1.4. > > Regards > > Stefan W. > > PS. Please excuse a previous personal mail which I had sent > with a wrong signature and without addressing qemu-devel. > > block/vpc.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Am 07.02.2013 20:26, schrieb Stefan Weil: > From: Stefan Weil <stefan@kiwi.(none)> > > The size calculated from the CHS values is not the real image (disk) size, > but usually a smaller value. This is caused by rounding effects. > > Only older operating systems use CHS. Such guests won't be able to use > the whole disk. All modern operating systems use the real size. > > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > This is a rebased extract from my patch series for block/vpc.c. > It's the minimum needed to fix the open bug for QEMU 1.4. > > The rest of the series can be discussed and applied after 1.4. > > Regards > > Stefan W. > > PS. Please excuse a previous personal mail which I had sent > with a wrong signature and without addressing qemu-devel. > > block/vpc.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 82229ef..b4ff564 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -34,6 +34,8 @@ > > #define HEADER_SIZE 512 > > +#define VHD_SECTOR_SIZE 512 > + > //#define CACHE > > enum vhd_type { > @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) > /* Write 'checksum' back to footer, or else will leave it with zero. */ > footer->checksum = be32_to_cpu(checksum); > > - // 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; > + /* The visible size of a image in Virtual PC depends on the guest: > + * QEMU and other emulators report the real size (here in sectors). > + * All modern operating systems use this real size. > + * Very old operating systems use CHS values to calculate the total size. > + * This calculated size is usually smaller than the real size. > + */ > + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; It's unfortunate that I don't have my old Virtual PC installation around any more so I could prove that you're wrong for at least some versions. Or does a Linux of 2009 already count as "very old"? If we want to commit this - and I still feel uncomfortable about it - then maybe it's best to remove the comment altogether instead of making such claims. The new code is the intuitively expected one anyway. Kevin
On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote: > Am 07.02.2013 20:26, schrieb Stefan Weil: > > From: Stefan Weil <stefan@kiwi.(none)> > > > > The size calculated from the CHS values is not the real image (disk) size, > > but usually a smaller value. This is caused by rounding effects. > > > > Only older operating systems use CHS. Such guests won't be able to use > > the whole disk. All modern operating systems use the real size. > > > > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. > > > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > > --- > > > > This is a rebased extract from my patch series for block/vpc.c. > > It's the minimum needed to fix the open bug for QEMU 1.4. > > > > The rest of the series can be discussed and applied after 1.4. > > > > Regards > > > > Stefan W. > > > > PS. Please excuse a previous personal mail which I had sent > > with a wrong signature and without addressing qemu-devel. > > > > block/vpc.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/block/vpc.c b/block/vpc.c > > index 82229ef..b4ff564 100644 > > --- a/block/vpc.c > > +++ b/block/vpc.c > > @@ -34,6 +34,8 @@ > > > > #define HEADER_SIZE 512 > > > > +#define VHD_SECTOR_SIZE 512 > > + > > //#define CACHE > > > > enum vhd_type { > > @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) > > /* Write 'checksum' back to footer, or else will leave it with zero. */ > > footer->checksum = be32_to_cpu(checksum); > > > > - // 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; > > + /* The visible size of a image in Virtual PC depends on the guest: > > + * QEMU and other emulators report the real size (here in sectors). > > + * All modern operating systems use this real size. > > + * Very old operating systems use CHS values to calculate the total size. > > + * This calculated size is usually smaller than the real size. > > + */ > > + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; > > It's unfortunate that I don't have my old Virtual PC installation around > any more so I could prove that you're wrong for at least some versions. > Or does a Linux of 2009 already count as "very old"? > > If we want to commit this - and I still feel uncomfortable about it - > then maybe it's best to remove the comment altogether instead of making > such claims. The new code is the intuitively expected one anyway. > > Kevin Kevin, I can test this on Virtual PC on a Win 7 install, as well as Hyper-V (I've already tested the equivalent of this patch on Hyper-V on Windows Server 2012). I'll do some testing today with this - if you have anything in particular you want me to look at, just let me know. Stefan, did you test if this behaves the same for all disk sizes, or only for larger disk sizes of 127G and up? Thanks, Jeff
Am 08.02.2013 09:18, schrieb Stefan Hajnoczi: > On Thu, Feb 07, 2013 at 08:26:52PM +0100, Stefan Weil wrote: >> From: Stefan Weil <stefan@kiwi.(none)> > Should be "From: Stefan Weil <sw@weilnetz.de>"? Yes, of course. Maybe whoever applies the patch can fix this, please. Thanks, Stefan W.
Am 08.02.2013 13:14, schrieb Jeff Cody: > On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote: >> Am 07.02.2013 20:26, schrieb Stefan Weil: >>> From: Stefan Weil <stefan@kiwi.(none)> >>> >>> The size calculated from the CHS values is not the real image (disk) size, >>> but usually a smaller value. This is caused by rounding effects. >>> >>> Only older operating systems use CHS. Such guests won't be able to use >>> the whole disk. All modern operating systems use the real size. >>> >>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. >>> >>> Signed-off-by: Stefan Weil <sw@weilnetz.de> >>> --- >>> >>> This is a rebased extract from my patch series for block/vpc.c. >>> It's the minimum needed to fix the open bug for QEMU 1.4. >>> >>> The rest of the series can be discussed and applied after 1.4. >>> >>> Regards >>> >>> Stefan W. >>> >>> PS. Please excuse a previous personal mail which I had sent >>> with a wrong signature and without addressing qemu-devel. >>> >>> block/vpc.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 82229ef..b4ff564 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -34,6 +34,8 @@ >>> >>> #define HEADER_SIZE 512 >>> >>> +#define VHD_SECTOR_SIZE 512 >>> + >>> //#define CACHE >>> >>> enum vhd_type { >>> @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) >>> /* Write 'checksum' back to footer, or else will leave it with zero. */ >>> footer->checksum = be32_to_cpu(checksum); >>> >>> - // 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; >>> + /* The visible size of a image in Virtual PC depends on the guest: >>> + * QEMU and other emulators report the real size (here in sectors). >>> + * All modern operating systems use this real size. >>> + * Very old operating systems use CHS values to calculate the total size. >>> + * This calculated size is usually smaller than the real size. >>> + */ >>> + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; >> It's unfortunate that I don't have my old Virtual PC installation around >> any more so I could prove that you're wrong for at least some versions. >> Or does a Linux of 2009 already count as "very old"? Linux is a modern OS per definition :-) I made an additional test with Knoppix (from December 2006), and it worked as expected. >> >> If we want to commit this - and I still feel uncomfortable about it - >> then maybe it's best to remove the comment altogether instead of making >> such claims. The new code is the intuitively expected one anyway. >> >> Kevin > Kevin, > > I can test this on Virtual PC on a Win 7 install, as well as > Hyper-V (I've already tested the equivalent of this patch on Hyper-V > on Windows Server 2012). I'll do some testing today with this - if > you have anything in particular you want me to look at, just let me > know. > > Stefan, did you test if this behaves the same for all disk sizes, or > only for larger disk sizes of 127G and up? This behaves the same in a test with a small (16 MiB) image. Regards, Stefan W.
On Fri, Feb 08, 2013 at 07:14:33AM -0500, Jeff Cody wrote: > On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote: > > Am 07.02.2013 20:26, schrieb Stefan Weil: > > > From: Stefan Weil <stefan@kiwi.(none)> > > > > > > The size calculated from the CHS values is not the real image (disk) size, > > > but usually a smaller value. This is caused by rounding effects. > > > > > > Only older operating systems use CHS. Such guests won't be able to use > > > the whole disk. All modern operating systems use the real size. > > > > > > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. > > > > > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > > > --- > > > > > > This is a rebased extract from my patch series for block/vpc.c. > > > It's the minimum needed to fix the open bug for QEMU 1.4. > > > > > > The rest of the series can be discussed and applied after 1.4. > > > > > > Regards > > > > > > Stefan W. > > > > > > PS. Please excuse a previous personal mail which I had sent > > > with a wrong signature and without addressing qemu-devel. > > > > > > block/vpc.c | 14 +++++++++----- > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/block/vpc.c b/block/vpc.c > > > index 82229ef..b4ff564 100644 > > > --- a/block/vpc.c > > > +++ b/block/vpc.c > > > @@ -34,6 +34,8 @@ > > > > > > #define HEADER_SIZE 512 > > > > > > +#define VHD_SECTOR_SIZE 512 > > > + > > > //#define CACHE > > > > > > enum vhd_type { > > > @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) > > > /* Write 'checksum' back to footer, or else will leave it with zero. */ > > > footer->checksum = be32_to_cpu(checksum); > > > > > > - // 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; > > > + /* The visible size of a image in Virtual PC depends on the guest: > > > + * QEMU and other emulators report the real size (here in sectors). > > > + * All modern operating systems use this real size. > > > + * Very old operating systems use CHS values to calculate the total size. > > > + * This calculated size is usually smaller than the real size. > > > + */ > > > + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; > > > > It's unfortunate that I don't have my old Virtual PC installation around > > any more so I could prove that you're wrong for at least some versions. > > Or does a Linux of 2009 already count as "very old"? > > > > If we want to commit this - and I still feel uncomfortable about it - > > then maybe it's best to remove the comment altogether instead of making > > such claims. The new code is the intuitively expected one anyway. > > > > Kevin > > Kevin, > > I can test this on Virtual PC on a Win 7 install, as well as > Hyper-V (I've already tested the equivalent of this patch on Hyper-V > on Windows Server 2012). I'll do some testing today with this - if > you have anything in particular you want me to look at, just let me > know. Hi Jeff, Thanks for testing this on your Virtual PC and Hyper-V setup. I'll merge this patch once you have confirmed your results. Stefan
Am 08.02.2013 18:43, schrieb Stefan Weil: > Am 08.02.2013 13:14, schrieb Jeff Cody: >> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote: >>> Am 07.02.2013 20:26, schrieb Stefan Weil: >>>> From: Stefan Weil <stefan@kiwi.(none)> >>>> >>>> The size calculated from the CHS values is not the real image (disk) size, >>>> but usually a smaller value. This is caused by rounding effects. >>>> >>>> Only older operating systems use CHS. Such guests won't be able to use >>>> the whole disk. All modern operating systems use the real size. >>>> >>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. >>>> >>>> Signed-off-by: Stefan Weil <sw@weilnetz.de> >>>> --- >>>> >>>> This is a rebased extract from my patch series for block/vpc.c. >>>> It's the minimum needed to fix the open bug for QEMU 1.4. >>>> >>>> The rest of the series can be discussed and applied after 1.4. >>>> >>>> Regards >>>> >>>> Stefan W. >>>> >>>> PS. Please excuse a previous personal mail which I had sent >>>> with a wrong signature and without addressing qemu-devel. >>>> >>>> block/vpc.c | 14 +++++++++----- >>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/block/vpc.c b/block/vpc.c >>>> index 82229ef..b4ff564 100644 >>>> --- a/block/vpc.c >>>> +++ b/block/vpc.c >>>> @@ -34,6 +34,8 @@ >>>> >>>> #define HEADER_SIZE 512 >>>> >>>> +#define VHD_SECTOR_SIZE 512 >>>> + >>>> //#define CACHE >>>> >>>> enum vhd_type { >>>> @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) >>>> /* Write 'checksum' back to footer, or else will leave it with zero. */ >>>> footer->checksum = be32_to_cpu(checksum); >>>> >>>> - // 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; >>>> + /* The visible size of a image in Virtual PC depends on the guest: >>>> + * QEMU and other emulators report the real size (here in sectors). >>>> + * All modern operating systems use this real size. >>>> + * Very old operating systems use CHS values to calculate the total size. >>>> + * This calculated size is usually smaller than the real size. >>>> + */ >>>> + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; >>> It's unfortunate that I don't have my old Virtual PC installation around >>> any more so I could prove that you're wrong for at least some versions. >>> Or does a Linux of 2009 already count as "very old"? > > Linux is a modern OS per definition :-) > > I made an additional test with Knoppix (from December 2006), > and it worked as expected. So I'm afraid that the reason isn't the guest OS but the Virtual PC version. I guess supporting newer versions of it is more important, so if Jeff's testing passes, I'm not against taking the patch, even though I don't feel completely comfortable about it. But I don't see how we could support both ways at the same time. Kevin
On Mon, Feb 11, 2013 at 09:54:56AM +0100, Kevin Wolf wrote: > Am 08.02.2013 18:43, schrieb Stefan Weil: > > Am 08.02.2013 13:14, schrieb Jeff Cody: > >> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote: > >>> Am 07.02.2013 20:26, schrieb Stefan Weil: > >>>> From: Stefan Weil <stefan@kiwi.(none)> > >>>> > >>>> The size calculated from the CHS values is not the real image (disk) size, > >>>> but usually a smaller value. This is caused by rounding effects. > >>>> > >>>> Only older operating systems use CHS. Such guests won't be able to use > >>>> the whole disk. All modern operating systems use the real size. > >>>> > >>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/. > >>>> > >>>> Signed-off-by: Stefan Weil <sw@weilnetz.de> > >>>> --- > >>>> > >>>> This is a rebased extract from my patch series for block/vpc.c. > >>>> It's the minimum needed to fix the open bug for QEMU 1.4. > >>>> > >>>> The rest of the series can be discussed and applied after 1.4. > >>>> > >>>> Regards > >>>> > >>>> Stefan W. > >>>> > >>>> PS. Please excuse a previous personal mail which I had sent > >>>> with a wrong signature and without addressing qemu-devel. > >>>> > >>>> block/vpc.c | 14 +++++++++----- > >>>> 1 file changed, 9 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/block/vpc.c b/block/vpc.c > >>>> index 82229ef..b4ff564 100644 > >>>> --- a/block/vpc.c > >>>> +++ b/block/vpc.c > >>>> @@ -34,6 +34,8 @@ > >>>> > >>>> #define HEADER_SIZE 512 > >>>> > >>>> +#define VHD_SECTOR_SIZE 512 > >>>> + > >>>> //#define CACHE > >>>> > >>>> enum vhd_type { > >>>> @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) > >>>> /* Write 'checksum' back to footer, or else will leave it with zero. */ > >>>> footer->checksum = be32_to_cpu(checksum); > >>>> > >>>> - // 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; > >>>> + /* The visible size of a image in Virtual PC depends on the guest: > >>>> + * QEMU and other emulators report the real size (here in sectors). > >>>> + * All modern operating systems use this real size. > >>>> + * Very old operating systems use CHS values to calculate the total size. > >>>> + * This calculated size is usually smaller than the real size. > >>>> + */ > >>>> + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; > >>> It's unfortunate that I don't have my old Virtual PC installation around > >>> any more so I could prove that you're wrong for at least some versions. > >>> Or does a Linux of 2009 already count as "very old"? > > > > Linux is a modern OS per definition :-) > > > > I made an additional test with Knoppix (from December 2006), > > and it worked as expected. > > So I'm afraid that the reason isn't the guest OS but the Virtual PC > version. I guess supporting newer versions of it is more important, so > if Jeff's testing passes, I'm not against taking the patch, even though > I don't feel completely comfortable about it. But I don't see how we > could support both ways at the same time. > I've done some testing back and forth between VPC, Win Server 2012 Hyper-V, and qemu/kvm with and without this patch applied. The short: this patch will break Virtual PC VHD compatibility (as it exists under Win7), but allow compatibility for VHD files created by Hyper-V (at least under Server 2012). VPC and Server 2012 Hyper-V are also incompatible with each other, when it comes to VHD files, if that is any consolation. I used sha1sums between the different combination of VHD files filled to capacity with random data under VPC, qemu/kvm (with & without the patch), and Hyper-V. Without this patch we are an exact math with VPC, and with this patch we are an exact match with Hyper-V. However, perhaps we can support both, depending on who created the VHD file. There is a "Creator Application" field in the VHD header - for the VPC-created VHD file, that field is ASCII "vpc ". For the Hyper-V created VHD file, that field is ASCII "win ". If we see the field as "vpc ", we could default to using the CHS geometry found in the header - otherwise, do as this patch does, and go by the virtual disk size. Jeff
Am 11.02.2013 20:38, schrieb Jeff Cody: > I've done some testing back and forth between VPC, Win Server 2012 > Hyper-V, and qemu/kvm with and without this patch applied. First of all, thanks a lot for doing all the testing! > The short: this patch will break Virtual PC VHD compatibility (as it > exists under Win7), but allow compatibility for VHD files created by > Hyper-V (at least under Server 2012). > > VPC and Server 2012 Hyper-V are also incompatible with each other, > when it comes to VHD files, if that is any consolation. Okay. I had hoped that Win 7 would be fine as well and we might just get away with applying the patch, but this we can't possibly ignore. > I used sha1sums between the different combination of VHD files filled to > capacity with random data under VPC, qemu/kvm (with & without the > patch), and Hyper-V. Without this patch we are an exact math with > VPC, and with this patch we are an exact match with Hyper-V. > > However, perhaps we can support both, depending on who created the VHD > file. There is a "Creator Application" field in the VHD header - for > the VPC-created VHD file, that field is ASCII "vpc ". For the Hyper-V > created VHD file, that field is ASCII "win ". If we see the field as > "vpc ", we could default to using the CHS geometry found in the > header - otherwise, do as this patch does, and go by the virtual disk > size. That they fill in different creator strings is good news. This isn't a nice solution, but then it wasn't nice to change the semantics between the two versions, so it's better than nothing. I'd vote for comparing the creator field. One open question is what to do with images that are neither "vpc " (or "qemu") nor "win ". I think the newer behaviour is saner, but I don't know if there are other tools for which the old semantics should be used. The other question is what to do with newly created images. Maybe we should add a creation option. At the same time we should probably switch from the "qemu" creator string to something different if Hyper-V semantics is used, in order to allow distinguishing the version. I'm also not sure what the right default would be for now. Kevin
Applied. Thanks. Regards, Anthony Liguori
On Mon, Feb 11, 2013 at 09:09:48PM +0100, Kevin Wolf wrote: > Am 11.02.2013 20:38, schrieb Jeff Cody: > > I've done some testing back and forth between VPC, Win Server 2012 > > Hyper-V, and qemu/kvm with and without this patch applied. > > First of all, thanks a lot for doing all the testing! > > > The short: this patch will break Virtual PC VHD compatibility (as it > > exists under Win7), but allow compatibility for VHD files created by > > Hyper-V (at least under Server 2012). > > > > VPC and Server 2012 Hyper-V are also incompatible with each other, > > when it comes to VHD files, if that is any consolation. > > Okay. I had hoped that Win 7 would be fine as well and we might just get > away with applying the patch, but this we can't possibly ignore. > > > I used sha1sums between the different combination of VHD files filled to > > capacity with random data under VPC, qemu/kvm (with & without the > > patch), and Hyper-V. Without this patch we are an exact math with > > VPC, and with this patch we are an exact match with Hyper-V. > > > > However, perhaps we can support both, depending on who created the VHD > > file. There is a "Creator Application" field in the VHD header - for > > the VPC-created VHD file, that field is ASCII "vpc ". For the Hyper-V > > created VHD file, that field is ASCII "win ". If we see the field as > > "vpc ", we could default to using the CHS geometry found in the > > header - otherwise, do as this patch does, and go by the virtual disk > > size. > > That they fill in different creator strings is good news. This isn't a > nice solution, but then it wasn't nice to change the semantics between > the two versions, so it's better than nothing. I'd vote for comparing > the creator field. +1 > > One open question is what to do with images that are neither "vpc " (or > "qemu") nor "win ". I think the newer behaviour is saner, but I don't > know if there are other tools for which the old semantics should be used. > And it does get a bit trickier than that - it is possible to have a creator field "lie". Hyper-V does not give any warnings or errors when opening the VPC VHD file, and since Hyper-V will just interpret the disk as slightly larger, it will most likely work fine. But it is possible that Hyper-V will then modify the disk in a way that uses the extra space (e.g. the guest reformatis / repartitions the drive). It does not update the creator field, so it still shows the older "vpc ". If we went by that alone we would have the same problem over again. This is kind of a mess, but I think the best solution may be something along the lines of: 1. By default, automatically use the creator field 2. Allow a way for the user to override this behavior, to allow manual recovery from fringe cases > The other question is what to do with newly created images. Maybe we > should add a creation option. At the same time we should probably switch > from the "qemu" creator string to something different if Hyper-V > semantics is used, in order to allow distinguishing the version. I'm > also not sure what the right default would be for now. >
Il 11/02/2013 21:13, Anthony Liguori ha scritto: > Applied. Thanks. > > Regards, > > Anthony Liguori I guess this was a mistake, please revert. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 11/02/2013 21:13, Anthony Liguori ha scritto: >> Applied. Thanks. >> >> Regards, >> >> Anthony Liguori > > I guess this was a mistake, please revert. No, it wasn't. The patch was reviewed and tested. What's the problem. It's proposed for 1.4 so it's now or never. I saw some discussion about improving the detection for the different varients of VHD... I don't mind reverting it, but just want to understand what the issue is. Regards, Anthony Liguori > > Paolo
On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 11/02/2013 21:13, Anthony Liguori ha scritto: > >> Applied. Thanks. > >> > >> Regards, > >> > >> Anthony Liguori > > > > I guess this was a mistake, please revert. > > No, it wasn't. The patch was reviewed and tested. What's the problem. > It's proposed for 1.4 so it's now or never. > > I saw some discussion about improving the detection for the different > varients of VHD... > > I don't mind reverting it, but just want to understand what the issue > is. > Hi Anthony, It was tested, the problem is that this will break existing support for Virtual PC VHD files under certain conditions (most likely exporting from qemu to VPC, but I don't know if we could guarantee that is the only way). Jeff
Am 11.02.2013 20:38, schrieb Jeff Cody: > I've done some testing back and forth between VPC, Win Server 2012 > Hyper-V, and qemu/kvm with and without this patch applied. > > The short: this patch will break Virtual PC VHD compatibility (as it > exists under Win7), but allow compatibility for VHD files created by > Hyper-V (at least under Server 2012). > > VPC and Server 2012 Hyper-V are also incompatible with each other, > when it comes to VHD files, if that is any consolation. > > I used sha1sums between the different combination of VHD files filled to > capacity with random data under VPC, qemu/kvm (with & without the > patch), and Hyper-V. Without this patch we are an exact math with > VPC, and with this patch we are an exact match with Hyper-V. > > However, perhaps we can support both, depending on who created the VHD > file. There is a "Creator Application" field in the VHD header - for > the VPC-created VHD file, that field is ASCII "vpc ". For the Hyper-V > created VHD file, that field is ASCII "win ". If we see the field as > "vpc ", we could default to using the CHS geometry found in the > header - otherwise, do as this patch does, and go by the virtual disk > size. > > Jeff Hi Jeff, could you please give some more details of your VPC test scenario? How did you create the VHD disk imagesfor VPC? Which guest OS did you run? Which disk size does the guest OS report? Does VPC behave differently when it gets disk images with a different signature (for example from Hyper V)? I'd like to reproduce your test results in my environment. Does the patch really break Virtual PC VHD compatibility? Or does VPC simply pass the size calculated from CHSto the guest - no matter how the disk image was created?In this case I see no need that QEMU must show the same behaviour. It's important to implement the VHD specification correctly, but we should not duplicate bugs of other emulators just to be compatible with them. Thanks, Stefan
On Mon, Feb 11, 2013 at 10:33:53PM +0100, Stefan Weil wrote: > Am 11.02.2013 20:38, schrieb Jeff Cody: > > I've done some testing back and forth between VPC, Win Server 2012 > > Hyper-V, and qemu/kvm with and without this patch applied. > > > > The short: this patch will break Virtual PC VHD compatibility (as it > > exists under Win7), but allow compatibility for VHD files created by > > Hyper-V (at least under Server 2012). > > > > VPC and Server 2012 Hyper-V are also incompatible with each other, > > when it comes to VHD files, if that is any consolation. > > > > I used sha1sums between the different combination of VHD files filled to > > capacity with random data under VPC, qemu/kvm (with & without the > > patch), and Hyper-V. Without this patch we are an exact math with > > VPC, and with this patch we are an exact match with Hyper-V. > > > > However, perhaps we can support both, depending on who created the VHD > > file. There is a "Creator Application" field in the VHD header - for > > the VPC-created VHD file, that field is ASCII "vpc ". For the Hyper-V > > created VHD file, that field is ASCII "win ". If we see the field as > > "vpc ", we could default to using the CHS geometry found in the > > header - otherwise, do as this patch does, and go by the virtual disk > > size. > > > > Jeff > > Hi Jeff, > > could you please give some more details of your VPC test scenario? Hello Stefan, Sure, no problem. I will answer each of your questions inline below, but before that perhaps an overview would be useful: Ultimately, it doesn't matter if VPC or Hyper-V created the image, what matters is who fills the data in, or partitions those images. Consider the two cases below: A.) Virtual PC appears to go by the CHS data in the header/footer, as qemu currently does without the patch. B.) Hyper-V, and qemu with the patch, use the virtual disk size. This usually yields a disk size slightly larger than what is seen in A). If you are importing from A->B, then there _likely_ will not be any issues, but of course this cannot be 100% guaranteed. If you are importing from B->A then there _will_ likely be issues, as you are going from a larger drive to smaller drive. This patch just changes which one is more supported from a VHD perspective - Virtual PC, or Hyper-V. If this was fresh code without existing support, I would say being compatible with Hyper-V would be more important. However, there are two important factors: 1.) QEMU currently is able to import and export dynamic VHD files from/to Virtual PC. This would break that.. 2.) I would wager most use cases of Hyper-V would be using VHDX, instead of VHD anyway. I am working currently on implementing VHDX support to address see (my work in progress is over at github: https://github.com/codyprime/qemu-kvm-jtc.git , branch jtc-vhdx-work). 3.) With some more effort, we can reasonably support both use cases, as long as the user has a way to force method if needed. > > How did you create the VHD disk imagesfor VPC? I created the images with Virtual PC under Windows 7. I created a new virtual machine, with a new drive image. I booted from a live ISO to partition the image, and fill it with data. I created images of varying sizes, but all of the same type: dynamic vhd images. The easiest way to replicate the problem is to use a smaller image, because you can fill it up with data without much trouble. For my small image test, I used ~130MB image. I also created images under Hyper-V, of the same type. As you know, those images work fine with the patch. And again, it is not so much the creator, but who partitioned or filled the drive with data. One easy way to reproduce the discrepancy: Using the same image, from either VPC or Hyper-V, inside the guest fill the test image with pseudo-random data, and make note of the sha1sum: e.g., from VPC, inside a linux guest: "cat /dev/urandom > /dev/sdX; sha1sum /dev/sdX" Then, under either Hyper-V or qemu with your patch, verify that sum: "sha1sum /dev/vdX" (qemu w/virtio) "sha1sum /dev/sdX" (Hyper-V) You'll notice that with your patch, the sha1sum will not match the one computed from the guest with VPC as the host. However, if you try qemu _without_ your patch, the sha1sum will match. Both Hyper-V and qemu will compute the same sha1sum, however. You can then do this in reverse, with the same image under Hyper-V: fill it with pseudo-random data, make note of the sha1sum, and check that sha1sum from VPC and qemu. You'll notice the same - with your patch, qemu and Hyper-V agree on sha1sums, and VPC does not. Without your patch, qemu and VPC agree on sha1sums, and Hyper-V does not. > Which guest OS did you run? I used Linux, either Fedora 18 or a lightweight live ISO of puppy Linux (slacko 5.4). Both gave the same results. The version of Linux did not seem to matter (as is to be expected). > Which disk size does the guest OS report? The guest OS under Virtual PC reports a smaller size, as observed by fdisk. For instance, on my smaller image, the disk size under Virtual PC was reported as "134111232 bytes". Under Hyper-V and qemu/kvm with your patch, that same image is reported as "134217728 bytes". Without your patch, qemu again agrees with VPC, with "134111232 bytes". > Does VPC behave differently when it gets disk images with > a different signature (for example from Hyper V)? Yes, and this is the case most likely to have an immediately noticeable failure. Linux refuses to mount the internal partition, because of the incorrect geometry and too-large partition. > > I'd like to reproduce your test results in my environment. > > Does the patch really break Virtual PC VHD compatibility? > Most likely, with this patch import of a VHD will in most cases be fine, because we will just detect a slightly larger disk in qemu. However, if the guest modifies the image, we don't know that it won't do so in a way that will case the image to not be readable by Virtual PC. Most likely, the operation that would cause this would be something like repartitioning the image - but anything that makes use of the slightly large space would naturally lose that data going to Virtual PC. > Or does VPC simply pass the size calculated from CHSto the > guest - no matter how the disk image was created?In this case > I see no need that QEMU must show the same behaviour. > It's important to implement the VHD specification correctly, > but we should not duplicate bugs of other emulators just > to be compatible with them. > I disagree. The purpose of supporting VHD is to be compatible with other emulators, so to the extent possible that we can do that, we should. > Thanks, > > Stefan > Thanks, Jeff
Am 11.02.2013 22:14, schrieb Anthony Liguori: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 11/02/2013 21:13, Anthony Liguori ha scritto: >>> Applied. Thanks. >>> >>> Regards, >>> >>> Anthony Liguori >> >> I guess this was a mistake, please revert. > > No, it wasn't. The patch was reviewed and tested. What's the problem. > It's proposed for 1.4 so it's now or never. The problem is that the test failed. Why do we even test things if we commit anyway when the test fails? And why do we have subsystem trees when a committer comes (without even having taken part in the discussion) and applies patches that the subsystem maintainers are very obviously not happy with? If you like, you can have the block subsystem back, just send - or better just apply - a patch against MAINTAINERS. > I saw some discussion about improving the detection for the different > varients of VHD... > > I don't mind reverting it, but just want to understand what the issue > is. It makes qemu use the wrong size for VHD images created by Virtual PC that used to work with qemu. This is a regression that can result in images appearing corrupted in Virtual PC, and we need very good justification to apply such a patch. At the same time the patch enables the use of the correct size for Hyper-V images, but that has never worked with qemu, so if we can't get it to work with both for 1.4, I'd rather compromise on this one and not apply the patch. Please revert. Kevin
Il 12/02/2013 09:42, Kevin Wolf ha scritto: >> > No, it wasn't. The patch was reviewed and tested. What's the problem. >> > It's proposed for 1.4 so it's now or never. > The problem is that the test failed. Why do we even test things if we > commit anyway when the test fails? > > And why do we have subsystem trees when a committer comes (without even > having taken part in the discussion) and applies patches that the > subsystem maintainers are very obviously not happy with? If you like, > you can have the block subsystem back, just send - or better just apply > - a patch against MAINTAINERS. > I think this is just a problem with the new tools, that didn't make it clear that you were not happy with the patch. (It was clear from the mailing list). In general, the new tools sound like an improvement, but it's normal to have a misunderstanding or two in the initial period. Paolo
Am 11.02.2013 22:33, schrieb Stefan Weil: > Does the patch really break Virtual PC VHD compatibility? > > Or does VPC simply pass the size calculated from CHSto the > guest - no matter how the disk image was created?In this case > I see no need that QEMU must show the same behaviour. > It's important to implement the VHD specification correctly, > but we should not duplicate bugs of other emulators just > to be compatible with them. Bollocks. The whole point of supporting non-native image formats is compatibility with other software. It's a good thing to be able to read standard compliant images, but it's much more important to be able to read real life images. We have real life images of both VPC and Hyper-V, so we need to do our best to support both. Failing that, we need to stay consistent with older qemu versions. Kevin
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 12/02/2013 09:42, Kevin Wolf ha scritto: >>> > No, it wasn't. The patch was reviewed and tested. What's the problem. >>> > It's proposed for 1.4 so it's now or never. >> The problem is that the test failed. Why do we even test things if we >> commit anyway when the test fails? >> >> And why do we have subsystem trees when a committer comes (without even >> having taken part in the discussion) and applies patches that the >> subsystem maintainers are very obviously not happy with? If you like, >> you can have the block subsystem back, just send - or better just apply >> - a patch against MAINTAINERS. >> > > I think this is just a problem with the new tools, that didn't make it > clear that you were not happy with the patch. (It was clear from the > mailing list). > > In general, the new tools sound like an improvement, but it's normal to > have a misunderstanding or two in the initial period. Yup. No need to get excited. Let's revert this patch for 1.4, keeping QEMU exactly as working and as broken as it has always been. Then figure out which additional cases we can make work for 1.5, and which ones we have to break for that, if any.
On Tue, Feb 12, 2013 at 10:12:59AM +0100, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 12/02/2013 09:42, Kevin Wolf ha scritto: > >>> > No, it wasn't. The patch was reviewed and tested. What's the problem. > >>> > It's proposed for 1.4 so it's now or never. > >> The problem is that the test failed. Why do we even test things if we > >> commit anyway when the test fails? > >> > >> And why do we have subsystem trees when a committer comes (without even > >> having taken part in the discussion) and applies patches that the > >> subsystem maintainers are very obviously not happy with? If you like, > >> you can have the block subsystem back, just send - or better just apply > >> - a patch against MAINTAINERS. > >> > > > > I think this is just a problem with the new tools, that didn't make it > > clear that you were not happy with the patch. (It was clear from the > > mailing list). > > > > In general, the new tools sound like an improvement, but it's normal to > > have a misunderstanding or two in the initial period. > > Yup. No need to get excited. > > Let's revert this patch for 1.4, keeping QEMU exactly as working and as > broken as it has always been. Then figure out which additional cases we > can make work for 1.5, and which ones we have to break for that, if any. I'll send a block pull request that includes a revert. Stefan
Am 12.02.2013 09:42, schrieb Kevin Wolf: > It makes qemu use the wrong size for VHD images created by Virtual PC > that used to work with qemu. This is a regression that can result in > images appearing corrupted in Virtual PC, and we need very good > justification to apply such a patch. > > At the same time the patch enables the use of the correct size for > Hyper-V images, but that has never worked with qemu, so if we can't get > it to work with both for 1.4, I'd rather compromise on this one and not > apply the patch. > > Please revert. > > Kevin I'd appreciate if we could use a more precise description of the results and the problems with or without the patch. QEMU does _not_ use a wrong size for any VHD images with the patch. It passes the correct real image size to the guest. This is compatible with the behaviour of Hyper-V, Virtual Box and maybe other emulators which implement the VHD specification as published by Microsoft. According to the tests run, it is incompatible with VPC which passes a calculated CHS size. For images created with QEMU, the calculated CHS size and the real size are always be the same, so these images work in any case. It should also be possible to create such images with any other tool by specifying an image size which is an exact CHS size, but in most cases VHD images will have a size which is larger than the calculated CHS size. As far as we currently know, VPC (and no other tool) passes the calculated CHS size to its guest. Copying such images to any other format using qemu-img works both with and without the patch because all valid data is below the CHS size. Copying any kind of non VHD image to VHD also works with and without the patch because QEMU always calculates an image size based on the CHS size for newly created images. Copying a VHD image which was written beyond the CHS size to VHD needs the patch because otherwise the data behind the CHS size would get lost. The resulting image should work with any emulator, also with VPC, because it again has a real size which is equal to the CHS size. This all means that the patch is still correct, does not introduce a regression and should be in QEMU 1.4. The only problem I see with the patch is my wrong mail address. Anthony's semi-automated commit did not fix the wrong address which I had supplied. Cheers, Stefan W.
Am 11.02.2013 22:20, schrieb Jeff Cody: > On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 11/02/2013 21:13, Anthony Liguori ha scritto: >>>> Applied. Thanks. >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>> I guess this was a mistake, please revert. >> No, it wasn't. The patch was reviewed and tested. What's the problem. >> It's proposed for 1.4 so it's now or never. >> >> I saw some discussion about improving the detection for the different >> varients of VHD... >> >> I don't mind reverting it, but just want to understand what the issue >> is. >> > Hi Anthony, > > It was tested, the problem is that this will break existing support > for Virtual PC VHD files under certain conditions (most likely > exporting from qemu to VPC, but I don't know if we could guarantee > that is the only way). > > > Jeff Hi Jeff, exporting from QEMU to VPCwill work because QEMU uses images where real size = CHS size, so those images work no matter whether the patch was applied or not. See my separate mail for more reasoning. I see no way how the patch could break a realistic test scenario, but I know such scenarios which are broken without the patch. Stefan
Am 12.02.2013 12:24, schrieb Stefan Hajnoczi: > On Tue, Feb 12, 2013 at 10:12:59AM +0100, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 12/02/2013 09:42, Kevin Wolf ha scritto: >>>>>> No, it wasn't. The patch was reviewed and tested. What's the problem. >>>>>> It's proposed for 1.4 so it's now or never. >>>> The problem is that the test failed. Why do we even test things if we >>>> commit anyway when the test fails? >>>> >>>> And why do we have subsystem trees when a committer comes (without even >>>> having taken part in the discussion) and applies patches that the >>>> subsystem maintainers are very obviously not happy with? If you like, >>>> you can have the block subsystem back, just send - or better just apply >>>> - a patch against MAINTAINERS. >>>> >>> I think this is just a problem with the new tools, that didn't make it >>> clear that you were not happy with the patch. (It was clear from the >>> mailing list). >>> >>> In general, the new tools sound like an improvement, but it's normal to >>> have a misunderstanding or two in the initial period. >> Yup. No need to get excited. >> >> Let's revert this patch for 1.4, keeping QEMU exactly as working and as >> broken as it has always been. Then figure out which additional cases we >> can make work for 1.5, and which ones we have to break for that, if any. > I'll send a block pull request that includes a revert. > > Stefan I'm not convinced that this would be a good idea. See my other mail for the reason. Stefan
On Tue, Feb 12, 2013 at 03:47:38PM +0100, Stefan Weil wrote: > Am 11.02.2013 22:20, schrieb Jeff Cody: > > On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >>> Il 11/02/2013 21:13, Anthony Liguori ha scritto: > >>>> Applied. Thanks. > >>>> > >>>> Regards, > >>>> > >>>> Anthony Liguori > >>> I guess this was a mistake, please revert. > >> No, it wasn't. The patch was reviewed and tested. What's the problem. > >> It's proposed for 1.4 so it's now or never. > >> > >> I saw some discussion about improving the detection for the different > >> varients of VHD... > >> > >> I don't mind reverting it, but just want to understand what the issue > >> is. > >> > > Hi Anthony, > > > > It was tested, the problem is that this will break existing support > > for Virtual PC VHD files under certain conditions (most likely > > exporting from qemu to VPC, but I don't know if we could guarantee > > that is the only way). > > > > > > Jeff > > Hi Jeff, > > exporting from QEMU to VPCwill work because QEMU uses > images where real size = CHS size, so those images work > no matter whether the patch was applied or not. > > See my separate mail for more reasoning. > > I see no way how the patch could break a realistic test scenario, > but I know such scenarios which are broken without the patch. > > > Stefan > Hi Stefan, I should have been clearer when I said export - I am talking about qemu modifying an existing VHD image created by VPC, and then trying to use image in VPC again. Here is a simple way to see how the proposed patch breaks current VHD compatibility: 1.) Under VPC, define a new VM that boots from a live dvd iso (I used a puppy linux iso because it was small and quick to test: http://puppylinux.org). For the hard drive for the machine, define a 128GB dynamic drive. (test.vhd). You don't need to boot this VM yet. 2.) Now, under QEMU, boot again to a live iso or existing linux image, and use the newly created test.vhd. Create a single partition, using the fdisk defaults for the maximum size. Shutdown. 3.) Use that same VHD image and boot the live iso with your VPC machine. Try to mount the partition you created under qemu with the guest in VPC. It will fail to detect the partition, and complain about invalid geometry. I think your patch is definitely needed, don't get me wrong, and I can verify that it works - for the bug report, I even came to the same conclusion. I just think it needs to be done such that we don't break something that used to work, and testing shows that it does, unfortunately. Thanks, Jeff
Kevin Wolf <kwolf@redhat.com> writes: > Am 11.02.2013 22:14, schrieb Anthony Liguori: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 11/02/2013 21:13, Anthony Liguori ha scritto: >>>> Applied. Thanks. >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>> >>> I guess this was a mistake, please revert. >> >> No, it wasn't. The patch was reviewed and tested. What's the problem. >> It's proposed for 1.4 so it's now or never. (Note, I already have a revert queued, tested over night, and will push this morning) > The problem is that the test failed. Why do we even test things if we > commit anyway when the test fails? That wasn't at all clear to me. Besides, by the time Jeff's note came, the commit was already pushed. The 'Thanks' note was delayed compared to the actual push. > And why do we have subsystem trees when a committer comes (without even > having taken part in the discussion) and applies patches that the > subsystem maintainers are very obviously not happy with? There are two block maintainers. One of the block maintainers, Stefan, offered a Reviewed-by. > If you like, > you can have the block subsystem back, just send - or better just apply > - a patch against MAINTAINERS. The real problem here was that it was not clear to me whether to expect a pull request for -rc2 or not and from whom. I received very few pull requests during this -rc cycle and most submaintainers have asked me to apply patches directly because of the short cycle. In fact, I reached out twice on IRC attempting to clarify whether I was going to get a block pull request for -rc2 and got no response from you or Stefan. Regards, Anthony Liguori
Am 12.02.2013 16:18, schrieb Jeff Cody: > On Tue, Feb 12, 2013 at 03:47:38PM +0100, Stefan Weil wrote: >> Am 11.02.2013 22:20, schrieb Jeff Cody: >>> On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote: >>>> Paolo Bonzini <pbonzini@redhat.com> writes: >>>> >>>>> Il 11/02/2013 21:13, Anthony Liguori ha scritto: >>>>>> Applied. Thanks. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Anthony Liguori >>>>> I guess this was a mistake, please revert. >>>> No, it wasn't. The patch was reviewed and tested. What's the problem. >>>> It's proposed for 1.4 so it's now or never. >>>> >>>> I saw some discussion about improving the detection for the different >>>> varients of VHD... >>>> >>>> I don't mind reverting it, but just want to understand what the issue >>>> is. >>>> >>> Hi Anthony, >>> >>> It was tested, the problem is that this will break existing support >>> for Virtual PC VHD files under certain conditions (most likely >>> exporting from qemu to VPC, but I don't know if we could guarantee >>> that is the only way). >>> >>> >>> Jeff >> Hi Jeff, >> >> exporting from QEMU to VPCwill work because QEMU uses >> images where real size = CHS size, so those images work >> no matter whether the patch was applied or not. >> >> See my separate mail for more reasoning. >> >> I see no way how the patch could break a realistic test scenario, >> but I know such scenarios which are broken without the patch. >> >> >> Stefan >> > Hi Stefan, > > I should have been clearer when I said export - I am talking about > qemu modifying an existing VHD image created by VPC, and then trying > to use image in VPC again. > > Here is a simple way to see how the proposed patch breaks current VHD > compatibility: > > 1.) Under VPC, define a new VM that boots from a live dvd iso (I used > a puppy linux iso because it was small and quick to test: > http://puppylinux.org). For the hard drive for the machine, define a > 128GB dynamic drive. (test.vhd). You don't need to boot this VM yet. > > 2.) Now, under QEMU, boot again to a live iso or existing linux image, > and use the newly created test.vhd. Create a single partition, using > the fdisk defaults for the maximum size. Shutdown. > > 3.) Use that same VHD image and boot the live iso with your VPC > machine. Try to mount the partition you created under qemu with the > guest in VPC. It will fail to detect the partition, and complain > about invalid geometry. > > I think your patch is definitely needed, don't get me wrong, and I can > verify that it works - for the bug report, I even came to the same > conclusion. I just think it needs to be done such that we don't break > something that used to work, and testing shows that it does, > unfortunately. > > Thanks, > Jeff Hi Jeff, the scenario which you describe here is not one which I'd call "realistic". Why should a user create a VHD image with VPC and then create the partitions using QEMU?Yes, if this is done, the image is no longer compatible with VPC as soon as blocks after the CHS size are used. The same kind of problem would occur if Hyper-V or VirtualBox were use instead of QEMU. I expect that the common use case is a user who creates and uses a VHD image with VPC (this includes partitioning). This kind of image can later be used with QEMU, VirtualBox or Hyper-V without problems as long as the partitions are not modified to use the additional blocks which are available after the CHS size. This restriction seems acceptable for me. I see no way to avoid it. The same kind of restriction exists for real hardware since the introduction of LBA as soon as software (BIOS, bootloaders, DOS, ...) tried using CHS. Regards, Stefan
Stefan Weil <sw@weilnetz.de> writes: > Hi Jeff, > > the scenario which you describe here is not one which I'd call > "realistic". Why should a user create a VHD image with VPC [...] Maybe, but why run a risk for 1.4? Let's delay your fix to 1.5. Gives us plenty of time to hash out the issues.
diff --git a/block/vpc.c b/block/vpc.c index 82229ef..b4ff564 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -34,6 +34,8 @@ #define HEADER_SIZE 512 +#define VHD_SECTOR_SIZE 512 + //#define CACHE enum vhd_type { @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int flags) /* Write 'checksum' back to footer, or else will leave it with zero. */ footer->checksum = be32_to_cpu(checksum); - // 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; + /* The visible size of a image in Virtual PC depends on the guest: + * QEMU and other emulators report the real size (here in sectors). + * All modern operating systems use this real size. + * Very old operating systems use CHS values to calculate the total size. + * This calculated size is usually smaller than the real size. + */ + bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE; /* Allow a maximum disk size of approximately 2 TB */ if (bs->total_sectors >= 65535LL * 255 * 255) {