Hi On Wed, Jul 13, 2022 at 5:07 PM Janosch Frank <fran...@linux.ibm.com> wrote: > > By splitting the writing of the section headers and (future) section > data we prepare for the addition of a string table section and > architecture sections. > > Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > dump/dump.c | 116 ++++++++++++++++++++++++++++++++---------- > include/sysemu/dump.h | 4 ++ > 2 files changed, 94 insertions(+), 26 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 16d7474258..467d934bc1 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -342,30 +342,73 @@ static void write_elf_phdr_note(DumpState *s, Error > **errp) > } > } > > -static void write_elf_section(DumpState *s, int type, Error **errp) > +static size_t write_elf_section_hdr_zero(DumpState *s, void *buff)
Since the function no longer write, I'd suggest to rename it with prepare_ prefix > { > - Elf32_Shdr shdr32; > - Elf64_Shdr shdr64; > - int shdr_size; > - void *shdr; > - int ret; > + if (dump_is_64bit(s)) { > + Elf64_Shdr *shdr64 = buff; > > - if (type == 0) { > - shdr_size = sizeof(Elf32_Shdr); > - memset(&shdr32, 0, shdr_size); > - shdr32.sh_info = cpu_to_dump32(s, s->phdr_num); > - shdr = &shdr32; > + memset(buff, 0, sizeof(Elf64_Shdr)); You can drop this > + shdr64->sh_info = cpu_to_dump32(s, s->phdr_num); > } else { > - shdr_size = sizeof(Elf64_Shdr); > - memset(&shdr64, 0, shdr_size); > - shdr64.sh_info = cpu_to_dump32(s, s->phdr_num); > - shdr = &shdr64; > + Elf32_Shdr *shdr32 = buff; > + > + memset(buff, 0, sizeof(Elf32_Shdr)); and this > + shdr32->sh_info = cpu_to_dump32(s, s->phdr_num); > } > > - ret = fd_write_vmcore(shdr, shdr_size, s); > + return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > +} > + > +static void prepare_elf_section_hdrs(DumpState *s) > +{ > + uint8_t *buff_hdr; > + size_t len, sizeof_shdr; > + > + /* > + * Section ordering: > + * - HDR zero (if needed) > + */ > + sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > + len = sizeof_shdr * s->shdr_num; > + s->elf_section_hdrs = g_malloc0(len); since you alloc0 here > + buff_hdr = s->elf_section_hdrs; > + > + /* Write special section first */ > + if (s->phdr_num == PN_XNUM) { > + write_elf_section_hdr_zero(s, buff_hdr); Eventually, drop buff_hdr, and pass only "s" as argument + Indentation is off > + } > +} > + > +static void prepare_elf_sections(DumpState *s, Error **errp) > +{ > + if (!s->shdr_num) { > + return; > + } > + > + prepare_elf_section_hdrs(s); > +} > + > +static void write_elf_section_headers(DumpState *s, Error **errp) > +{ > + size_t sizeof_shdr; > + int ret; > + > + sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > + > + ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s); > if (ret < 0) { > - error_setg_errno(errp, -ret, > - "dump: failed to write section header table"); > + error_setg_errno(errp, -ret, "dump: failed to write section data"); nit: data->header > + } > +} > + > +static void write_elf_sections(DumpState *s, Error **errp) > +{ > + int ret; > + > + /* Write section zero */ > + ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "dump: failed to write section data"); > } > } > > @@ -557,12 +600,22 @@ static void dump_begin(DumpState *s, Error **errp) > /* Write elf header to buffer */ > prepare_elf_header(s); > > + prepare_elf_sections(s, errp); > + if (*errp) { > + return; > + } > + > /* Start to write stuff into files*/ > write_elf_header(s, errp); > if (*errp) { > return; > } > > + write_elf_section_headers(s, errp); Why do you reorder the sections? Could you explain in the commit message why? Is this is format compliant? and update the comment above? thanks > + if (*errp) { > + return; > + } > + > /* write PT_NOTE to vmcore */ > write_elf_phdr_note(s, errp); > if (*errp) { > @@ -575,14 +628,6 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > - /* write section to vmcore */ > - if (s->shdr_num) { > - write_elf_section(s, 1, errp); > - if (*errp) { > - return; > - } > - } > - > /* write notes to vmcore */ > write_elf_notes(s, errp); > } > @@ -610,6 +655,19 @@ static void dump_iterate(DumpState *s, Error **errp) > } > } > > +static void dump_end(DumpState *s, Error **errp) > +{ > + ERRP_GUARD(); > + > + if (!s->elf_section_data_size) { > + return; > + } > + s->elf_section_data = g_malloc0(s->elf_section_data_size); > + > + /* write sections to vmcore */ > + write_elf_sections(s, errp); > +} > + > static void create_vmcore(DumpState *s, Error **errp) > { > ERRP_GUARD(); > @@ -620,6 +678,12 @@ static void create_vmcore(DumpState *s, Error **errp) > } > > dump_iterate(s, errp); > + if (*errp) { > + return; > + } > + > + /* Write section data after memory has been dumped */ > + dump_end(s, errp); > } > > static int write_start_flat_header(int fd) > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 736f681d01..bd49532232 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -172,6 +172,10 @@ typedef struct DumpState { > int64_t length; /* Length of the dump we want to dump */ > > void *elf_header; > + void *elf_section_hdrs; > + uint64_t elf_section_data_size; > + void *elf_section_data; > + > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */ > uint32_t nr_cpus; /* number of guest's cpu */ > -- > 2.34.1 >