[PATCH 0/7] dump: Cleanup and consolidation

[PATCH 0/7] dump: Cleanup and consolidation

[Janosch Frank]

The dump/dump.c file has lots of duplicated code for handling 64 and
32 bit elf files. Additionally there are many instances where code can
be improved by adding a variable to avoid having to specify the same
calculation or check over and over.

This series is the cleanup step onto which my series that adds custom
section support and finally the series that introduces PV dump support
are based on.

Personal comments:
I'd be happy if someone looks at the error handling.

It's taken me quite a while to understand how the code works and I
expect that this patch might improve that but it won't fix every
issue. Going forward it might make sense to split kdump and elf dump
code into separate files and also cleanup the kdump code.

Janosch Frank (7):
  dump: Introduce shdr_num to decrease complexity
  dump: Remove the sh_info variable
  dump: Add more offset variables
  dump: Introduce dump_is_64bit() helper function
  dump: Consolidate phdr note writes
  dump: Cleanup dump_begin write functions
  dump: Consolidate elf note function

 dump/dump.c           | 256 +++++++++++++++++++-----------------------
 include/sysemu/dump.h |  15 ++-
 2 files changed, 125 insertions(+), 146 deletions(-)

-- 
2.32.0

[PATCH 1/7] dump: Introduce shdr_num to decrease complexity

[Janosch Frank]

Let's move from a boolean to a int variable which will later enable us
to store the number of sections that are in the dump file.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 43 ++++++++++++++++++-------------------------
 include/sysemu/dump.h |  2 +-
 2 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index a84d8b1598..6696d9819a 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error **errp)
     elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
     elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
-    if (s->have_section) {
+    if (s->shdr_num) {
         uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->sh_info;
 
         elf_header.e_shoff = cpu_to_dump64(s, shoff);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
-        elf_header.e_shnum = cpu_to_dump16(s, 1);
+        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
     }
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error **errp)
     elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
     elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
-    if (s->have_section) {
+    if (s->shdr_num) {
         uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->sh_info;
 
         elf_header.e_shoff = cpu_to_dump32(s, shoff);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
-        elf_header.e_shnum = cpu_to_dump16(s, 1);
+        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
     }
 
     ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
     uint32_t max_index;
     Error *local_err = NULL;
 
-    if (s->have_section) {
+    if (s->shdr_num) {
         max_index = s->sh_info;
     } else {
         max_index = s->phdr_num;
@@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
         }
 
         /* write section to vmcore */
-        if (s->have_section) {
+        if (s->shdr_num) {
             write_elf_section(s, 1, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
@@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
         }
 
         /* write section to vmcore */
-        if (s->have_section) {
+        if (s->shdr_num) {
             write_elf_section(s, 0, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
@@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool has_format,
      */
     s->phdr_num = 1; /* PT_NOTE */
     if (s->list.num < UINT16_MAX - 2) {
+        s->shdr_num = 0;
         s->phdr_num += s->list.num;
-        s->have_section = false;
     } else {
-        s->have_section = true;
+        /* sh_info of section 0 holds the real number of phdrs */
         s->phdr_num = PN_XNUM;
+        s->shdr_num = 1;
         s->sh_info = 1; /* PT_NOTE */
 
         /* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
@@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     if (s->dump_info.d_class == ELFCLASS64) {
-        if (s->have_section) {
-            s->memory_offset = sizeof(Elf64_Ehdr) +
-                               sizeof(Elf64_Phdr) * s->sh_info +
-                               sizeof(Elf64_Shdr) + s->note_size;
-        } else {
-            s->memory_offset = sizeof(Elf64_Ehdr) +
-                               sizeof(Elf64_Phdr) * s->phdr_num + s->note_size;
-        }
+        s->memory_offset = sizeof(Elf64_Ehdr) +
+                           sizeof(Elf64_Phdr) * s->sh_info +
+                           sizeof(Elf64_Shdr) * s->shdr_num +
+                           s->note_size;
     } else {
-        if (s->have_section) {
-            s->memory_offset = sizeof(Elf32_Ehdr) +
-                               sizeof(Elf32_Phdr) * s->sh_info +
-                               sizeof(Elf32_Shdr) + s->note_size;
-        } else {
-            s->memory_offset = sizeof(Elf32_Ehdr) +
-                               sizeof(Elf32_Phdr) * s->phdr_num + s->note_size;
-        }
+        s->memory_offset = sizeof(Elf32_Ehdr) +
+                           sizeof(Elf32_Phdr) * s->sh_info +
+                           sizeof(Elf32_Shdr) * s->shdr_num +
+                           s->note_size;
     }
 
     return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 250143cb5a..854341da0d 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -155,8 +155,8 @@ typedef struct DumpState {
     ArchDumpInfo dump_info;
     MemoryMappingList list;
     uint16_t phdr_num;
+    uint32_t shdr_num;
     uint32_t sh_info;
-    bool have_section;
     bool resume;
     bool detached;
     ssize_t note_size;
-- 
2.32.0

[PATCH 2/7] dump: Remove the sh_info variable

[Janosch Frank]

There's no need to have phdr_num and sh_info at the same time. We can
make phdr_num 32 bit and set PN_XNUM when we write the header if
phdr_num >= PN_XNUM.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 33 +++++++++++++--------------------
 include/sysemu/dump.h |  3 +--
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 6696d9819a..ce3a5e7003 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -124,6 +124,7 @@ static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
 
 static void write_elf64_header(DumpState *s, Error **errp)
 {
+    uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
     Elf64_Ehdr elf_header;
     int ret;
 
@@ -138,9 +139,9 @@ static void write_elf64_header(DumpState *s, Error **errp)
     elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
     elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
-    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
+    elf_header.e_phnum = cpu_to_dump16(s, phnum);
     if (s->shdr_num) {
-        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->sh_info;
+        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
 
         elf_header.e_shoff = cpu_to_dump64(s, shoff);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
@@ -155,6 +156,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
 
 static void write_elf32_header(DumpState *s, Error **errp)
 {
+    uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
     Elf32_Ehdr elf_header;
     int ret;
 
@@ -169,9 +171,9 @@ static void write_elf32_header(DumpState *s, Error **errp)
     elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
     elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
-    elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
+    elf_header.e_phnum = cpu_to_dump16(s, phnum);
     if (s->shdr_num) {
-        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->sh_info;
+        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
 
         elf_header.e_shoff = cpu_to_dump32(s, shoff);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
@@ -358,12 +360,12 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
     if (type == 0) {
         shdr_size = sizeof(Elf32_Shdr);
         memset(&shdr32, 0, shdr_size);
-        shdr32.sh_info = cpu_to_dump32(s, s->sh_info);
+        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->sh_info);
+        shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
         shdr = &shdr64;
     }
 
@@ -479,15 +481,8 @@ static void write_elf_loads(DumpState *s, Error **errp)
     hwaddr offset, filesz;
     MemoryMapping *memory_mapping;
     uint32_t phdr_index = 1;
-    uint32_t max_index;
     Error *local_err = NULL;
 
-    if (s->shdr_num) {
-        max_index = s->sh_info;
-    } else {
-        max_index = s->phdr_num;
-    }
-
     QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
         get_offset_range(memory_mapping->phys_addr,
                          memory_mapping->length,
@@ -505,7 +500,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
             return;
         }
 
-        if (phdr_index >= max_index) {
+        if (phdr_index >= s->phdr_num) {
             break;
         }
     }
@@ -1822,26 +1817,24 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         s->phdr_num += s->list.num;
     } else {
         /* sh_info of section 0 holds the real number of phdrs */
-        s->phdr_num = PN_XNUM;
         s->shdr_num = 1;
-        s->sh_info = 1; /* PT_NOTE */
 
         /* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
         if (s->list.num <= UINT32_MAX - 1) {
-            s->sh_info += s->list.num;
+            s->phdr_num += s->list.num;
         } else {
-            s->sh_info = UINT32_MAX;
+            s->phdr_num = UINT32_MAX;
         }
     }
 
     if (s->dump_info.d_class == ELFCLASS64) {
         s->memory_offset = sizeof(Elf64_Ehdr) +
-                           sizeof(Elf64_Phdr) * s->sh_info +
+                           sizeof(Elf64_Phdr) * s->phdr_num +
                            sizeof(Elf64_Shdr) * s->shdr_num +
                            s->note_size;
     } else {
         s->memory_offset = sizeof(Elf32_Ehdr) +
-                           sizeof(Elf32_Phdr) * s->sh_info +
+                           sizeof(Elf32_Phdr) * s->phdr_num +
                            sizeof(Elf32_Shdr) * s->shdr_num +
                            s->note_size;
     }
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 854341da0d..19458bffbd 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -154,9 +154,8 @@ typedef struct DumpState {
     GuestPhysBlockList guest_phys_blocks;
     ArchDumpInfo dump_info;
     MemoryMappingList list;
-    uint16_t phdr_num;
+    uint32_t phdr_num;
     uint32_t shdr_num;
-    uint32_t sh_info;
     bool resume;
     bool detached;
     ssize_t note_size;
-- 
2.32.0

[PATCH 3/7] dump: Add more offset variables

[Janosch Frank]

Offset calculations are easy enough to get wrong. Let's add a few
variables to make moving around elf headers and data sections easier.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 34 ++++++++++++++--------------------
 include/sysemu/dump.h |  4 ++++
 2 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index ce3a5e7003..242f83db95 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -137,13 +137,11 @@ static void write_elf64_header(DumpState *s, Error **errp)
     elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
     elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
     elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
-    elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
+    elf_header.e_phoff = cpu_to_dump64(s, s->phdr_offset);
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
     elf_header.e_phnum = cpu_to_dump16(s, phnum);
     if (s->shdr_num) {
-        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
-
-        elf_header.e_shoff = cpu_to_dump64(s, shoff);
+        elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
         elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
     }
@@ -169,13 +167,11 @@ static void write_elf32_header(DumpState *s, Error **errp)
     elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
     elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
     elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
-    elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
+    elf_header.e_phoff = cpu_to_dump32(s, s->phdr_offset);
     elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
     elf_header.e_phnum = cpu_to_dump16(s, phnum);
     if (s->shdr_num) {
-        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
-
-        elf_header.e_shoff = cpu_to_dump32(s, shoff);
+        elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
         elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
         elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
     }
@@ -238,12 +234,11 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
 static void write_elf64_note(DumpState *s, Error **errp)
 {
     Elf64_Phdr phdr;
-    hwaddr begin = s->memory_offset - s->note_size;
     int ret;
 
     memset(&phdr, 0, sizeof(Elf64_Phdr));
     phdr.p_type = cpu_to_dump32(s, PT_NOTE);
-    phdr.p_offset = cpu_to_dump64(s, begin);
+    phdr.p_offset = cpu_to_dump64(s, s->note_offset);
     phdr.p_paddr = 0;
     phdr.p_filesz = cpu_to_dump64(s, s->note_size);
     phdr.p_memsz = cpu_to_dump64(s, s->note_size);
@@ -303,13 +298,12 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
 
 static void write_elf32_note(DumpState *s, Error **errp)
 {
-    hwaddr begin = s->memory_offset - s->note_size;
     Elf32_Phdr phdr;
     int ret;
 
     memset(&phdr, 0, sizeof(Elf32_Phdr));
     phdr.p_type = cpu_to_dump32(s, PT_NOTE);
-    phdr.p_offset = cpu_to_dump32(s, begin);
+    phdr.p_offset = cpu_to_dump32(s, s->note_offset);
     phdr.p_paddr = 0;
     phdr.p_filesz = cpu_to_dump32(s, s->note_size);
     phdr.p_memsz = cpu_to_dump32(s, s->note_size);
@@ -1828,15 +1822,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     if (s->dump_info.d_class == ELFCLASS64) {
-        s->memory_offset = sizeof(Elf64_Ehdr) +
-                           sizeof(Elf64_Phdr) * s->phdr_num +
-                           sizeof(Elf64_Shdr) * s->shdr_num +
-                           s->note_size;
+        s->phdr_offset = sizeof(Elf64_Ehdr);
+        s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
+        s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
+        s->memory_offset = s->note_offset + s->note_size;
     } else {
-        s->memory_offset = sizeof(Elf32_Ehdr) +
-                           sizeof(Elf32_Phdr) * s->phdr_num +
-                           sizeof(Elf32_Shdr) * s->shdr_num +
-                           s->note_size;
+        s->phdr_offset = sizeof(Elf32_Ehdr);
+        s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
+        s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
+        s->memory_offset = s->note_offset + s->note_size;
     }
 
     return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 19458bffbd..ffc2ea1072 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -159,6 +159,10 @@ typedef struct DumpState {
     bool resume;
     bool detached;
     ssize_t note_size;
+    hwaddr shdr_offset;
+    hwaddr phdr_offset;
+    hwaddr section_offset;
+    hwaddr note_offset;
     hwaddr memory_offset;
     int fd;
 
-- 
2.32.0

[PATCH 4/7] dump: Introduce dump_is_64bit() helper function

[Janosch Frank]

Checking d_class in dump_info leads to lengthy conditionals so let's
shorten things a bit by introducing a helper function.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c           | 14 +++++++-------
 include/sysemu/dump.h |  6 ++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 242f83db95..bb152bddff 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -481,7 +481,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
         get_offset_range(memory_mapping->phys_addr,
                          memory_mapping->length,
                          s, &offset, &filesz);
-        if (s->dump_info.d_class == ELFCLASS64) {
+        if (dump_is_64bit(s)) {
             write_elf64_load(s, memory_mapping, phdr_index++, offset,
                              filesz, &local_err);
         } else {
@@ -530,7 +530,7 @@ static void dump_begin(DumpState *s, Error **errp)
      */
 
     /* write elf header to vmcore */
-    if (s->dump_info.d_class == ELFCLASS64) {
+    if (dump_is_64bit(s)) {
         write_elf64_header(s, &local_err);
     } else {
         write_elf32_header(s, &local_err);
@@ -540,7 +540,7 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
-    if (s->dump_info.d_class == ELFCLASS64) {
+    if (dump_is_64bit(s)) {
         /* write PT_NOTE to vmcore */
         write_elf64_note(s, &local_err);
         if (local_err) {
@@ -761,7 +761,7 @@ static void get_note_sizes(DumpState *s, const void *note,
     uint64_t name_sz;
     uint64_t desc_sz;
 
-    if (s->dump_info.d_class == ELFCLASS64) {
+    if (dump_is_64bit(s)) {
         const Elf64_Nhdr *hdr = note;
         note_head_sz = sizeof(Elf64_Nhdr);
         name_sz = tswap64(hdr->n_namesz);
@@ -1023,7 +1023,7 @@ out:
 
 static void write_dump_header(DumpState *s, Error **errp)
 {
-    if (s->dump_info.d_class == ELFCLASS32) {
+    if (!dump_is_64bit(s)) {
         create_header32(s, errp);
     } else {
         create_header64(s, errp);
@@ -1716,7 +1716,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         uint32_t size;
         uint16_t format;
 
-        note_head_size = s->dump_info.d_class == ELFCLASS32 ?
+        note_head_size = !dump_is_64bit(s) ?
             sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
 
         format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
@@ -1821,7 +1821,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         }
     }
 
-    if (s->dump_info.d_class == ELFCLASS64) {
+    if (dump_is_64bit(s)) {
         s->phdr_offset = sizeof(Elf64_Ehdr);
         s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
         s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index ffc2ea1072..078b3d57a1 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -203,4 +203,10 @@ typedef struct DumpState {
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
 uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
 uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
+
+static inline bool dump_is_64bit(DumpState *s)
+{
+    return s->dump_info.d_class == ELFCLASS64;
+}
+
 #endif
-- 
2.32.0

[PATCH 5/7] dump: Consolidate phdr note writes

[Janosch Frank]

There's no need to have two write functions. Let's rather have two
functions that set the data for elf 32/64 and then write it in a
common function.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index bb152bddff..88c2dbb466 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
     }
 }
 
-static void write_elf64_note(DumpState *s, Error **errp)
+static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
 {
-    Elf64_Phdr phdr;
-    int ret;
-
-    memset(&phdr, 0, sizeof(Elf64_Phdr));
-    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
-    phdr.p_offset = cpu_to_dump64(s, s->note_offset);
-    phdr.p_paddr = 0;
-    phdr.p_filesz = cpu_to_dump64(s, s->note_size);
-    phdr.p_memsz = cpu_to_dump64(s, s->note_size);
-    phdr.p_vaddr = 0;
-
-    ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret,
-                         "dump: failed to write program header table");
-    }
+    memset(phdr, 0, sizeof(*phdr));
+    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
+    phdr->p_offset = cpu_to_dump64(s, s->note_offset);
+    phdr->p_paddr = 0;
+    phdr->p_filesz = cpu_to_dump64(s, s->note_size);
+    phdr->p_memsz = cpu_to_dump64(s, s->note_size);
+    phdr->p_vaddr = 0;
 }
 
 static inline int cpu_index(CPUState *cpu)
@@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
     write_guest_note(f, s, errp);
 }
 
-static void write_elf32_note(DumpState *s, Error **errp)
+static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
 {
-    Elf32_Phdr phdr;
-    int ret;
-
-    memset(&phdr, 0, sizeof(Elf32_Phdr));
-    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
-    phdr.p_offset = cpu_to_dump32(s, s->note_offset);
-    phdr.p_paddr = 0;
-    phdr.p_filesz = cpu_to_dump32(s, s->note_size);
-    phdr.p_memsz = cpu_to_dump32(s, s->note_size);
-    phdr.p_vaddr = 0;
-
-    ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret,
-                         "dump: failed to write program header table");
-    }
+    memset(phdr, 0, sizeof(*phdr));
+    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
+    phdr->p_offset = cpu_to_dump32(s, s->note_offset);
+    phdr->p_paddr = 0;
+    phdr->p_filesz = cpu_to_dump32(s, s->note_size);
+    phdr->p_memsz = cpu_to_dump32(s, s->note_size);
+    phdr->p_vaddr = 0;
 }
 
 static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
@@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
     write_guest_note(f, s, errp);
 }
 
+static void write_elf_phdr_note(DumpState *s, Error **errp)
+{
+    Elf32_Phdr phdr32;
+    Elf64_Phdr phdr64;
+    void *phdr;
+    size_t size;
+    int ret;
+
+    if (dump_is_64bit(s)) {
+        write_elf64_phdr_note(s, &phdr64);
+        size = sizeof(phdr64);
+        phdr = &phdr64;
+    } else {
+        write_elf32_phdr_note(s, &phdr32);
+        size = sizeof(phdr32);
+        phdr = &phdr32;
+    }
+
+    ret = fd_write_vmcore(phdr, size, s);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "dump: failed to write program header table");
+    }
+}
+
 static void write_elf_section(DumpState *s, int type, Error **errp)
 {
     Elf32_Shdr shdr32;
@@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
-    if (dump_is_64bit(s)) {
-        /* write PT_NOTE to vmcore */
-        write_elf64_note(s, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
+    /* write PT_NOTE to vmcore */
+    write_elf_phdr_note(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
+    if (dump_is_64bit(s)) {
         /* write all PT_LOAD to vmcore */
         write_elf_loads(s, &local_err);
         if (local_err) {
@@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
             return;
         }
     } else {
-        /* write PT_NOTE to vmcore */
-        write_elf32_note(s, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-
         /* write all PT_LOAD to vmcore */
         write_elf_loads(s, &local_err);
         if (local_err) {
-- 
2.32.0

[PATCH 6/7] dump: Cleanup dump_begin write functions

[Janosch Frank]

There's no need to have a gigantic if in there let's move the elf
32/64 bit logic into the section, segment or note code.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 52 ++++++++++++++++------------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 88c2dbb466..78654b9c27 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -554,52 +554,32 @@ static void dump_begin(DumpState *s, Error **errp)
         return;
     }
 
-    if (dump_is_64bit(s)) {
-        /* write all PT_LOAD to vmcore */
-        write_elf_loads(s, &local_err);
+    /* write all PT_LOAD to vmcore */
+    write_elf_loads(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    /* write section to vmcore */
+    if (s->shdr_num) {
+        write_elf_section(s, 1, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
+    }
 
-        /* write section to vmcore */
-        if (s->shdr_num) {
-            write_elf_section(s, 1, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                return;
-            }
-        }
-
+    if (dump_is_64bit(s)) {
         /* write notes to vmcore */
         write_elf64_notes(fd_write_vmcore, s, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
     } else {
-        /* write all PT_LOAD to vmcore */
-        write_elf_loads(s, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-
-        /* write section to vmcore */
-        if (s->shdr_num) {
-            write_elf_section(s, 0, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                return;
-            }
-        }
-
         /* write notes to vmcore */
         write_elf32_notes(fd_write_vmcore, s, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
     }
 }
 
-- 
2.32.0

[PATCH 7/7] dump: Consolidate elf note function

[Janosch Frank]

Just like with the other write functions let's move the 32/64 bit elf
handling to a function to improve readability.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 78654b9c27..9ba0392e00 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
     }
 }
 
+static void write_elf_notes(DumpState *s, Error **errp)
+{
+    Error *local_err = NULL;
+
+    if (dump_is_64bit(s)) {
+        write_elf64_notes(fd_write_vmcore, s, &local_err);
+    } else {
+        write_elf32_notes(fd_write_vmcore, s, &local_err);
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 /* write elf header, PT_NOTE and elf note to vmcore. */
 static void dump_begin(DumpState *s, Error **errp)
 {
@@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
         }
     }
 
-    if (dump_is_64bit(s)) {
-        /* write notes to vmcore */
-        write_elf64_notes(fd_write_vmcore, s, &local_err);
-    } else {
-        /* write notes to vmcore */
-        write_elf32_notes(fd_write_vmcore, s, &local_err);
-    }
+    /* write notes to vmcore */
+    write_elf_notes(s, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
-- 
2.32.0

Re: [PATCH 5/7] dump: Consolidate phdr note writes

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 5283 bytes --]

Hi

On Tue, Mar 1, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> There's no need to have two write functions. Let's rather have two
> functions that set the data for elf 32/64 and then write it in a
> common function.
>
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------

 1 file changed, 48 insertions(+), 48 deletions(-)
>

I suppose there are other parts of the series that make use of that,
because as such, it's not a clear improvement to me.

>

>
> diff --git a/dump/dump.c b/dump/dump.c
> index bb152bddff..88c2dbb466 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s,
> MemoryMapping *memory_mapping,
>      }
>  }
>
> -static void write_elf64_note(DumpState *s, Error **errp)
> +static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
>  {
> -    Elf64_Phdr phdr;
> -    int ret;
> -
> -    memset(&phdr, 0, sizeof(Elf64_Phdr));
> -    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> -    phdr.p_offset = cpu_to_dump64(s, s->note_offset);
> -    phdr.p_paddr = 0;
> -    phdr.p_filesz = cpu_to_dump64(s, s->note_size);
> -    phdr.p_memsz = cpu_to_dump64(s, s->note_size);
> -    phdr.p_vaddr = 0;
> -
> -    ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret,
> -                         "dump: failed to write program header table");
> -    }
> +    memset(phdr, 0, sizeof(*phdr));
> +    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> +    phdr->p_offset = cpu_to_dump64(s, s->note_offset);
> +    phdr->p_paddr = 0;
> +    phdr->p_filesz = cpu_to_dump64(s, s->note_size);
> +    phdr->p_memsz = cpu_to_dump64(s, s->note_size);
> +    phdr->p_vaddr = 0;
>  }
>
>  static inline int cpu_index(CPUState *cpu)
> @@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction
> f, DumpState *s,
>      write_guest_note(f, s, errp);
>  }
>
> -static void write_elf32_note(DumpState *s, Error **errp)
> +static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
>  {
> -    Elf32_Phdr phdr;
> -    int ret;
> -
> -    memset(&phdr, 0, sizeof(Elf32_Phdr));
> -    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> -    phdr.p_offset = cpu_to_dump32(s, s->note_offset);
> -    phdr.p_paddr = 0;
> -    phdr.p_filesz = cpu_to_dump32(s, s->note_size);
> -    phdr.p_memsz = cpu_to_dump32(s, s->note_size);
> -    phdr.p_vaddr = 0;
> -
> -    ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret,
> -                         "dump: failed to write program header table");
> -    }
> +    memset(phdr, 0, sizeof(*phdr));
> +    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> +    phdr->p_offset = cpu_to_dump32(s, s->note_offset);
> +    phdr->p_paddr = 0;
> +    phdr->p_filesz = cpu_to_dump32(s, s->note_size);
> +    phdr->p_memsz = cpu_to_dump32(s, s->note_size);
> +    phdr->p_vaddr = 0;
>  }
>
>  static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
> @@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction
> f, DumpState *s,
>      write_guest_note(f, s, errp);
>  }
>
> +static void write_elf_phdr_note(DumpState *s, Error **errp)
> +{
> +    Elf32_Phdr phdr32;
> +    Elf64_Phdr phdr64;
> +    void *phdr;
> +    size_t size;
> +    int ret;
> +
> +    if (dump_is_64bit(s)) {
> +        write_elf64_phdr_note(s, &phdr64);
> +        size = sizeof(phdr64);
> +        phdr = &phdr64;
> +    } else {
> +        write_elf32_phdr_note(s, &phdr32);
> +        size = sizeof(phdr32);
> +        phdr = &phdr32;
> +    }
> +
> +    ret = fd_write_vmcore(phdr, size, s);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "dump: failed to write program header table");
> +    }
> +}
> +
>  static void write_elf_section(DumpState *s, int type, Error **errp)
>  {
>      Elf32_Shdr shdr32;
> @@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>
> -    if (dump_is_64bit(s)) {
> -        /* write PT_NOTE to vmcore */
> -        write_elf64_note(s, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> +    /* write PT_NOTE to vmcore */
> +    write_elf_phdr_note(s, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>
> +    if (dump_is_64bit(s)) {
>          /* write all PT_LOAD to vmcore */
>          write_elf_loads(s, &local_err);
>          if (local_err) {
> @@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
>              return;
>          }
>      } else {
> -        /* write PT_NOTE to vmcore */
> -        write_elf32_note(s, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> -
>          /* write all PT_LOAD to vmcore */
>          write_elf_loads(s, &local_err);
>          if (local_err) {
> --
> 2.32.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 7081 bytes --]

Re: [PATCH 4/7] dump: Introduce dump_is_64bit() helper function

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

Hi

On Tue, Mar 1, 2022 at 6:30 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> Checking d_class in dump_info leads to lengthy conditionals so let's
> shorten things a bit by introducing a helper function.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 14 +++++++-------
>  include/sysemu/dump.h |  6 ++++++
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 242f83db95..bb152bddff 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -481,7 +481,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
>          get_offset_range(memory_mapping->phys_addr,
>                           memory_mapping->length,
>                           s, &offset, &filesz);
> -        if (s->dump_info.d_class == ELFCLASS64) {
> +        if (dump_is_64bit(s)) {
>              write_elf64_load(s, memory_mapping, phdr_index++, offset,
>                               filesz, &local_err);
>          } else {
> @@ -530,7 +530,7 @@ static void dump_begin(DumpState *s, Error **errp)
>       */
>
>      /* write elf header to vmcore */
> -    if (s->dump_info.d_class == ELFCLASS64) {
> +    if (dump_is_64bit(s)) {
>          write_elf64_header(s, &local_err);
>      } else {
>          write_elf32_header(s, &local_err);
> @@ -540,7 +540,7 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>
> -    if (s->dump_info.d_class == ELFCLASS64) {
> +    if (dump_is_64bit(s)) {
>          /* write PT_NOTE to vmcore */
>          write_elf64_note(s, &local_err);
>          if (local_err) {
> @@ -761,7 +761,7 @@ static void get_note_sizes(DumpState *s, const void
> *note,
>      uint64_t name_sz;
>      uint64_t desc_sz;
>
> -    if (s->dump_info.d_class == ELFCLASS64) {
> +    if (dump_is_64bit(s)) {
>          const Elf64_Nhdr *hdr = note;
>          note_head_sz = sizeof(Elf64_Nhdr);
>          name_sz = tswap64(hdr->n_namesz);
> @@ -1023,7 +1023,7 @@ out:
>
>  static void write_dump_header(DumpState *s, Error **errp)
>  {
> -    if (s->dump_info.d_class == ELFCLASS32) {
> +    if (!dump_is_64bit(s)) {
>          create_header32(s, errp);
>      } else {
>          create_header64(s, errp);
> @@ -1716,7 +1716,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>          uint32_t size;
>          uint16_t format;
>
> -        note_head_size = s->dump_info.d_class == ELFCLASS32 ?
> +        note_head_size = !dump_is_64bit(s) ?
>              sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
>
>          format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> @@ -1821,7 +1821,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>          }
>      }
>
> -    if (s->dump_info.d_class == ELFCLASS64) {
> +    if (dump_is_64bit(s)) {
>          s->phdr_offset = sizeof(Elf64_Ehdr);
>          s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) *
> s->phdr_num;
>          s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) *
> s->shdr_num;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..078b3d57a1 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -203,4 +203,10 @@ typedef struct DumpState {
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
>  uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
>  uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
> +
> +static inline bool dump_is_64bit(DumpState *s)
> +{
> +    return s->dump_info.d_class == ELFCLASS64;
> +}
>

Since it's not used outside of dump.c (so far), I'd keep this as a regular
static function there. Otherwise, lgtm.


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4683 bytes --]

Re: [PATCH 1/7] dump: Introduce shdr_num to decrease complexity

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 5823 bytes --]

Hi

On Tue, Mar 1, 2022 at 6:33 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's move from a boolean to a int variable which will later enable us
> to store the number of sections that are in the dump file.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 43 ++++++++++++++++++-------------------------
>  include/sysemu/dump.h |  2 +-
>  2 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a84d8b1598..6696d9819a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error
> **errp)
>      elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
>      elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
>      elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> -    if (s->have_section) {
> +    if (s->shdr_num) {
>          uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> s->sh_info;
>
>          elf_header.e_shoff = cpu_to_dump64(s, shoff);
>          elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> -        elf_header.e_shnum = cpu_to_dump16(s, 1);
> +        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>      }
>
>      ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> @@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error
> **errp)
>      elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
>      elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
>      elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> -    if (s->have_section) {
> +    if (s->shdr_num) {
>          uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> s->sh_info;
>
>          elf_header.e_shoff = cpu_to_dump32(s, shoff);
>          elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> -        elf_header.e_shnum = cpu_to_dump16(s, 1);
> +        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>      }
>
>      ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> @@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
>      uint32_t max_index;
>      Error *local_err = NULL;
>
> -    if (s->have_section) {
> +    if (s->shdr_num) {
>          max_index = s->sh_info;
>      } else {
>          max_index = s->phdr_num;
> @@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
>          }
>
>          /* write section to vmcore */
> -        if (s->have_section) {
> +        if (s->shdr_num) {
>              write_elf_section(s, 1, &local_err);
>              if (local_err) {
>                  error_propagate(errp, local_err);
> @@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
>          }
>
>          /* write section to vmcore */
> -        if (s->have_section) {
> +        if (s->shdr_num) {
>              write_elf_section(s, 0, &local_err);
>              if (local_err) {
>                  error_propagate(errp, local_err);
> @@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>       */
>      s->phdr_num = 1; /* PT_NOTE */
>      if (s->list.num < UINT16_MAX - 2) {
> +        s->shdr_num = 0;
>          s->phdr_num += s->list.num;
> -        s->have_section = false;
>      } else {
> -        s->have_section = true;
> +        /* sh_info of section 0 holds the real number of phdrs */
>

...


>          s->phdr_num = PN_XNUM;
> +        s->shdr_num = 1;
>          s->sh_info = 1; /* PT_NOTE */
>
>          /* the type of shdr->sh_info is uint32_t, so we should avoid
> overflow */
> @@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
>      }
>
>      if (s->dump_info.d_class == ELFCLASS64) {
> -        if (s->have_section) {
> -            s->memory_offset = sizeof(Elf64_Ehdr) +
> -                               sizeof(Elf64_Phdr) * s->sh_info +
> -                               sizeof(Elf64_Shdr) + s->note_size;
> -        } else {
> -            s->memory_offset = sizeof(Elf64_Ehdr) +
> -                               sizeof(Elf64_Phdr) * s->phdr_num +
> s->note_size;
> -        }
> +        s->memory_offset = sizeof(Elf64_Ehdr) +
> +                           sizeof(Elf64_Phdr) * s->sh_info +
>

The change "removing/replacing sizeof(Elf64_Phdr) * s->phdr_num by *
s->sh_info" should be done as a preliminary step, with more comments.

+                           sizeof(Elf64_Shdr) * s->shdr_num +
> +                           s->note_size;
>      } else {
> -        if (s->have_section) {
> -            s->memory_offset = sizeof(Elf32_Ehdr) +
> -                               sizeof(Elf32_Phdr) * s->sh_info +
> -                               sizeof(Elf32_Shdr) + s->note_size;
> -        } else {
> -            s->memory_offset = sizeof(Elf32_Ehdr) +
> -                               sizeof(Elf32_Phdr) * s->phdr_num +
> s->note_size;
> -        }
> +        s->memory_offset = sizeof(Elf32_Ehdr) +
> +                           sizeof(Elf32_Phdr) * s->sh_info +
> +                           sizeof(Elf32_Shdr) * s->shdr_num +
> +                           s->note_size;
>      }
>
>      return;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 250143cb5a..854341da0d 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -155,8 +155,8 @@ typedef struct DumpState {
>      ArchDumpInfo dump_info;
>      MemoryMappingList list;
>      uint16_t phdr_num;
> +    uint32_t shdr_num;
>      uint32_t sh_info;
> -    bool have_section;
>      bool resume;
>      bool detached;
>      ssize_t note_size;
> --
> 2.32.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 7571 bytes --]

Re: [PATCH 5/7] dump: Consolidate phdr note writes

[Janosch Frank]

On 3/1/22 15:30, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 1, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> There's no need to have two write functions. Let's rather have two
>> functions that set the data for elf 32/64 and then write it in a
>> common function.
>>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------
> 
>   1 file changed, 48 insertions(+), 48 deletions(-)
>>
> 
> I suppose there are other parts of the series that make use of that,
> because as such, it's not a clear improvement to me.

It's one piece of the effort to get rid of that big if/else in 
dump_begin() and that frees the way to make re-ordering the write 
functions possible.

I'll send out the other two series tomorrow.

> 
>>
> 
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index bb152bddff..88c2dbb466 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s,
>> MemoryMapping *memory_mapping,
>>       }
>>   }
>>
>> -static void write_elf64_note(DumpState *s, Error **errp)
>> +static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
>>   {
>> -    Elf64_Phdr phdr;
>> -    int ret;
>> -
>> -    memset(&phdr, 0, sizeof(Elf64_Phdr));
>> -    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
>> -    phdr.p_offset = cpu_to_dump64(s, s->note_offset);
>> -    phdr.p_paddr = 0;
>> -    phdr.p_filesz = cpu_to_dump64(s, s->note_size);
>> -    phdr.p_memsz = cpu_to_dump64(s, s->note_size);
>> -    phdr.p_vaddr = 0;
>> -
>> -    ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
>> -    if (ret < 0) {
>> -        error_setg_errno(errp, -ret,
>> -                         "dump: failed to write program header table");
>> -    }
>> +    memset(phdr, 0, sizeof(*phdr));
>> +    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
>> +    phdr->p_offset = cpu_to_dump64(s, s->note_offset);
>> +    phdr->p_paddr = 0;
>> +    phdr->p_filesz = cpu_to_dump64(s, s->note_size);
>> +    phdr->p_memsz = cpu_to_dump64(s, s->note_size);
>> +    phdr->p_vaddr = 0;
>>   }
>>
>>   static inline int cpu_index(CPUState *cpu)
>> @@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction
>> f, DumpState *s,
>>       write_guest_note(f, s, errp);
>>   }
>>
>> -static void write_elf32_note(DumpState *s, Error **errp)
>> +static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
>>   {
>> -    Elf32_Phdr phdr;
>> -    int ret;
>> -
>> -    memset(&phdr, 0, sizeof(Elf32_Phdr));
>> -    phdr.p_type = cpu_to_dump32(s, PT_NOTE);
>> -    phdr.p_offset = cpu_to_dump32(s, s->note_offset);
>> -    phdr.p_paddr = 0;
>> -    phdr.p_filesz = cpu_to_dump32(s, s->note_size);
>> -    phdr.p_memsz = cpu_to_dump32(s, s->note_size);
>> -    phdr.p_vaddr = 0;
>> -
>> -    ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
>> -    if (ret < 0) {
>> -        error_setg_errno(errp, -ret,
>> -                         "dump: failed to write program header table");
>> -    }
>> +    memset(phdr, 0, sizeof(*phdr));
>> +    phdr->p_type = cpu_to_dump32(s, PT_NOTE);
>> +    phdr->p_offset = cpu_to_dump32(s, s->note_offset);
>> +    phdr->p_paddr = 0;
>> +    phdr->p_filesz = cpu_to_dump32(s, s->note_size);
>> +    phdr->p_memsz = cpu_to_dump32(s, s->note_size);
>> +    phdr->p_vaddr = 0;
>>   }
>>
>>   static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
>> @@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction
>> f, DumpState *s,
>>       write_guest_note(f, s, errp);
>>   }
>>
>> +static void write_elf_phdr_note(DumpState *s, Error **errp)
>> +{
>> +    Elf32_Phdr phdr32;
>> +    Elf64_Phdr phdr64;
>> +    void *phdr;
>> +    size_t size;
>> +    int ret;
>> +
>> +    if (dump_is_64bit(s)) {
>> +        write_elf64_phdr_note(s, &phdr64);
>> +        size = sizeof(phdr64);
>> +        phdr = &phdr64;
>> +    } else {
>> +        write_elf32_phdr_note(s, &phdr32);
>> +        size = sizeof(phdr32);
>> +        phdr = &phdr32;
>> +    }
>> +
>> +    ret = fd_write_vmcore(phdr, size, s);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret,
>> +                         "dump: failed to write program header table");
>> +    }
>> +}
>> +
>>   static void write_elf_section(DumpState *s, int type, Error **errp)
>>   {
>>       Elf32_Shdr shdr32;
>> @@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
>>           return;
>>       }
>>
>> -    if (dump_is_64bit(s)) {
>> -        /* write PT_NOTE to vmcore */
>> -        write_elf64_note(s, &local_err);
>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            return;
>> -        }
>> +    /* write PT_NOTE to vmcore */
>> +    write_elf_phdr_note(s, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>
>> +    if (dump_is_64bit(s)) {
>>           /* write all PT_LOAD to vmcore */
>>           write_elf_loads(s, &local_err);
>>           if (local_err) {
>> @@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
>>               return;
>>           }
>>       } else {
>> -        /* write PT_NOTE to vmcore */
>> -        write_elf32_note(s, &local_err);
>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            return;
>> -        }
>> -
>>           /* write all PT_LOAD to vmcore */
>>           write_elf_loads(s, &local_err);
>>           if (local_err) {
>> --
>> 2.32.0
>>
>>
>>
> 

Re: [PATCH 3/7] dump: Add more offset variables

[Marc-André Lureau]

On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Offset calculations are easy enough to get wrong. Let's add a few
> variables to make moving around elf headers and data sections easier.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c           | 34 ++++++++++++++--------------------
>  include/sysemu/dump.h |  4 ++++
>  2 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index ce3a5e7003..242f83db95 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -137,13 +137,11 @@ static void write_elf64_header(DumpState *s, Error **errp)
>      elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
>      elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
>      elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> -    elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
> +    elf_header.e_phoff = cpu_to_dump64(s, s->phdr_offset);
>      elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
>      elf_header.e_phnum = cpu_to_dump16(s, phnum);
>      if (s->shdr_num) {
> -        uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
> -
> -        elf_header.e_shoff = cpu_to_dump64(s, shoff);
> +        elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
>          elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
>          elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>      }
> @@ -169,13 +167,11 @@ static void write_elf32_header(DumpState *s, Error **errp)
>      elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
>      elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
>      elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> -    elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
> +    elf_header.e_phoff = cpu_to_dump32(s, s->phdr_offset);
>      elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
>      elf_header.e_phnum = cpu_to_dump16(s, phnum);
>      if (s->shdr_num) {
> -        uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
> -
> -        elf_header.e_shoff = cpu_to_dump32(s, shoff);
> +        elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
>          elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
>          elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>      }
> @@ -238,12 +234,11 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
>  static void write_elf64_note(DumpState *s, Error **errp)
>  {
>      Elf64_Phdr phdr;
> -    hwaddr begin = s->memory_offset - s->note_size;
>      int ret;
>
>      memset(&phdr, 0, sizeof(Elf64_Phdr));
>      phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> -    phdr.p_offset = cpu_to_dump64(s, begin);
> +    phdr.p_offset = cpu_to_dump64(s, s->note_offset);
>      phdr.p_paddr = 0;
>      phdr.p_filesz = cpu_to_dump64(s, s->note_size);
>      phdr.p_memsz = cpu_to_dump64(s, s->note_size);
> @@ -303,13 +298,12 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>
>  static void write_elf32_note(DumpState *s, Error **errp)
>  {
> -    hwaddr begin = s->memory_offset - s->note_size;
>      Elf32_Phdr phdr;
>      int ret;
>
>      memset(&phdr, 0, sizeof(Elf32_Phdr));
>      phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> -    phdr.p_offset = cpu_to_dump32(s, begin);
> +    phdr.p_offset = cpu_to_dump32(s, s->note_offset);
>      phdr.p_paddr = 0;
>      phdr.p_filesz = cpu_to_dump32(s, s->note_size);
>      phdr.p_memsz = cpu_to_dump32(s, s->note_size);
> @@ -1828,15 +1822,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      }
>
>      if (s->dump_info.d_class == ELFCLASS64) {
> -        s->memory_offset = sizeof(Elf64_Ehdr) +
> -                           sizeof(Elf64_Phdr) * s->phdr_num +
> -                           sizeof(Elf64_Shdr) * s->shdr_num +
> -                           s->note_size;
> +        s->phdr_offset = sizeof(Elf64_Ehdr);
> +        s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
> +        s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
> +        s->memory_offset = s->note_offset + s->note_size;
>      } else {
> -        s->memory_offset = sizeof(Elf32_Ehdr) +
> -                           sizeof(Elf32_Phdr) * s->phdr_num +
> -                           sizeof(Elf32_Shdr) * s->shdr_num +
> -                           s->note_size;
> +        s->phdr_offset = sizeof(Elf32_Ehdr);
> +        s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
> +        s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
> +        s->memory_offset = s->note_offset + s->note_size;
>      }
>
>      return;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 19458bffbd..ffc2ea1072 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -159,6 +159,10 @@ typedef struct DumpState {
>      bool resume;
>      bool detached;
>      ssize_t note_size;
> +    hwaddr shdr_offset;
> +    hwaddr phdr_offset;
> +    hwaddr section_offset;
> +    hwaddr note_offset;
>      hwaddr memory_offset;
>      int fd;
>
> --
> 2.32.0
>

Re: [PATCH 7/7] dump: Consolidate elf note function

[Marc-André Lureau]

Hi

On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Just like with the other write functions let's move the 32/64 bit elf
> handling to a function to improve readability.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 78654b9c27..9ba0392e00 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
>      }
>  }
>
> +static void write_elf_notes(DumpState *s, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    if (dump_is_64bit(s)) {
> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
> +    } else {
> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
> +    }
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Please use "modern"-style ERRP_GUARD(), and indicate failure with a
bool (see include/qapi/error.h)

(perhaps this should be preliminary to this series)

> +}
> +
>  /* write elf header, PT_NOTE and elf note to vmcore. */
>  static void dump_begin(DumpState *s, Error **errp)
>  {
> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
>          }
>      }
>
> -    if (dump_is_64bit(s)) {
> -        /* write notes to vmcore */
> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
> -    } else {
> -        /* write notes to vmcore */
> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
> -    }
> +    /* write notes to vmcore */
> +    write_elf_notes(s, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> --
> 2.32.0
>

Re: [PATCH 7/7] dump: Consolidate elf note function

[Janosch Frank]

On 3/2/22 11:30, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>> Just like with the other write functions let's move the 32/64 bit elf
>> handling to a function to improve readability.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   dump/dump.c | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 78654b9c27..9ba0392e00 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
>>       }
>>   }
>>
>> +static void write_elf_notes(DumpState *s, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    if (dump_is_64bit(s)) {
>> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
>> +    } else {
>> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
>> +    }
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
> 
> Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> bool (see include/qapi/error.h)

Didn't know that's a thing, I'll have a look

> 
> (perhaps this should be preliminary to this series)

So you want me to change all the local_error + error_propagate()s in 
this file?

> 
>> +}
>> +
>>   /* write elf header, PT_NOTE and elf note to vmcore. */
>>   static void dump_begin(DumpState *s, Error **errp)
>>   {
>> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
>>           }
>>       }
>>
>> -    if (dump_is_64bit(s)) {
>> -        /* write notes to vmcore */
>> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
>> -    } else {
>> -        /* write notes to vmcore */
>> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
>> -    }
>> +    /* write notes to vmcore */
>> +    write_elf_notes(s, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>           return;
>> --
>> 2.32.0
>>
> 

Re: [PATCH 7/7] dump: Consolidate elf note function

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

Hi

On Wed, Mar 2, 2022 at 6:02 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/2/22 11:30, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com>
> wrote:
> >>
> >> Just like with the other write functions let's move the 32/64 bit elf
> >> handling to a function to improve readability.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>   dump/dump.c | 24 +++++++++++++++++-------
> >>   1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index 78654b9c27..9ba0392e00 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> >>       }
> >>   }
> >>
> >> +static void write_elf_notes(DumpState *s, Error **errp)
> >> +{
> >> +    Error *local_err = NULL;
> >> +
> >> +    if (dump_is_64bit(s)) {
> >> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> +    } else {
> >> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> +    }
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >
> > Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> > bool (see include/qapi/error.h)
>
> Didn't know that's a thing, I'll have a look
>
> >
> > (perhaps this should be preliminary to this series)
>
> So you want me to change all the local_error + error_propagate()s in
> this file?
>
>
It's not mandatory, but if you do that would be nice. Otherwise, try to
update the code you touch.

thanks



> >
> >> +}
> >> +
> >>   /* write elf header, PT_NOTE and elf note to vmcore. */
> >>   static void dump_begin(DumpState *s, Error **errp)
> >>   {
> >> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
> >>           }
> >>       }
> >>
> >> -    if (dump_is_64bit(s)) {
> >> -        /* write notes to vmcore */
> >> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> -    } else {
> >> -        /* write notes to vmcore */
> >> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> -    }
> >> +    /* write notes to vmcore */
> >> +    write_elf_notes(s, &local_err);
> >>       if (local_err) {
> >>           error_propagate(errp, local_err);
> >>           return;
> >> --
> >> 2.32.0
> >>
> >
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3827 bytes --]

Re: [PATCH 1/7] dump: Introduce shdr_num to decrease complexity

[Janosch Frank]

On 3/1/22 15:42, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 1, 2022 at 6:33 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Let's move from a boolean to a int variable which will later enable us
>> to store the number of sections that are in the dump file.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   dump/dump.c           | 43 ++++++++++++++++++-------------------------
>>   include/sysemu/dump.h |  2 +-
>>   2 files changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index a84d8b1598..6696d9819a 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error
>> **errp)
>>       elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
>>       elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
>>       elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
>> -    if (s->have_section) {
>> +    if (s->shdr_num) {
>>           uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
>> s->sh_info;
>>
>>           elf_header.e_shoff = cpu_to_dump64(s, shoff);
>>           elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
>> -        elf_header.e_shnum = cpu_to_dump16(s, 1);
>> +        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>>       }
>>
>>       ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
>> @@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error
>> **errp)
>>       elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
>>       elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
>>       elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
>> -    if (s->have_section) {
>> +    if (s->shdr_num) {
>>           uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
>> s->sh_info;
>>
>>           elf_header.e_shoff = cpu_to_dump32(s, shoff);
>>           elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
>> -        elf_header.e_shnum = cpu_to_dump16(s, 1);
>> +        elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>>       }
>>
>>       ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
>> @@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
>>       uint32_t max_index;
>>       Error *local_err = NULL;
>>
>> -    if (s->have_section) {
>> +    if (s->shdr_num) {
>>           max_index = s->sh_info;
>>       } else {
>>           max_index = s->phdr_num;
>> @@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
>>           }
>>
>>           /* write section to vmcore */
>> -        if (s->have_section) {
>> +        if (s->shdr_num) {
>>               write_elf_section(s, 1, &local_err);
>>               if (local_err) {
>>                   error_propagate(errp, local_err);
>> @@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
>>           }
>>
>>           /* write section to vmcore */
>> -        if (s->have_section) {
>> +        if (s->shdr_num) {
>>               write_elf_section(s, 0, &local_err);
>>               if (local_err) {
>>                   error_propagate(errp, local_err);
>> @@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool
>> has_format,
>>        */
>>       s->phdr_num = 1; /* PT_NOTE */
>>       if (s->list.num < UINT16_MAX - 2) {
>> +        s->shdr_num = 0;
>>           s->phdr_num += s->list.num;
>> -        s->have_section = false;
>>       } else {
>> -        s->have_section = true;
>> +        /* sh_info of section 0 holds the real number of phdrs */
>>
> 
> ...
> 
> 
>>           s->phdr_num = PN_XNUM;
>> +        s->shdr_num = 1;
>>           s->sh_info = 1; /* PT_NOTE */
>>
>>           /* the type of shdr->sh_info is uint32_t, so we should avoid
>> overflow */
>> @@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool
>> has_format,
>>       }
>>
>>       if (s->dump_info.d_class == ELFCLASS64) {
>> -        if (s->have_section) {
>> -            s->memory_offset = sizeof(Elf64_Ehdr) +
>> -                               sizeof(Elf64_Phdr) * s->sh_info +
>> -                               sizeof(Elf64_Shdr) + s->note_size;
>> -        } else {
>> -            s->memory_offset = sizeof(Elf64_Ehdr) +
>> -                               sizeof(Elf64_Phdr) * s->phdr_num +
>> s->note_size;
>> -        }
>> +        s->memory_offset = sizeof(Elf64_Ehdr) +
>> +                           sizeof(Elf64_Phdr) * s->sh_info +
>>
> 
> The change "removing/replacing sizeof(Elf64_Phdr) * s->phdr_num by *
> s->sh_info" should be done as a preliminary step, with more comments.

I had another look at this and this patch won't work without the next 
patch anyway. I'll try to recombine and split the patches in a way that 
makes this easier to read.


> 
> +                           sizeof(Elf64_Shdr) * s->shdr_num +
>> +                           s->note_size;
>>       } else {
>> -        if (s->have_section) {
>> -            s->memory_offset = sizeof(Elf32_Ehdr) +
>> -                               sizeof(Elf32_Phdr) * s->sh_info +
>> -                               sizeof(Elf32_Shdr) + s->note_size;
>> -        } else {
>> -            s->memory_offset = sizeof(Elf32_Ehdr) +
>> -                               sizeof(Elf32_Phdr) * s->phdr_num +
>> s->note_size;
>> -        }
>> +        s->memory_offset = sizeof(Elf32_Ehdr) +
>> +                           sizeof(Elf32_Phdr) * s->sh_info +
>> +                           sizeof(Elf32_Shdr) * s->shdr_num +
>> +                           s->note_size;
>>       }
>>
>>       return;
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 250143cb5a..854341da0d 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -155,8 +155,8 @@ typedef struct DumpState {
>>       ArchDumpInfo dump_info;
>>       MemoryMappingList list;
>>       uint16_t phdr_num;
>> +    uint32_t shdr_num;
>>       uint32_t sh_info;
>> -    bool have_section;
>>       bool resume;
>>       bool detached;
>>       ssize_t note_size;
>> --
>> 2.32.0
>>
>>
>>
>