Re: [PATCH v5 12/18] dump/dump: Add section string table support

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank 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.

Why? Is there any harm in always having a (possibly empty) string
table? Can't put garbage into sh_name then but that's not relevant.
Seems like it would make the code a bit simpler.

I would also expect the string data to be written in this patch,
instead you do that in the next.
With that and Steffen's .shstrtab addressed:
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Some minor suggestions below.

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/dump.h |  4 +++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 31eb20108c..0d6dbf453a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c

[...]

> @@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s)
>      }
>  }
>  
> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;

Could just = {} those and drop the memset below.

> +    int shdr_size;
> +    void *shdr;
> +
> +    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"));

Could put the ".shstrtab" in a char[] variable.

> +        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);
> +}

[...]

> @@ -1844,6 +1903,18 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          }
>      }
>  
> +    /*
> +     * calculate shdr_num and elf_section_data_size so we know the offsets and

What is the elf_section_data_size thing about?

> +     * sizes of all parts.
> +     *
> +     * If phdr_num overflowed we have at least one section header
> +     * More sections/hdrs can be added by the architectures
> +     */
> +    if (s->shdr_num > 1) {
> +        /* Reserve the string table */
> +        s->shdr_num += 1;
> +    }
> +
>      if (dump_is_64bit(s)) {
>          s->shdr_offset = sizeof(Elf64_Ehdr);
>          s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;

[...]