Re: [PATCH v3 05/14] dump: Split write of section headers and data and add a prepare step

[Janosch Frank]

On 7/21/22 16:41, Marc-André Lureau wrote:
> On Thu, Jul 21, 2022 at 5:23 PM Janosch Frank <frankja@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.
> >
> > At the same time we move the writing of the section to the end of the
> > dump process. This allows the upcoming architecture section code to
> > add data after all of the common dump data has been written.
> >
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   dump/dump.c           | 114 ++++++++++++++++++++++++++++++++----------
> >   include/sysemu/dump.h |   4 ++
> >   2 files changed, 92 insertions(+), 26 deletions(-)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index 2d04e06815..980702d476 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> > @@ -342,30 +342,71 @@ static void write_elf_phdr_note(DumpState *s, Error 
> > **errp)
> >       }
> >   }
> >
> > -static void write_elf_section(DumpState *s, int type, Error **errp)
> > +static size_t prepare_elf_section_hdr_zero(DumpState *s, void *buff)
> >   {
> > -    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;
> > +        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;
> > +
> > +        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);
> > +    buff_hdr = s->elf_section_hdrs;
> > +
> > +    /* Write special section first */
> > +    if (s->phdr_num == PN_XNUM) {
> > +            prepare_elf_section_hdr_zero(s, buff_hdr);
>
>  From v2 review:
>
> Eventually, drop buff_hdr, and pass only "s" as argument

You need to specify what you mean by "eventually" here.

> + Indentation is off

I fixed that Tuesday but somehow it didn't end up in this mail...
I'll have another go at it.

> > +    }
> > +}
> > +
> > +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 headers");
> > +    }
> > +}
> > +
> > +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 +598,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 file descriptor */
> >       write_elf_header(s, errp);
> >       if (*errp) {
> >           return;
> >       }
> >
> > +    write_elf_section_headers(s, errp);
> > +    if (*errp) {
> > +        return;
> > +    }
> > +
> >       /* write PT_NOTE to vmcore */
> >       write_elf_phdr_note(s, errp);
> >       if (*errp) {
> > @@ -575,14 +626,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 +653,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 +676,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 58f41bbf45..dad10dee0b 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