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

[Janosch Frank]

On 7/29/22 21:35, Janis Schoetterl-Glausch wrote:
> On 7/26/22 11:22, Janosch Frank wrote:
> > As sections don't have a type like the notes do we need another way to
>
> Having a string table seems like a good idea to me, as we don't know
> the requirements any architecture might have, but sections do have sh_type.
> Could we use one of those, e.g. one of the processor specific ones?

I'd like to avoid defining more constants in elf.h if at all possible.

> Is
>         SHT_PROGBITS
>                 The section holds information defined by the program,
>                 whose format and meaning are determined solely by the program.
> appropriate for us? It seems to me that our program is the guest and
> it doesn't determine the meta information on how to decrypt the dump.

SHT_PROGBITS is being set in the last patch (PV dump) for our arch 
headers. The architecture writes the full shdr into the header buffer 
and can set any type.

> > 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>
> > ---
[...]
> > +    /*
> > +     * String table needs to be last section since strings are added
> > +     * via arch_sections_write_hdr().
> > +     */
> > +    write_elf_section_hdr_string(s, buff_hdr);
> > +    if (dump_is_64bit(s)) {
> > +        hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> > +    } else {
> > +        hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> >       }
>
> Might want to assert e_shstrndx < SHN_LORESERVE (0xff00) or handle that case 
> correctly.

Right, more things to worry about

> >   }
> >   
> > @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp)
> >   {
> >       int ret;
> >   
> > -    /* Write section zero */
> > +    /* Write section zero and arch sections */
>
> Since that doesn't concern the string section it seems wrong to change this in 
> this patch.

I'm currently doing another round of cleanups anyway :)