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

[Gerhard Wiesinger <lists@wiesinger.com>]

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