Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 06/09/15 09:36, Alexander Graf wrote:

> On 05.09.15 21:51, Mark Cave-Ayland wrote:
>> Commit 61964 "Add configuration section" broke the analyze-migration.py script
>> which terminates due to the unrecognised section. Fix the script by parsing
>> the contents of the configuration section directly into a new
>> ConfigurationSection object (although nothing is done with it yet).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  scripts/analyze-migration.py |   13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>> index f6894be..1455387 100755
>> --- a/scripts/analyze-migration.py
>> +++ b/scripts/analyze-migration.py
>> @@ -252,6 +252,15 @@ class HTABSection(object):
>>      def getDict(self):
>>          return ""
>>  
>> +
>> +class ConfigurationSection(object):
>> +    def __init__(self, file):
>> +        self.file = file
>> +
>> +    def read(self):
>> +        name_len = self.file.read32()
>> +        name = self.file.readstr(len = name_len)
>> +
>>  class VMSDFieldGeneric(object):
>>      def __init__(self, desc, file):
>>          self.file = file
>> @@ -474,6 +483,7 @@ class MigrationDump(object):
>>      QEMU_VM_SECTION_FULL  = 0x04
>>      QEMU_VM_SUBSECTION    = 0x05
>>      QEMU_VM_VMDESCRIPTION = 0x06
>> +    QEMU_VM_CONFIGURATION = 0x07
>>      QEMU_VM_SECTION_FOOTER= 0x7e
>>  
>>      def __init__(self, filename):
>> @@ -514,6 +524,9 @@ class MigrationDump(object):
>>              section_type = file.read8()
>>              if section_type == self.QEMU_VM_EOF:
>>                  break
>> +            elif section_type == self.QEMU_VM_CONFIGURATION:
>> +                section = ConfigurationSection(file)
>> +                section.read()
> 
> So since we don't have a normal section header, there is no version
> field either. That in turn means that the format is determined by the
> machine version only - bleks.

Yes :(  I double-checked the output of a migration file with hexdump and
confirmed that just the raw fields are included without any additional
metadata, even though the state is held in a VMStateDescription.

> So if there ever has to be more in the configuration section than the
> machine name, please move to a more detectable scheme. Ideally something
> that contains
> 
>   * version
>   * length of dynamically sized fields
>   * lenght of full blob
> 
> would be ideal, so that we have a chance to at least put code into the
> analyze script to examine it.
> 
> For now, I think the hard coded solution in the analyze script is
> reasonable.
> 
> However, I think we should print out the name if we find it. It should
> be as simple as adding a special case for the configuration section in
> MigrationDump.getDict().

I did play with adding a separate JSON object for configuration but was
torn between whether configuration should have its own JSON object
(better if we include extra fields and metadata as above) or to just
embed it as a simple "machine" property similar to "page_size". I'll
wait until we hear back from David/Juan and submit a v2 accordingly.


ATB,

Mark.