Hi
On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <fran...@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 <fran...@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?
> + Elf32_Shdr shdr32;
> + Elf64_Shdr shdr64;
> + int shdr_size;
> + void *shdr = buff;
Why assign here?
> +
> + 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.
> + 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);
> }
> }
>
> @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error
> **errp)
> {
> int ret;
>
> - /* Write section zero */
> + /* Write section zero and arch sections */
> ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "dump: failed to write section data");
> }
> +
> + /* Write string table data */
> + ret = fd_write_vmcore(s->string_table_buf->data,
> + s->string_table_buf->len, s);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "dump: failed to write string table
> data");
> + }
> }
>
> static void write_data(DumpState *s, void *buf, int length, Error **errp)
> @@ -713,6 +772,7 @@ static void create_vmcore(DumpState *s, Error **errp)
> return;
> }
>
> + /* Iterate over memory and dump it to file */
> dump_iterate(s, errp);
> if (*errp) {
> return;
> @@ -1695,6 +1755,13 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> s->has_filter = has_filter;
> s->begin = begin;
> 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, ...
>
> memory_mapping_list_init(&s->list);
>
> @@ -1855,6 +1922,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
> + * 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;
> + }
> +
> tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num;
> if (dump_is_64bit(s)) {
> s->shdr_offset = sizeof(Elf64_Ehdr);
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 696e6c67d6..299b611180 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -178,6 +178,7 @@ typedef struct DumpState {
> void *elf_section_hdrs;
> uint64_t elf_section_data_size;
> void *elf_section_data;
> + GArray *string_table_buf; /* String table section */
>
> uint8_t *note_buf; /* buffer for notes */
> size_t note_buf_offset; /* the writing place in note_buf */
> --
> 2.34.1
>
