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

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Eric Blake (eblake@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?
>
>> > 
>> >>>
>> >>> 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.

In my case here, is that we want the "new" functionality to be the
default, and just disable it for the old machine types.

So, for now on, section footers are sent.  Except, if we are on an old
machine type, that we don't sent them then.  So, the only obvious way
that I can think of is naming it as "skip_foo".  Notice that we have
also to accomodate the achitecture that don't care about cross-version
migration, and there, we just want the new behaviour.

If you have a nicer approach on mind, I am all for it.

THanks, Juan.


>
> Dave
>
>> -- 
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>> 
>
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK