| Message ID | 1499429683-73361-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
* Alexander Graf (agraf@suse.de) wrote: > The configuration section has a new subsection to transmit the target page > size along with the migration stream. The analyze migration script needs > to learn about that to read configuration streams that were triggering > this subsection to get transmitted. > > With this patch applied, I can successfully analyze migration streams > on AArch64 again. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > scripts/analyze-migration.py | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py > index 1455387..02784f2 100755 > --- a/scripts/analyze-migration.py > +++ b/scripts/analyze-migration.py > @@ -254,12 +254,25 @@ class HTABSection(object): (Note I'm not a particularly python person, so take lightly) > class ConfigurationSection(object): > + QEMU_VM_SUBSECTION = 0x05 > + It's odd, you already have this constant defined twice in this script. > def __init__(self, file): > self.file = file > > def read(self): > name_len = self.file.read32() > name = self.file.readstr(len = name_len) > + oldpos = self.file.tell() > + if self.file.read8() == self.QEMU_VM_SUBSECTION: > + name = self.file.readstr() > + version_id = self.file.read32() > + if name == "configuration/target-page-bits": > + target_page_size = self.file.read32() All of your other references to target_page_size in the script are self.TARGET_PAGE_SIZE. You might want to make the conditional subsection check into a function somewhere, but that's OK for now. Dave > + else: > + raise Exception("Unknown config subsection: %s" % name) > + else: > + # No subsection following, forget that we ever read anything > + self.file.seek(oldpos) > > class VMSDFieldGeneric(object): > def __init__(self, desc, file): > -- > 1.8.5.6 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07.07.17 17:00, Dr. David Alan Gilbert wrote: > * Alexander Graf (agraf@suse.de) wrote: >> The configuration section has a new subsection to transmit the target page >> size along with the migration stream. The analyze migration script needs >> to learn about that to read configuration streams that were triggering >> this subsection to get transmitted. >> >> With this patch applied, I can successfully analyze migration streams >> on AArch64 again. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> scripts/analyze-migration.py | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py >> index 1455387..02784f2 100755 >> --- a/scripts/analyze-migration.py >> +++ b/scripts/analyze-migration.py >> @@ -254,12 +254,25 @@ class HTABSection(object): > > (Note I'm not a particularly python person, so take lightly) > >> class ConfigurationSection(object): >> + QEMU_VM_SUBSECTION = 0x05 >> + > > It's odd, you already have this constant defined twice in this script. Yes, it lives once per class. I am not sure how to easily make it a global. > >> def __init__(self, file): >> self.file = file >> >> def read(self): >> name_len = self.file.read32() >> name = self.file.readstr(len = name_len) >> + oldpos = self.file.tell() >> + if self.file.read8() == self.QEMU_VM_SUBSECTION: >> + name = self.file.readstr() >> + version_id = self.file.read32() >> + if name == "configuration/target-page-bits": >> + target_page_size = self.file.read32() > > All of your other references to target_page_size in the script > are self.TARGET_PAGE_SIZE. Well, all other ones are actual page sizes :). This variable is never used anywhere - we just want to stash it somewhere. > You might want to make the conditional subsection check into a function > somewhere, but that's OK for now. I really hope we don't need it again. All subsections should be described via the in-stream json description. We just missed out the configuration one because it's not part of the object model. Alex
* Alexander Graf (agraf@suse.de) wrote: > > > On 07.07.17 17:00, Dr. David Alan Gilbert wrote: > > * Alexander Graf (agraf@suse.de) wrote: > > > The configuration section has a new subsection to transmit the target page > > > size along with the migration stream. The analyze migration script needs > > > to learn about that to read configuration streams that were triggering > > > this subsection to get transmitted. > > > > > > With this patch applied, I can successfully analyze migration streams > > > on AArch64 again. > > > > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > --- > > > scripts/analyze-migration.py | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py > > > index 1455387..02784f2 100755 > > > --- a/scripts/analyze-migration.py > > > +++ b/scripts/analyze-migration.py > > > @@ -254,12 +254,25 @@ class HTABSection(object): > > > > (Note I'm not a particularly python person, so take lightly) > > > > > class ConfigurationSection(object): > > > + QEMU_VM_SUBSECTION = 0x05 > > > + > > > > It's odd, you already have this constant defined twice in this script. > > Yes, it lives once per class. I am not sure how to easily make it a global. > > > > > > def __init__(self, file): > > > self.file = file > > > def read(self): > > > name_len = self.file.read32() > > > name = self.file.readstr(len = name_len) > > > + oldpos = self.file.tell() > > > + if self.file.read8() == self.QEMU_VM_SUBSECTION: > > > + name = self.file.readstr() > > > + version_id = self.file.read32() > > > + if name == "configuration/target-page-bits": > > > + target_page_size = self.file.read32() > > > > All of your other references to target_page_size in the script > > are self.TARGET_PAGE_SIZE. > > Well, all other ones are actual page sizes :). This variable is never used > anywhere - we just want to stash it somewhere. oh, so you're not actually using the value read here to influence the parsing later? > > You might want to make the conditional subsection check into a function > > somewhere, but that's OK for now. > > I really hope we don't need it again. All subsections should be described > via the in-stream json description. We just missed out the configuration one > because it's not part of the object model. OK. Dave > > > Alex -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07.07.17 17:06, Dr. David Alan Gilbert wrote: > * Alexander Graf (agraf@suse.de) wrote: >> >> >> On 07.07.17 17:00, Dr. David Alan Gilbert wrote: >>> * Alexander Graf (agraf@suse.de) wrote: >>>> The configuration section has a new subsection to transmit the target page >>>> size along with the migration stream. The analyze migration script needs >>>> to learn about that to read configuration streams that were triggering >>>> this subsection to get transmitted. >>>> >>>> With this patch applied, I can successfully analyze migration streams >>>> on AArch64 again. >>>> >>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>> --- >>>> scripts/analyze-migration.py | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py >>>> index 1455387..02784f2 100755 >>>> --- a/scripts/analyze-migration.py >>>> +++ b/scripts/analyze-migration.py >>>> @@ -254,12 +254,25 @@ class HTABSection(object): >>> >>> (Note I'm not a particularly python person, so take lightly) >>> >>>> class ConfigurationSection(object): >>>> + QEMU_VM_SUBSECTION = 0x05 >>>> + >>> >>> It's odd, you already have this constant defined twice in this script. >> >> Yes, it lives once per class. I am not sure how to easily make it a global. >> >>> >>>> def __init__(self, file): >>>> self.file = file >>>> def read(self): >>>> name_len = self.file.read32() >>>> name = self.file.readstr(len = name_len) >>>> + oldpos = self.file.tell() >>>> + if self.file.read8() == self.QEMU_VM_SUBSECTION: >>>> + name = self.file.readstr() >>>> + version_id = self.file.read32() >>>> + if name == "configuration/target-page-bits": >>>> + target_page_size = self.file.read32() >>> >>> All of your other references to target_page_size in the script >>> are self.TARGET_PAGE_SIZE. >> >> Well, all other ones are actual page sizes :). This variable is never used >> anywhere - we just want to stash it somewhere. > > oh, so you're not actually using the value read here to influence the > parsing later? Exactly. As far as I can tell it doesn't have any impact on the migration format, so there's no need to use it. Alex
* Alexander Graf (agraf@suse.de) wrote: > > > On 07.07.17 17:00, Dr. David Alan Gilbert wrote: > > * Alexander Graf (agraf@suse.de) wrote: > > > The configuration section has a new subsection to transmit the target page > > > size along with the migration stream. The analyze migration script needs > > > to learn about that to read configuration streams that were triggering > > > this subsection to get transmitted. > > > > > > With this patch applied, I can successfully analyze migration streams > > > on AArch64 again. > > > > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > --- > > > scripts/analyze-migration.py | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py > > > index 1455387..02784f2 100755 > > > --- a/scripts/analyze-migration.py > > > +++ b/scripts/analyze-migration.py > > > @@ -254,12 +254,25 @@ class HTABSection(object): > > > > (Note I'm not a particularly python person, so take lightly) > > > > > class ConfigurationSection(object): > > > + QEMU_VM_SUBSECTION = 0x05 > > > + > > > > It's odd, you already have this constant defined twice in this script. > > Yes, it lives once per class. I am not sure how to easily make it a global. > > > > > > def __init__(self, file): > > > self.file = file > > > def read(self): > > > name_len = self.file.read32() > > > name = self.file.readstr(len = name_len) > > > + oldpos = self.file.tell() > > > + if self.file.read8() == self.QEMU_VM_SUBSECTION: > > > + name = self.file.readstr() > > > + version_id = self.file.read32() > > > + if name == "configuration/target-page-bits": > > > + target_page_size = self.file.read32() > > > > All of your other references to target_page_size in the script > > are self.TARGET_PAGE_SIZE. > > Well, all other ones are actual page sizes :). This variable is never used > anywhere - we just want to stash it somewhere. Well you do read self.TARGET_PAGE_SIZE elsewhere - it determines the amount of data to read from each RAM header in RamSections::read Also note that the value being read in the config header is target_page_bits - i.e. ln2(target_page_size). > > You might want to make the conditional subsection check into a function > > somewhere, but that's OK for now. > > I really hope we don't need it again. All subsections should be described > via the in-stream json description. We just missed out the configuration one > because it's not part of the object model. Dave > > Alex -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 07, 2017 at 02:14:43PM +0200, Alexander Graf wrote: > The configuration section has a new subsection to transmit the target page > size along with the migration stream. The analyze migration script needs > to learn about that to read configuration streams that were triggering > this subsection to get transmitted. > > With this patch applied, I can successfully analyze migration streams > on AArch64 again. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > scripts/analyze-migration.py | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py > index 1455387..02784f2 100755 > --- a/scripts/analyze-migration.py > +++ b/scripts/analyze-migration.py > @@ -254,12 +254,25 @@ class HTABSection(object): > > > class ConfigurationSection(object): > + QEMU_VM_SUBSECTION = 0x05 > + > def __init__(self, file): > self.file = file > > def read(self): > name_len = self.file.read32() > name = self.file.readstr(len = name_len) > + oldpos = self.file.tell() > + if self.file.read8() == self.QEMU_VM_SUBSECTION: Here name_len missing? name_len = self.file.read8() (though all of them are going to be threw away...) > + name = self.file.readstr() > + version_id = self.file.read32() > + if name == "configuration/target-page-bits": > + target_page_size = self.file.read32() > + else: > + raise Exception("Unknown config subsection: %s" % name) > + else: > + # No subsection following, forget that we ever read anything > + self.file.seek(oldpos) I guess we need self.file.file.seek(oldpos) here? Since otherwise on x86 we'll get: Traceback (most recent call last): File "./analyze-migration.py", line 615, in <module> dump.read(dump_memory = args.memory) File "./analyze-migration.py", line 542, in read section.read() File "./analyze-migration.py", line 275, in read self.file.seek(oldpos) AttributeError: 'MigrationFile' object has no attribute 'seek' Thanks,
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py index 1455387..02784f2 100755 --- a/scripts/analyze-migration.py +++ b/scripts/analyze-migration.py @@ -254,12 +254,25 @@ class HTABSection(object): class ConfigurationSection(object): + QEMU_VM_SUBSECTION = 0x05 + def __init__(self, file): self.file = file def read(self): name_len = self.file.read32() name = self.file.readstr(len = name_len) + oldpos = self.file.tell() + if self.file.read8() == self.QEMU_VM_SUBSECTION: + name = self.file.readstr() + version_id = self.file.read32() + if name == "configuration/target-page-bits": + target_page_size = self.file.read32() + else: + raise Exception("Unknown config subsection: %s" % name) + else: + # No subsection following, forget that we ever read anything + self.file.seek(oldpos) class VMSDFieldGeneric(object): def __init__(self, desc, file):
The configuration section has a new subsection to transmit the target page size along with the migration stream. The analyze migration script needs to learn about that to read configuration streams that were triggering this subsection to get transmitted. With this patch applied, I can successfully analyze migration streams on AArch64 again. Signed-off-by: Alexander Graf <agraf@suse.de> --- scripts/analyze-migration.py | 13 +++++++++++++ 1 file changed, 13 insertions(+)