Re: [Qemu-devel] [PATCH] block.c, block/vmdk.c: Fixed major bug in VMDK WRITE and READ handling - FIXES DATA CORRUPTION

[Gerhard Wiesinger]

On 09.11.2012 09:26, Paolo Bonzini wrote:
> Il 08/11/2012 20:05, Gerhard Wiesinger ha scritto:
> > Fixed a MAJOR BUG in VMDK files on file boundaries on reads
> > and ALSO ON WRITES WHICH MIGHT CORRUPT THE IMAGE AND DATA!!!!!!
> >
> > Triggered for example with the following VMDK file (partly listed):
> > # Extent description
> > RW 4193792 FLAT "XP-W1-f001.vmdk" 0
> > RW 2097664 FLAT "XP-W1-f002.vmdk" 0
> > RW 4193792 FLAT "XP-W1-f003.vmdk" 0
> > RW 512 FLAT "XP-W1-f004.vmdk" 0
> > RW 4193792 FLAT "XP-W1-f005.vmdk" 0
> > RW 2097664 FLAT "XP-W1-f006.vmdk" 0
> > RW 4193792 FLAT "XP-W1-f007.vmdk" 0
> > RW 512 FLAT "XP-W1-f008.vmdk" 0
> >
> > Patch includes:
> > 1.) Patch fixes wrong calculation on extent boundaries. Especially it
> > fixes the relativeness of the sector number to the current extent.
> Please just fix _this_ part.  Everything else is not necessary for example
> for distributions to fix this.  It's an important bug, so we actually want
> to make that as simple as this.

Sent.

> > 2.) Added debug code to block.c and to block/vmdk.c to verify correctness
> Same here.  Also, please use the tracing infrastructure---a lot of the debug
> messages you're adding, though not all, are in fact already available (not
> saying the others aren't useful!)

Any chance that the patch with debug code only (after some cleaning) 
would be accepted (other modules do debug logging, too)?
I  don't like to do useless work.
Tracing infrastructure is quite limited to function calls only (as far 
as I saw).


> > 3.) Also optimized code which avoids multiplication and uses shifts.
> The compiler can do this for you.
>
> Most importantly, making it more complex for reviewers to find only the
> "interesting" part.
>
> Please check that the attached patch still works.

Made some tests and gcc is really clever in optimizing. Even other 
multipliers (513, 514, ...) than 512 will be optimized to shifts and 
adds until a limit of 512+8 (I think it was that) was reached. :-)

Bugfix only resent.

Ciao,
Gerhard