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

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

[Mark Cave-Ayland]

Whilst investigating a migration issue, I noticed that the 
analyze-migration.py script had accidentally been broken by commit 61964.

I've implemented a quick workaround to parse the configuration section into a
separate Python object, however the solution works by parsing the SaveState
properties directly rather than reading them dynamically. This is because 
vmstate_save_state() doesn't have a valid QJSON object available at the time
the configuration is written out which makes it tricky to include the
configuration state in the JSON schema.

This is probably a reasonable solution if the configuration state is deemed
to be static, although more thought is required if it is deemed useful for
the machine name to be made available in the analyze-migration.py JSON output
or if the configuration properties should be read dynamically.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (1):
  migration: fix analyze-migration.py script

 scripts/analyze-migration.py |   13 +++++++++++++
 1 file changed, 13 insertions(+)

-- 
1.7.10.4

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

[Mark Cave-Ayland]

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()
             elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL:
                 section_id = file.read32()
                 name = file.readstr()
-- 
1.7.10.4

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

[Alexander Graf]

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.

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().


Thanks a lot for the patch :),

Alex

>              elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL:
>                  section_id = file.read32()
>                  name = file.readstr()
> 

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

[Mark Cave-Ayland]

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.

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

[Mark Cave-Ayland]

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 <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.

Ping again from Juan/David? The analyze-migration.py script is currently
broken without this patch (or another equivalent) applied.


ATB,

Mark.

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

[Mark Cave-Ayland]

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 <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.
> 
> 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.


ATB,

Mark.

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

[Alexander Graf]

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


Alex

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

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) 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 <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.
> >>
> >> 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 / dgilbert@redhat.com / Manchester, UK

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

[Alexander Graf]

On 26.11.15 16:31, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@suse.de) 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 <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.
>>>>
>>>> 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.


Alex

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

[Juan Quintela]

Alexander Graf <agraf@suse.de> wrote:
> On 26.11.15 16:31, Dr. David Alan Gilbert wrote:
>> * Alexander Graf (agraf@suse.de) 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 <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.
>>>>>
>>>>> 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.