* [Qemu-devel] [PATCH] migration: fix analyze-migration.py script @ 2015-09-05 19:51 Mark Cave-Ayland 2015-09-05 19:51 ` Mark Cave-Ayland 0 siblings, 1 reply; 10+ messages in thread From: Mark Cave-Ayland @ 2015-09-05 19:51 UTC (permalink / raw) To: quintela, agraf, qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-09-05 19:51 [Qemu-devel] [PATCH] migration: fix analyze-migration.py script Mark Cave-Ayland @ 2015-09-05 19:51 ` Mark Cave-Ayland 2015-09-06 8:36 ` Alexander Graf 0 siblings, 1 reply; 10+ messages in thread From: Mark Cave-Ayland @ 2015-09-05 19:51 UTC (permalink / raw) To: quintela, agraf, qemu-devel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-09-05 19:51 ` Mark Cave-Ayland @ 2015-09-06 8:36 ` Alexander Graf 2015-09-06 11:54 ` Mark Cave-Ayland 0 siblings, 1 reply; 10+ messages in thread From: Alexander Graf @ 2015-09-06 8:36 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: qemu-devel, dgilbert, quintela 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() > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-09-06 8:36 ` Alexander Graf @ 2015-09-06 11:54 ` Mark Cave-Ayland 2015-10-26 9:48 ` Mark Cave-Ayland 0 siblings, 1 reply; 10+ messages in thread From: Mark Cave-Ayland @ 2015-09-06 11:54 UTC (permalink / raw) To: Alexander Graf; +Cc: quintela, qemu-devel, dgilbert 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-09-06 11:54 ` Mark Cave-Ayland @ 2015-10-26 9:48 ` Mark Cave-Ayland 2015-10-30 16:50 ` Mark Cave-Ayland 0 siblings, 1 reply; 10+ messages in thread From: Mark Cave-Ayland @ 2015-10-26 9:48 UTC (permalink / raw) To: Alexander Graf; +Cc: qemu-devel, dgilbert, quintela 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-10-26 9:48 ` Mark Cave-Ayland @ 2015-10-30 16:50 ` Mark Cave-Ayland 2015-11-26 15:23 ` Alexander Graf 0 siblings, 1 reply; 10+ messages in thread From: Mark Cave-Ayland @ 2015-10-30 16:50 UTC (permalink / raw) To: Alexander Graf; +Cc: quintela, qemu-devel, dgilbert 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-10-30 16:50 ` Mark Cave-Ayland @ 2015-11-26 15:23 ` Alexander Graf 2015-11-26 15:31 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 10+ messages in thread From: Alexander Graf @ 2015-11-26 15:23 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: quintela, qemu-devel, dgilbert 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-11-26 15:23 ` Alexander Graf @ 2015-11-26 15:31 ` Dr. David Alan Gilbert 2015-11-26 15:32 ` Alexander Graf 0 siblings, 1 reply; 10+ messages in thread From: Dr. David Alan Gilbert @ 2015-11-26 15:31 UTC (permalink / raw) To: Alexander Graf; +Cc: Mark Cave-Ayland, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-11-26 15:31 ` Dr. David Alan Gilbert @ 2015-11-26 15:32 ` Alexander Graf 2015-11-26 21:40 ` Juan Quintela 0 siblings, 1 reply; 10+ messages in thread From: Alexander Graf @ 2015-11-26 15:32 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: Mark Cave-Ayland, qemu-devel, quintela 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: fix analyze-migration.py script 2015-11-26 15:32 ` Alexander Graf @ 2015-11-26 21:40 ` Juan Quintela 0 siblings, 0 replies; 10+ messages in thread From: Juan Quintela @ 2015-11-26 21:40 UTC (permalink / raw) To: Alexander Graf; +Cc: Mark Cave-Ayland, Dr. David Alan Gilbert, qemu-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-11-26 21:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-05 19:51 [Qemu-devel] [PATCH] migration: fix analyze-migration.py script Mark Cave-Ayland 2015-09-05 19:51 ` Mark Cave-Ayland 2015-09-06 8:36 ` Alexander Graf 2015-09-06 11:54 ` Mark Cave-Ayland 2015-10-26 9:48 ` Mark Cave-Ayland 2015-10-30 16:50 ` Mark Cave-Ayland 2015-11-26 15:23 ` Alexander Graf 2015-11-26 15:31 ` Dr. David Alan Gilbert 2015-11-26 15:32 ` Alexander Graf 2015-11-26 21:40 ` Juan Quintela
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).