Re: [PATCH v4 11/17] dump/dump: Add section string table support

[Janosch Frank]

On 7/26/22 13:25, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> > As sections don't have a type like the notes do we need another way to
> > determine their contents. The string table allows us to assign each
> > section an identification string which architectures can then use to
> > tag their sections with.
> >
> > There will be no string table if the architecture doesn't add custom
> > sections which are introduced in a following patch.
> >
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   dump/dump.c           | 81 ++++++++++++++++++++++++++++++++++++++++++-
> >   include/sysemu/dump.h |  1 +
> >   2 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index 3cf846d0a0..298a1e923f 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> > @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s)
> >       close(s->fd);
> >       g_free(s->guest_note);
> >       g_free(s->elf_header);
> > +    g_array_unref(s->string_table_buf);
> >       s->guest_note = NULL;
> >       if (s->resume) {
> >           if (s->detached) {
> > @@ -357,21 +358,72 @@ static size_t prepare_elf_section_hdr_zero(DumpState *s)
> >       return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> >   }
> >
> > +static void write_elf_section_hdr_string(DumpState *s, void *buff)
> > +{
>
> Mildly annoyed that we use "write_" prefix for actually writing to the
> fd, and sometimes to pre-fill (or "prepare_" structures). Would you
> mind cleaning that up?

Yes, absolutely

> > +    Elf32_Shdr shdr32;
> > +    Elf64_Shdr shdr64;
> > +    int shdr_size;
> > +    void *shdr = buff;
>
> Why assign here?

Great question

> > +
> > +    if (dump_is_64bit(s)) {
> > +        shdr_size = sizeof(Elf64_Shdr);
> > +        memset(&shdr64, 0, shdr_size);
> > +        shdr64.sh_type = SHT_STRTAB;
> > +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
> > +        shdr64.sh_name = s->string_table_buf->len;
> > +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> > +        shdr64.sh_size = s->string_table_buf->len;
> > +        shdr = &shdr64;
> > +    } else {
> > +        shdr_size = sizeof(Elf32_Shdr);
> > +        memset(&shdr32, 0, shdr_size);
> > +        shdr32.sh_type = SHT_STRTAB;
> > +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
> > +        shdr32.sh_name = s->string_table_buf->len;
> > +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> > +        shdr32.sh_size = s->string_table_buf->len;
> > +        shdr = &shdr32;
> > +    }
> > +
> > +    memcpy(buff, shdr, shdr_size);
> > +}
> > +
> >   static void prepare_elf_section_hdrs(DumpState *s)
> >   {
> >       size_t len, sizeof_shdr;
> > +    Elf64_Ehdr *hdr64 = s->elf_header;
> > +    Elf32_Ehdr *hdr32 = s->elf_header;
> > +    void *buff_hdr;
> >
> >       /*
> >        * Section ordering:
> >        * - HDR zero (if needed)
> > +     * - String table hdr
> >        */
> >       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 += sizeof_shdr;
> > +    }
> > +
> > +    if (s->shdr_num < 2) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * String table needs to be last section since strings are added
> > +     * via arch_sections_write_hdr().
> > +     */
>
> This comment isn't exactly relevant yet, since that code isn't there, but ok.

What about something like this, it's a bit more precise and I'll add the 
arch_sections_write_hdr() reference in the next patch?

/*
* String table needs to be the last section since it will be populated 
when adding other elf structures.
*/

[..]
> >       s->length = length;
> > +    /* First index is 0, it's the special null name */
> > +    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> > +    /*
> > +     * Allocate the null name, due to the clearing option set to true
> > +     * it will be 0.
> > +     */
> > +    g_array_set_size(s->string_table_buf, 1);
>
> I wonder if GByteArray wouldn't be more appropriate, even if it
> doesn't have the clearing option. If it's just for one byte, ...

I don't really care but I need a decision on it to change it :)