| Message ID | 20221017083822.43118-3-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
On Mon, Oct 17, 2022 at 12:39 PM Janosch Frank <frankja@linux.ibm.com> wrote: > Let's start bundling the writes of the headers and of the data so we > have a clear ordering between them. Since the ELF header uses offsets > to the headers we can freely order them. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > dump/dump.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index e7a3b54ebe..b168a25321 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) > * -------------- > * | elf header | > * -------------- > + * | sctn_hdr | > + * -------------- > * | PT_NOTE | > * -------------- > * | PT_LOAD | > @@ -591,8 +593,6 @@ static void dump_begin(DumpState *s, Error **errp) > * -------------- > * | PT_LOAD | > * -------------- > - * | sec_hdr | > - * -------------- > * | elf note | > * -------------- > * | memory | > @@ -608,6 +608,12 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > + /* write section headers to vmcore */ > + write_elf_section_headers(s, errp); > + if (*errp) { > + return; > + } > + > /* write PT_NOTE to vmcore */ > write_elf_phdr_note(s, errp); > if (*errp) { > @@ -620,12 +626,6 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > - /* write section headers to vmcore */ > - write_elf_section_headers(s, errp); > - if (*errp) { > - return; > - } > - > /* write notes to vmcore */ > write_elf_notes(s, errp); > } > @@ -1868,16 +1868,13 @@ static void dump_init(DumpState *s, int fd, bool > has_format, > } > > if (dump_is_64bit(s)) { > - s->phdr_offset = sizeof(Elf64_Ehdr); > - s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * > s->phdr_num; > - s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * > s->shdr_num; > - s->memory_offset = s->note_offset + s->note_size; > + s->shdr_offset = sizeof(Elf64_Ehdr); > + s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * > s->shdr_num; > + s->note_offset = s->phdr_offset + sizeof(Elf64_Phdr) * > s->phdr_num; > } else { > - > - s->phdr_offset = sizeof(Elf32_Ehdr); > - s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * > s->phdr_num; > - s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * > s->shdr_num; > - s->memory_offset = s->note_offset + s->note_size; > + s->shdr_offset = sizeof(Elf32_Ehdr); > + s->phdr_offset = s->shdr_offset + sizeof(Elf32_Shdr) * > s->shdr_num; > + s->note_offset = s->phdr_offset + sizeof(Elf32_Phdr) * > s->phdr_num; > } > > return; > -- > 2.34.1 > >
Janosch Frank <frankja@linux.ibm.com> writes: > Let's start bundling the writes of the headers and of the data so we > have a clear ordering between them. Since the ELF header uses offsets > to the headers we can freely order them. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index e7a3b54ebe..b168a25321 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) > * -------------- > * | elf header | > * -------------- > + * | sctn_hdr | > + * -------------- While you’re at it, I would suggest to add the location for the program headers (phdr) as well. This would it make easier to understand the memory layout & the code below as well. I guess it looks like: … --------------- | sctn_hdr | --------------- | prog_hdr | --------------- … […snip]
On 10/17/22 14:49, Marc Hartmayer wrote: > Janosch Frank <frankja@linux.ibm.com> writes: > >> Let's start bundling the writes of the headers and of the data so we >> have a clear ordering between them. Since the ELF header uses offsets >> to the headers we can freely order them. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> dump/dump.c | 31 ++++++++++++++----------------- >> 1 file changed, 14 insertions(+), 17 deletions(-) >> >> diff --git a/dump/dump.c b/dump/dump.c >> index e7a3b54ebe..b168a25321 100644 >> --- a/dump/dump.c >> +++ b/dump/dump.c >> @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) >> * -------------- >> * | elf header | >> * -------------- >> + * | sctn_hdr | >> + * -------------- > > While you’re at it, I would suggest to add the location for the program > headers (phdr) as well. This would it make easier to understand the > memory layout & the code below as well. > > I guess it looks like: > > … > --------------- > | sctn_hdr | > --------------- > | prog_hdr | > --------------- > … > > > […snip] > They are already in there, have a look at the PT_* entries. I've left them like this because I assumed that the original author wanted to make a point by having them like this. * -------------- * | elf header | * -------------- * | sctn_hdr | * -------------- * | PT_NOTE | * -------------- * | PT_LOAD | * -------------- * | ...... | * -------------- * | PT_LOAD | * --------------
Janosch Frank <frankja@linux.ibm.com> writes: > On 10/17/22 14:49, Marc Hartmayer wrote: >> Janosch Frank <frankja@linux.ibm.com> writes: >> >>> Let's start bundling the writes of the headers and of the data so we >>> have a clear ordering between them. Since the ELF header uses offsets >>> to the headers we can freely order them. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> dump/dump.c | 31 ++++++++++++++----------------- >>> 1 file changed, 14 insertions(+), 17 deletions(-) >>> >>> diff --git a/dump/dump.c b/dump/dump.c >>> index e7a3b54ebe..b168a25321 100644 >>> --- a/dump/dump.c >>> +++ b/dump/dump.c >>> @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) >>> * -------------- >>> * | elf header | >>> * -------------- >>> + * | sctn_hdr | >>> + * -------------- >> >> While you’re at it, I would suggest to add the location for the program >> headers (phdr) as well. This would it make easier to understand the >> memory layout & the code below as well. >> >> I guess it looks like: >> >> … >> --------------- >> | sctn_hdr | >> --------------- >> | prog_hdr | >> --------------- >> … >> >> >> […snip] >> > > > They are already in there, have a look at the PT_* entries. I've left > them like this because I assumed that the original author wanted to make > a point by having them like this. Makes sense - I mistakenly assumed that these were the actual segment contents. […snip]
diff --git a/dump/dump.c b/dump/dump.c index e7a3b54ebe..b168a25321 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -583,6 +583,8 @@ static void dump_begin(DumpState *s, Error **errp) * -------------- * | elf header | * -------------- + * | sctn_hdr | + * -------------- * | PT_NOTE | * -------------- * | PT_LOAD | @@ -591,8 +593,6 @@ static void dump_begin(DumpState *s, Error **errp) * -------------- * | PT_LOAD | * -------------- - * | sec_hdr | - * -------------- * | elf note | * -------------- * | memory | @@ -608,6 +608,12 @@ static void dump_begin(DumpState *s, Error **errp) return; } + /* write section headers to vmcore */ + write_elf_section_headers(s, errp); + if (*errp) { + return; + } + /* write PT_NOTE to vmcore */ write_elf_phdr_note(s, errp); if (*errp) { @@ -620,12 +626,6 @@ static void dump_begin(DumpState *s, Error **errp) return; } - /* write section headers to vmcore */ - write_elf_section_headers(s, errp); - if (*errp) { - return; - } - /* write notes to vmcore */ write_elf_notes(s, errp); } @@ -1868,16 +1868,13 @@ static void dump_init(DumpState *s, int fd, bool has_format, } if (dump_is_64bit(s)) { - s->phdr_offset = sizeof(Elf64_Ehdr); - s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num; - s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; - s->memory_offset = s->note_offset + s->note_size; + s->shdr_offset = sizeof(Elf64_Ehdr); + s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; + s->note_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num; } else { - - s->phdr_offset = sizeof(Elf32_Ehdr); - s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num; - s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num; - s->memory_offset = s->note_offset + s->note_size; + s->shdr_offset = sizeof(Elf32_Ehdr); + s->phdr_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num; + s->note_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num; } return;
Let's start bundling the writes of the headers and of the data so we have a clear ordering between them. Since the ELF header uses offsets to the headers we can freely order them. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)