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

[Juan Quintela]

Alexander Graf <address@hidden> wrote:
> On 26.11.15 16:31, Dr. David Alan Gilbert wrote:
>> * Alexander Graf (address@hidden) wrote:
>>>
>>>
>>> On 30.10.15 17:50, Mark Cave-Ayland wrote:
>>>> On 26/10/15 09:48, Mark Cave-Ayland wrote:
>>>>
>>>>> On 06/09/15 12:54, Mark Cave-Ayland wrote:
>>>>>
>>>>>> 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 <address@hidden>
>>>>>>>> ---
>>>>>>>>  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.
>>>>>
>>>>> Ping again from Juan/David? The analyze-migration.py script is currently
>>>>> broken without this patch (or another equivalent) applied.
>>>>
>>>> Ping^2? FWIW I've added this to the wiki at
>>>> http://wiki.qemu.org/Planning/2.5 since without this patch or something
>>>> similar applied, this feature is completely broken.
>>>
>>> Juan, David?
>> 
>> Isn't this already in ( 96e5c9bc77acef8b7b56cbe23a8a2611feff9e34 ) - or is
>> this a different breakage?
>
> Awesome, I didn't see an accepted email :). Sorry for the fuss.

Opps, my fault.

I do that by hand, and then sometimes I forget.

Later, Juan.