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

[Dr. David Alan Gilbert]

* 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?

Dave

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