On 05/19/2015 08:28 AM, Dr. David Alan Gilbert wrote: > * Eric Blake (ebl...@redhat.com) wrote: >> On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote: >> >>>> Does it let us detect a corrupted >>>> stream earlier in the process? Or is the main benefit that it gives >>>> better error messages at the point corruption is first detected? >>> >>> Both; there are two cases that often happen; both triggered by a section >>> reading too little or too much, and it gets back to the main loop and >>> we read the next byte: >>> 1) the next byte on the stream is a 0x00 - that's read as an >>> end-of-migration >>> marker, we start the VM and you get a hung VM with no errors. >>> >>> 2) the next byte is between 0x01..0x04 - and it looks like a section >>> header, >>> then we try and read the next few bytes to figure out which section; >>> this could a) result in an error saying it's an unknown section or >>> b) Happen to match a section ID and then get an error about a problem >>> in that section. In either case you don't get an error pointing to >>> the previous section which was the actual problem. >> >> Probably worth incorporating into the commit body then :) > > Well the original text does say: > Badly formatted migration streams can go undetected or produce > misleading errors due to a lock of checking at the end of sections. > In particular a section that adds an extra 0x00 at the end > causes what looks like a normal end of stream and thus doesn't produce > any errors, and something that ends in a 0x01..0x04 kind of look > like real section headers and then fail when the section parser tries > to figure out which section they are. This is made worse by the > choice of 0x00..0x04 being small numbers that are particularly common > in normal section data. > > which is pretty close to that; do you want me to flip that other explanation > in?
Up to you, but I found the bullet points a bit more descriptive (for example, bullet 1 mentions a hung VM, while the original text says "no errors" but doesn't mention "hung"). >> I'm not asking about the machine type defaults, but a command line >> override: -machine suppress-vmdesc=on, commit 9850c604 > > That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after > the end marker which broke some implicit rules about the behaviour > of the streams and the way they interacted with the file buffers. > I'd have sympathy for the opposite direction - i.e. turning on the > footer protection for older machine types when you know you've got > modern qemu's; but it doesn't seem worth the extra boiler plate unless > someone wants to do it (especially since it looks from that like it takes > two functions and ~20 lines of code to add one boolean flag to the machine > type!). We may still add it later. And since suppress-vmdesc was added later, I can live with the decision if you don't want to add command-line override on the initial commit series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
