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.
*/
ok
[..]
>> 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 :)
I haven't tried, but I think it would be a better fit.