* [PATCH v2 1/9] dump: Use ERRP_GUARD()
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 18:45 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 2/9] dump: Remove the sh_info variable Janosch Frank
` (7 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
Let's move to the new way of handling errors before changing the dump
code. This patch has mostly been generated by the coccinelle script
scripts/coccinelle/errp-guard.cocci.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 144 ++++++++++++++++++++++------------------------------
1 file changed, 61 insertions(+), 83 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index a84d8b1598..ef1700174b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -390,23 +390,21 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
static void write_memory(DumpState *s, GuestPhysBlock *block, ram_addr_t start,
int64_t size, Error **errp)
{
+ ERRP_GUARD();
int64_t i;
- Error *local_err = NULL;
for (i = 0; i < size / s->dump_info.page_size; i++) {
write_data(s, block->host_addr + start + i * s->dump_info.page_size,
- s->dump_info.page_size, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ s->dump_info.page_size, errp);
+ if (*errp) {
return;
}
}
if ((size % s->dump_info.page_size) != 0) {
write_data(s, block->host_addr + start + i * s->dump_info.page_size,
- size % s->dump_info.page_size, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ size % s->dump_info.page_size, errp);
+ if (*errp) {
return;
}
}
@@ -476,11 +474,11 @@ static void get_offset_range(hwaddr phys_addr,
static void write_elf_loads(DumpState *s, Error **errp)
{
+ ERRP_GUARD();
hwaddr offset, filesz;
MemoryMapping *memory_mapping;
uint32_t phdr_index = 1;
uint32_t max_index;
- Error *local_err = NULL;
if (s->have_section) {
max_index = s->sh_info;
@@ -494,14 +492,13 @@ static void write_elf_loads(DumpState *s, Error **errp)
s, &offset, &filesz);
if (s->dump_info.d_class == ELFCLASS64) {
write_elf64_load(s, memory_mapping, phdr_index++, offset,
- filesz, &local_err);
+ filesz, errp);
} else {
write_elf32_load(s, memory_mapping, phdr_index++, offset,
- filesz, &local_err);
+ filesz, errp);
}
- if (local_err) {
- error_propagate(errp, local_err);
+ if (*errp) {
return;
}
@@ -514,7 +511,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
/* write elf header, PT_NOTE and elf note to vmcore. */
static void dump_begin(DumpState *s, Error **errp)
{
- Error *local_err = NULL;
+ ERRP_GUARD();
/*
* the vmcore's format is:
@@ -542,73 +539,64 @@ static void dump_begin(DumpState *s, Error **errp)
/* write elf header to vmcore */
if (s->dump_info.d_class == ELFCLASS64) {
- write_elf64_header(s, &local_err);
+ write_elf64_header(s, errp);
} else {
- write_elf32_header(s, &local_err);
+ write_elf32_header(s, errp);
}
- if (local_err) {
- error_propagate(errp, local_err);
+ if (*errp) {
return;
}
if (s->dump_info.d_class == ELFCLASS64) {
/* write PT_NOTE to vmcore */
- write_elf64_note(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf64_note(s, errp);
+ if (*errp) {
return;
}
/* write all PT_LOAD to vmcore */
- write_elf_loads(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf_loads(s, errp);
+ if (*errp) {
return;
}
/* write section to vmcore */
if (s->have_section) {
- write_elf_section(s, 1, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf_section(s, 1, errp);
+ if (*errp) {
return;
}
}
/* write notes to vmcore */
- write_elf64_notes(fd_write_vmcore, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf64_notes(fd_write_vmcore, s, errp);
+ if (*errp) {
return;
}
} else {
/* write PT_NOTE to vmcore */
- write_elf32_note(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf32_note(s, errp);
+ if (*errp) {
return;
}
/* write all PT_LOAD to vmcore */
- write_elf_loads(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf_loads(s, errp);
+ if (*errp) {
return;
}
/* write section to vmcore */
if (s->have_section) {
- write_elf_section(s, 0, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf_section(s, 0, errp);
+ if (*errp) {
return;
}
}
/* write notes to vmcore */
- write_elf32_notes(fd_write_vmcore, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf32_notes(fd_write_vmcore, s, errp);
+ if (*errp) {
return;
}
}
@@ -644,9 +632,9 @@ static int get_next_block(DumpState *s, GuestPhysBlock *block)
/* write all memory to vmcore */
static void dump_iterate(DumpState *s, Error **errp)
{
+ ERRP_GUARD();
GuestPhysBlock *block;
int64_t size;
- Error *local_err = NULL;
do {
block = s->next_block;
@@ -658,9 +646,8 @@ static void dump_iterate(DumpState *s, Error **errp)
size -= block->target_end - (s->begin + s->length);
}
}
- write_memory(s, block, s->start, size, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_memory(s, block, s->start, size, errp);
+ if (*errp) {
return;
}
@@ -669,11 +656,10 @@ static void dump_iterate(DumpState *s, Error **errp)
static void create_vmcore(DumpState *s, Error **errp)
{
- Error *local_err = NULL;
+ ERRP_GUARD();
- dump_begin(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ dump_begin(s, errp);
+ if (*errp) {
return;
}
@@ -810,6 +796,7 @@ static bool note_name_equal(DumpState *s,
/* write common header, sub header and elf note to vmcore */
static void create_header32(DumpState *s, Error **errp)
{
+ ERRP_GUARD();
DiskDumpHeader32 *dh = NULL;
KdumpSubHeader32 *kh = NULL;
size_t size;
@@ -818,7 +805,6 @@ static void create_header32(DumpState *s, Error **errp)
uint32_t bitmap_blocks;
uint32_t status = 0;
uint64_t offset_note;
- Error *local_err = NULL;
/* write common header, the version of kdump-compressed format is 6th */
size = sizeof(DiskDumpHeader32);
@@ -894,9 +880,8 @@ static void create_header32(DumpState *s, Error **errp)
s->note_buf_offset = 0;
/* use s->note_buf to store notes temporarily */
- write_elf32_notes(buf_write_note, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf32_notes(buf_write_note, s, errp);
+ if (*errp) {
goto out;
}
if (write_buffer(s->fd, offset_note, s->note_buf,
@@ -922,6 +907,7 @@ out:
/* write common header, sub header and elf note to vmcore */
static void create_header64(DumpState *s, Error **errp)
{
+ ERRP_GUARD();
DiskDumpHeader64 *dh = NULL;
KdumpSubHeader64 *kh = NULL;
size_t size;
@@ -930,7 +916,6 @@ static void create_header64(DumpState *s, Error **errp)
uint32_t bitmap_blocks;
uint32_t status = 0;
uint64_t offset_note;
- Error *local_err = NULL;
/* write common header, the version of kdump-compressed format is 6th */
size = sizeof(DiskDumpHeader64);
@@ -1006,9 +991,8 @@ static void create_header64(DumpState *s, Error **errp)
s->note_buf_offset = 0;
/* use s->note_buf to store notes temporarily */
- write_elf64_notes(buf_write_note, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_elf64_notes(buf_write_note, s, errp);
+ if (*errp) {
goto out;
}
@@ -1464,8 +1448,8 @@ out:
static void create_kdump_vmcore(DumpState *s, Error **errp)
{
+ ERRP_GUARD();
int ret;
- Error *local_err = NULL;
/*
* the kdump-compressed format is:
@@ -1495,21 +1479,18 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
return;
}
- write_dump_header(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_dump_header(s, errp);
+ if (*errp) {
return;
}
- write_dump_bitmap(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_dump_bitmap(s, errp);
+ if (*errp) {
return;
}
- write_dump_pages(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ write_dump_pages(s, errp);
+ if (*errp) {
return;
}
@@ -1639,10 +1620,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
DumpGuestMemoryFormat format, bool paging, bool has_filter,
int64_t begin, int64_t length, Error **errp)
{
+ ERRP_GUARD();
VMCoreInfoState *vmci = vmcoreinfo_find();
CPUState *cpu;
int nr_cpus;
- Error *err = NULL;
int ret;
s->has_format = has_format;
@@ -1761,9 +1742,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
/* get memory mapping */
if (paging) {
- qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
- if (err != NULL) {
- error_propagate(errp, err);
+ qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, errp);
+ if (*errp) {
goto cleanup;
}
} else {
@@ -1862,33 +1842,32 @@ cleanup:
/* this operation might be time consuming. */
static void dump_process(DumpState *s, Error **errp)
{
- Error *local_err = NULL;
+ ERRP_GUARD();
DumpQueryResult *result = NULL;
if (s->has_format && s->format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP) {
#ifdef TARGET_X86_64
- create_win_dump(s, &local_err);
+ create_win_dump(s, errp);
#endif
} else if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
- create_kdump_vmcore(s, &local_err);
+ create_kdump_vmcore(s, errp);
} else {
- create_vmcore(s, &local_err);
+ create_vmcore(s, errp);
}
/* make sure status is written after written_size updates */
smp_wmb();
qatomic_set(&s->status,
- (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
+ (*errp ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
/* send DUMP_COMPLETED message (unconditionally) */
result = qmp_query_dump(NULL);
/* should never fail */
assert(result);
- qapi_event_send_dump_completed(result, !!local_err, (local_err ?
- error_get_pretty(local_err) : NULL));
+ qapi_event_send_dump_completed(result, !!*errp, (*errp ?
+ error_get_pretty(*errp) : NULL));
qapi_free_DumpQueryResult(result);
- error_propagate(errp, local_err);
dump_cleanup(s);
}
@@ -1917,10 +1896,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
int64_t length, bool has_format,
DumpGuestMemoryFormat format, Error **errp)
{
+ ERRP_GUARD();
const char *p;
int fd = -1;
DumpState *s;
- Error *local_err = NULL;
bool detach_p = false;
if (runstate_check(RUN_STATE_INMIGRATE)) {
@@ -2020,9 +1999,8 @@ void qmp_dump_guest_memory(bool paging, const char *file,
dump_state_prepare(s);
dump_init(s, fd, has_format, format, paging, has_begin,
- begin, length, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ begin, length, errp);
+ if (*errp) {
qatomic_set(&s->status, DUMP_STATUS_FAILED);
return;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/9] dump: Use ERRP_GUARD()
2022-03-10 11:08 ` [PATCH v2 1/9] dump: Use ERRP_GUARD() Janosch Frank
@ 2022-03-11 18:45 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 18:45 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank wrote:
> Let's move to the new way of handling errors before changing the dump
> code. This patch has mostly been generated by the coccinelle script
> scripts/coccinelle/errp-guard.cocci.
>
> Signed-off-by: Janosch Frank<frankja@linux.ibm.com>
> ---
> dump/dump.c | 144 ++++++++++++++++++++++------------------------------
> 1 file changed, 61 insertions(+), 83 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/9] dump: Remove the sh_info variable
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-10 11:08 ` [PATCH v2 1/9] dump: Use ERRP_GUARD() Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:40 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 34 ++++++++++++++--------------------
include/sysemu/dump.h | 3 +--
2 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index ef1700174b..7015c92177 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->have_section) {
- 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->have_section) {
- 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;
}
@@ -478,13 +480,6 @@ static void write_elf_loads(DumpState *s, Error **errp)
hwaddr offset, filesz;
MemoryMapping *memory_mapping;
uint32_t phdr_index = 1;
- uint32_t max_index;
-
- if (s->have_section) {
- 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,
@@ -502,7 +497,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
return;
}
- if (phdr_index >= max_index) {
+ if (phdr_index >= s->phdr_num) {
break;
}
}
@@ -1801,22 +1796,21 @@ static void dump_init(DumpState *s, int fd, bool has_format,
s->phdr_num += s->list.num;
s->have_section = false;
} else {
+ /* sh_info of section 0 holds the real number of phdrs */
s->have_section = true;
- s->phdr_num = PN_XNUM;
- 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) {
if (s->have_section) {
s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->sh_info +
+ sizeof(Elf64_Phdr) * s->phdr_num +
sizeof(Elf64_Shdr) + s->note_size;
} else {
s->memory_offset = sizeof(Elf64_Ehdr) +
@@ -1825,7 +1819,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
} else {
if (s->have_section) {
s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->sh_info +
+ sizeof(Elf32_Phdr) * s->phdr_num +
sizeof(Elf32_Shdr) + s->note_size;
} else {
s->memory_offset = sizeof(Elf32_Ehdr) +
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 250143cb5a..b463fc9c02 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -154,8 +154,7 @@ typedef struct DumpState {
GuestPhysBlockList guest_phys_blocks;
ArchDumpInfo dump_info;
MemoryMappingList list;
- uint16_t phdr_num;
- uint32_t sh_info;
+ uint32_t phdr_num;
bool have_section;
bool resume;
bool detached;
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/9] dump: Remove the sh_info variable
2022-03-10 11:08 ` [PATCH v2 2/9] dump: Remove the sh_info variable Janosch Frank
@ 2022-03-11 19:40 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:40 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank wrote:
> 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 | 34 ++++++++++++++--------------------
> include/sysemu/dump.h | 3 +--
> 2 files changed, 15 insertions(+), 22 deletions(-)
Nice.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/9] dump: Introduce shdr_num to decrease complexity
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-10 11:08 ` [PATCH v2 1/9] dump: Use ERRP_GUARD() Janosch Frank
2022-03-10 11:08 ` [PATCH v2 2/9] dump: Remove the sh_info variable Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:41 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 24 ++++++++++++------------
include/sysemu/dump.h | 2 +-
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 7015c92177..c5ca6027ae 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -140,12 +140,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, phnum);
- if (s->have_section) {
+ 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_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);
@@ -172,12 +172,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, phnum);
- if (s->have_section) {
+ 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_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);
@@ -556,7 +556,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, errp);
if (*errp) {
return;
@@ -582,7 +582,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, errp);
if (*errp) {
return;
@@ -1793,11 +1793,11 @@ 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 {
/* sh_info of section 0 holds the real number of phdrs */
- s->have_section = true;
+ s->shdr_num = 1;
/* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
if (s->list.num <= UINT32_MAX - 1) {
@@ -1808,19 +1808,19 @@ static void dump_init(DumpState *s, int fd, bool has_format,
}
if (s->dump_info.d_class == ELFCLASS64) {
- if (s->have_section) {
+ if (s->shdr_num) {
s->memory_offset = sizeof(Elf64_Ehdr) +
sizeof(Elf64_Phdr) * s->phdr_num +
- sizeof(Elf64_Shdr) + s->note_size;
+ sizeof(Elf64_Shdr) * s->shdr_num + s->note_size;
} else {
s->memory_offset = sizeof(Elf64_Ehdr) +
sizeof(Elf64_Phdr) * s->phdr_num + s->note_size;
}
} else {
- if (s->have_section) {
+ if (s->shdr_num) {
s->memory_offset = sizeof(Elf32_Ehdr) +
sizeof(Elf32_Phdr) * s->phdr_num +
- sizeof(Elf32_Shdr) + s->note_size;
+ sizeof(Elf32_Shdr) * s->shdr_num + s->note_size;
} else {
s->memory_offset = sizeof(Elf32_Ehdr) +
sizeof(Elf32_Phdr) * s->phdr_num + s->note_size;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index b463fc9c02..19458bffbd 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -155,7 +155,7 @@ typedef struct DumpState {
ArchDumpInfo dump_info;
MemoryMappingList list;
uint32_t phdr_num;
- bool have_section;
+ uint32_t shdr_num;
bool resume;
bool detached;
ssize_t note_size;
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/9] dump: Introduce shdr_num to decrease complexity
2022-03-10 11:08 ` [PATCH v2 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-11 19:41 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:41 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank 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 | 24 ++++++++++++------------
> include/sysemu/dump.h | 2 +-
> 2 files changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/9] dump: Remove the section if when calculating the memory offset
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (2 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:42 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 5/9] dump: Add more offset variables Janosch Frank
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
When s->shdr_num is 0 we'll add 0 bytes of section headers which is
equivalent to not adding section headers but with the multiplication
we can remove a if/else.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index c5ca6027ae..316c636f24 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1808,23 +1808,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
}
if (s->dump_info.d_class == ELFCLASS64) {
- if (s->shdr_num) {
- s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->phdr_num +
- sizeof(Elf64_Shdr) * s->shdr_num + 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->phdr_num +
+ sizeof(Elf64_Shdr) * s->shdr_num +
+ s->note_size;
} else {
- if (s->shdr_num) {
- s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->phdr_num +
- sizeof(Elf32_Shdr) * s->shdr_num + 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->phdr_num +
+ sizeof(Elf32_Shdr) * s->shdr_num +
+ s->note_size;
}
return;
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 5/9] dump: Add more offset variables
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (3 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:44 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 35 +++++++++++++++--------------------
include/sysemu/dump.h | 4 ++++
2 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 316c636f24..12b3a1a83e 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);
@@ -1808,15 +1802,16 @@ 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/9] dump: Add more offset variables
2022-03-10 11:08 ` [PATCH v2 5/9] dump: Add more offset variables Janosch Frank
@ 2022-03-11 19:44 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:44 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank 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 | 35 +++++++++++++++--------------------
> include/sysemu/dump.h | 4 ++++
> 2 files changed, 19 insertions(+), 20 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 6/9] dump: Introduce dump_is_64bit() helper function
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (4 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 5/9] dump: Add more offset variables Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:47 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 7/9] dump: Consolidate phdr note writes Janosch Frank
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 12b3a1a83e..8fcd23571f 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -55,6 +55,11 @@ static Error *dump_migration_blocker;
DIV_ROUND_UP((name_size), 4) + \
DIV_ROUND_UP((desc_size), 4)) * 4)
+static inline bool dump_is_64bit(DumpState *s)
+{
+ return s->dump_info.d_class == ELFCLASS64;
+}
+
uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
{
if (s->dump_info.d_endian == ELFDATA2LSB) {
@@ -479,7 +484,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, errp);
} else {
@@ -527,7 +532,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, errp);
} else {
write_elf32_header(s, errp);
@@ -536,7 +541,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, errp);
if (*errp) {
@@ -747,7 +752,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);
@@ -1007,7 +1012,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);
@@ -1697,7 +1702,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);
@@ -1801,7 +1806,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;
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/9] dump: Introduce dump_is_64bit() helper function
2022-03-10 11:08 ` [PATCH v2 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-11 19:47 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:47 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank 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 | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> @@ -1007,7 +1012,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);
> @@ -1697,7 +1702,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);
It would be nice to standardize on positive tests, which in this case would reverse these
two conditionals.
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 7/9] dump: Consolidate phdr note writes
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (5 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:49 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 8/9] dump: Cleanup dump_begin write functions Janosch Frank
2022-03-10 11:08 ` [PATCH v2 9/9] dump: Consolidate elf note function Janosch Frank
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 94 +++++++++++++++++++++++++++--------------------------
1 file changed, 48 insertions(+), 46 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 8fcd23571f..1294673444 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -236,24 +236,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)
@@ -301,24 +292,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,
@@ -348,6 +330,32 @@ 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)
+{
+ ERRP_GUARD();
+ 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;
@@ -541,13 +549,13 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
- if (dump_is_64bit(s)) {
- /* write PT_NOTE to vmcore */
- write_elf64_note(s, errp);
- if (*errp) {
- return;
- }
+ /* write PT_NOTE to vmcore */
+ write_elf_phdr_note(s, errp);
+ if (*errp) {
+ return;
+ }
+ if (dump_is_64bit(s)) {
/* write all PT_LOAD to vmcore */
write_elf_loads(s, errp);
if (*errp) {
@@ -568,12 +576,6 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
} else {
- /* write PT_NOTE to vmcore */
- write_elf32_note(s, errp);
- if (*errp) {
- return;
- }
-
/* write all PT_LOAD to vmcore */
write_elf_loads(s, errp);
if (*errp) {
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 7/9] dump: Consolidate phdr note writes
2022-03-10 11:08 ` [PATCH v2 7/9] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-11 19:49 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:49 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank 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 | 94 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 48 insertions(+), 46 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 8/9] dump: Cleanup dump_begin write functions
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (6 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 7/9] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:51 ` Richard Henderson
2022-03-10 11:08 ` [PATCH v2 9/9] dump: Consolidate elf note function Janosch Frank
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 42 +++++++++++-------------------------------
1 file changed, 11 insertions(+), 31 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 1294673444..5542adf7b8 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -555,46 +555,26 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
- if (dump_is_64bit(s)) {
- /* write all PT_LOAD to vmcore */
- write_elf_loads(s, errp);
+ /* write all PT_LOAD to vmcore */
+ write_elf_loads(s, errp);
+ if (*errp) {
+ return;
+ }
+
+ /* write section to vmcore */
+ if (s->shdr_num) {
+ write_elf_section(s, 1, errp);
if (*errp) {
return;
}
+ }
- /* write section to vmcore */
- if (s->shdr_num) {
- write_elf_section(s, 1, errp);
- if (*errp) {
- return;
- }
- }
-
+ if (dump_is_64bit(s)) {
/* write notes to vmcore */
write_elf64_notes(fd_write_vmcore, s, errp);
- if (*errp) {
- return;
- }
} else {
- /* write all PT_LOAD to vmcore */
- write_elf_loads(s, errp);
- if (*errp) {
- return;
- }
-
- /* write section to vmcore */
- if (s->shdr_num) {
- write_elf_section(s, 0, errp);
- if (*errp) {
- return;
- }
- }
-
/* write notes to vmcore */
write_elf32_notes(fd_write_vmcore, s, errp);
- if (*errp) {
- return;
- }
}
}
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 8/9] dump: Cleanup dump_begin write functions
2022-03-10 11:08 ` [PATCH v2 8/9] dump: Cleanup dump_begin write functions Janosch Frank
@ 2022-03-11 19:51 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:51 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank wrote:
> 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 | 42 +++++++++++-------------------------------
> 1 file changed, 11 insertions(+), 31 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 9/9] dump: Consolidate elf note function
2022-03-10 11:08 [PATCH v2 0/9] dump: Cleanup and consolidation Janosch Frank
` (7 preceding siblings ...)
2022-03-10 11:08 ` [PATCH v2 8/9] dump: Cleanup dump_begin write functions Janosch Frank
@ 2022-03-10 11:08 ` Janosch Frank
2022-03-11 19:54 ` Richard Henderson
8 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-03-10 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
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 | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 5542adf7b8..ae8ec527de 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -510,6 +510,20 @@ static void write_elf_loads(DumpState *s, Error **errp)
}
}
+static void write_elf_notes(DumpState *s, Error **errp)
+{
+ ERRP_GUARD();
+
+ if (dump_is_64bit(s)) {
+ write_elf64_notes(fd_write_vmcore, s, errp);
+ } else {
+ write_elf32_notes(fd_write_vmcore, s, errp);
+ }
+ if (*errp) {
+ return;
+ }
+}
+
/* write elf header, PT_NOTE and elf note to vmcore. */
static void dump_begin(DumpState *s, Error **errp)
{
@@ -569,13 +583,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, errp);
- } else {
- /* write notes to vmcore */
- write_elf32_notes(fd_write_vmcore, s, errp);
- }
+ /* write notes to vmcore */
+ write_elf_notes(s, errp);
}
static int get_next_block(DumpState *s, GuestPhysBlock *block)
--
2.32.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 9/9] dump: Consolidate elf note function
2022-03-10 11:08 ` [PATCH v2 9/9] dump: Consolidate elf note function Janosch Frank
@ 2022-03-11 19:54 ` Richard Henderson
0 siblings, 0 replies; 19+ messages in thread
From: Richard Henderson @ 2022-03-11 19:54 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: marcandre.lureau, pbonzini
On 3/10/22 03:08, Janosch Frank wrote:
> +static void write_elf_notes(DumpState *s, Error **errp)
> +{
> + ERRP_GUARD();
> +
> + if (dump_is_64bit(s)) {
> + write_elf64_notes(fd_write_vmcore, s, errp);
> + } else {
> + write_elf32_notes(fd_write_vmcore, s, errp);
> + }
> + if (*errp) {
> + return;
> + }
> +}
Are you anticipating adding more code to this function?
Otherwise the early return, and the guard are useless.
r~
^ permalink raw reply [flat|nested] 19+ messages in thread