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

[Janosch Frank <frankja@linux.ibm.com>]

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 :)