Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) 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?

> > 
> >>>
> >>> The footers are tied to new machine types (on both pc types).
> >>
> >> Good that you tied it to machine type, but is it enough?  When we added
> >> the optional section for giving the json representation of the stream,
> >> we ended up having to add a knob to turn off that section, so that
> >> backwards migration from a new qemu to an older one did not send it.
> >> I'm wondering if we'll need to expose a knob to turn off footers, again
> >> for the sake of backwards migration in downstream distros.
> > 
> > That knob is already the knob that I've created and tied to the machine
> > type; the downstream distros will just turn the same knob in their old
> > machine types.
> 
> 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!).

> [uggh - why'd we give that option an inverted name? Just so we could
> have a default of off?  It might have been nicer as -machine
> send-vmdesc=off with a default of on for new machine types]

We seem to do that a lot - and indeed my code does that to keep
it consistent with the other uses.  I think we like to assume 0 initialised
flags do the 'normal' thing;  still nothing like a double or triple negative
to keep you on your guard.

Dave

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK