| Message ID | 1441482708-31848-2-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
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() >
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.
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.
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.
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
* 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
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
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.
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()
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(+)