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

[Eric Blake]

On 05/19/2015 08:28 AM, Dr. David Alan Gilbert wrote:
> * 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?

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