Hi
On Thu, Aug 11, 2022 at 4:16 PM Janosch Frank <fran...@linux.ibm.com> wrote:
> Currently we're writing the NULL section header if we overflow the
> physical header number in the ELF header. But in the future we'll add
> custom section headers AND section data.
>
> To facilitate this we need to rearange section handling a bit. As with
> the other ELF headers we split the code into a prepare and a write
> step.
>
> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
>
---
> dump/dump.c | 83 +++++++++++++++++++++++++++++--------------
> include/sysemu/dump.h | 2 ++
> 2 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a905316fe5..0051c71d08 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error
> **errp)
> }
> }
>
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static void prepare_elf_section_hdr_zero(DumpState *s)
> {
> - Elf32_Shdr shdr32;
> - Elf64_Shdr shdr64;
> - int shdr_size;
> - void *shdr;
> + if (dump_is_64bit(s)) {
> + Elf64_Shdr *shdr64 = s->elf_section_hdrs;
> +
> + shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
> + } else {
> + Elf32_Shdr *shdr32 = s->elf_section_hdrs;
> +
> + shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
> + }
> +}
> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> + size_t len, sizeof_shdr;
> +
> + /*
> + * Section ordering:
> + * - HDR zero
> + */
> + 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);
> +
> + /*
> + * The first section header is ALWAYS a special initial section
> + * header.
> + *
> + * The header should be 0 with one exception being that if
> + * phdr_num is PN_XNUM then the sh_info field contains the real
> + * number of segment entries.
> + *
> + * As we zero allocate the buffer we will only need to modify
> + * sh_info for the PN_XNUM case.
> + */
> + if (s->phdr_num >= PN_XNUM) {
> + prepare_elf_section_hdr_zero(s);
> + }
> +}
> +
> +static void write_elf_section_headers(DumpState *s, Error **errp)
> +{
> + size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) :
> sizeof(Elf32_Shdr);
> int ret;
>
> - if (type == 0) {
> - shdr_size = sizeof(Elf32_Shdr);
> - memset(&shdr32, 0, shdr_size);
> - shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
> - shdr = &shdr32;
> - } else {
> - shdr_size = sizeof(Elf64_Shdr);
> - memset(&shdr64, 0, shdr_size);
> - shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> - shdr = &shdr64;
> - }
> + prepare_elf_section_hdrs(s);
>
> - ret = fd_write_vmcore(shdr, shdr_size, s);
> + ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr,
> s);
> if (ret < 0) {
> - error_setg_errno(errp, -ret,
> - "dump: failed to write section header table");
> + error_setg_errno(errp, -ret, "dump: failed to write section
> headers");
> }
> }
>
> @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
> return;
> }
>
> + /* write section headers to vmcore */
> + write_elf_section_headers(s, errp);
> + if (*errp) {
> + return;
> + }
>
Can you make that move a separate commit? And please explain why this is
valid, and also update the table in the comment too.
Otherwise, changes look good to me.
+
> /* write PT_NOTE to vmcore */
> write_elf_phdr_note(s, errp);
> if (*errp) {
> @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
> return;
> }
>
> - /* write section to vmcore */
> - if (s->shdr_num) {
> - write_elf_section(s, 1, errp);
> - if (*errp) {
> - return;
> - }
> - }
> -
> /* write notes to vmcore */
> write_elf_notes(s, errp);
> }
> @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
> return;
> }
>
> + /* Iterate over memory and dump it to file */
> dump_iterate(s, errp);
> + if (*errp) {
> + return;
> + }
> }
>
> static int write_start_flat_header(int fd)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b62513d87d..9995f65dc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -177,6 +177,8 @@ typedef struct DumpState {
> int64_t filter_area_begin; /* Start address of partial guest memory
> area */
> int64_t filter_area_length; /* Length of partial guest memory area */
>
> + void *elf_section_hdrs; /* Pointer to section header buffer */
> +
> uint8_t *note_buf; /* buffer for notes */
> size_t note_buf_offset; /* the writing place in note_buf */
> uint32_t nr_cpus; /* number of guest's cpu */
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
