* [PATCH v3 0/9] dump: Cleanup and consolidation
@ 2022-03-30 12:35 Janosch Frank
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
` (8 more replies)
0 siblings, 9 replies; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, pbonzini
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:
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.
v3:
* Rebased
* Moved 64 bit tests to positive checks
* Removed unneeded code
* Added review-bys
v2:
* Added the ERRP_GUARD() patch which converts dump.c to the
new error handling methods
* Switched patches #2 and #3 around
* Added a patch that removes the section if/else
* Moved dump_is_64bit() to dump.c
Janosch Frank (9):
dump: Use ERRP_GUARD()
dump: Remove the sh_info variable
dump: Introduce shdr_num to decrease complexity
dump: Remove the section if when calculating the memory offset
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 | 362 ++++++++++++++++++------------------------
include/sysemu/dump.h | 9 +-
2 files changed, 162 insertions(+), 209 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/9] dump: Use ERRP_GUARD()
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
@ 2022-03-30 12:35 ` Janosch Frank
2022-03-31 8:59 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 2/9] dump: Remove the sh_info variable Janosch Frank
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
dump/dump.c | 144 ++++++++++++++++++++++------------------------------
1 file changed, 61 insertions(+), 83 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index f57ed76fa7..58c4923fce 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] 23+ messages in thread
* [PATCH v3 2/9] dump: Remove the sh_info variable
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
@ 2022-03-30 12:35 ` Janosch Frank
2022-03-31 8:58 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
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 58c4923fce..0e6187c962 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] 23+ messages in thread
* [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
2022-03-30 12:35 ` [PATCH v3 2/9] dump: Remove the sh_info variable Janosch Frank
@ 2022-03-30 12:35 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
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 0e6187c962..cd11dec0f4 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] 23+ messages in thread
* [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (2 preceding siblings ...)
2022-03-30 12:35 ` [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-30 12:35 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 5/9] dump: Add more offset variables Janosch Frank
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
dump/dump.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index cd11dec0f4..bdff651da4 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] 23+ messages in thread
* [PATCH v3 5/9] dump: Add more offset variables
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (3 preceding siblings ...)
2022-03-30 12:35 ` [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
@ 2022-03-30 12:35 ` Janosch Frank
2022-03-30 12:36 ` [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
` (3 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:35 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
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 bdff651da4..4be4dcab24 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] 23+ messages in thread
* [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (4 preceding siblings ...)
2022-03-30 12:35 ` [PATCH v3 5/9] dump: Add more offset variables Janosch Frank
@ 2022-03-30 12:36 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 7/9] dump: Consolidate phdr note writes Janosch Frank
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:36 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
dump/dump.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 4be4dcab24..a7cf112d8f 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,10 +1012,10 @@ out:
static void write_dump_header(DumpState *s, Error **errp)
{
- if (s->dump_info.d_class == ELFCLASS32) {
- create_header32(s, errp);
- } else {
+ if (dump_is_64bit(s)) {
create_header64(s, errp);
+ } else {
+ create_header32(s, errp);
}
}
@@ -1697,8 +1702,8 @@ 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 ?
- sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
+ note_head_size = dump_is_64bit(s) ?
+ sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);
format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
size = le32_to_cpu(vmci->vmcoreinfo.size);
@@ -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] 23+ messages in thread
* [PATCH v3 7/9] dump: Consolidate phdr note writes
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (5 preceding siblings ...)
2022-03-30 12:36 ` [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-30 12:36 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 8/9] dump: Cleanup dump_begin write functions Janosch Frank
2022-03-30 12:36 ` [PATCH v3 9/9] dump: Consolidate elf note function Janosch Frank
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:36 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
dump/dump.c | 94 +++++++++++++++++++++++++++--------------------------
1 file changed, 48 insertions(+), 46 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index a7cf112d8f..365798f5a1 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] 23+ messages in thread
* [PATCH v3 8/9] dump: Cleanup dump_begin write functions
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (6 preceding siblings ...)
2022-03-30 12:36 ` [PATCH v3 7/9] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-30 12:36 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 9/9] dump: Consolidate elf note function Janosch Frank
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:36 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
dump/dump.c | 42 +++++++++++-------------------------------
1 file changed, 11 insertions(+), 31 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 365798f5a1..92acd9eb5c 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] 23+ messages in thread
* [PATCH v3 9/9] dump: Consolidate elf note function
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
` (7 preceding siblings ...)
2022-03-30 12:36 ` [PATCH v3 8/9] dump: Cleanup dump_begin write functions Janosch Frank
@ 2022-03-30 12:36 ` Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
8 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-30 12:36 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, richard.henderson, 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 | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 92acd9eb5c..4af71de9b1 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -510,6 +510,15 @@ static void write_elf_loads(DumpState *s, Error **errp)
}
}
+static void write_elf_notes(DumpState *s, Error **errp)
+{
+ if (dump_is_64bit(s)) {
+ write_elf64_notes(fd_write_vmcore, s, errp);
+ } else {
+ write_elf32_notes(fd_write_vmcore, s, errp);
+ }
+}
+
/* write elf header, PT_NOTE and elf note to vmcore. */
static void dump_begin(DumpState *s, Error **errp)
{
@@ -569,13 +578,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] 23+ messages in thread
* Re: [PATCH v3 9/9] dump: Consolidate elf note function
2022-03-30 12:36 ` [PATCH v3 9/9] dump: Consolidate elf note function Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]
On Wed, Mar 30, 2022 at 4:37 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>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 92acd9eb5c..4af71de9b1 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -510,6 +510,15 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> }
> }
>
> +static void write_elf_notes(DumpState *s, Error **errp)
> +{
> + if (dump_is_64bit(s)) {
> + write_elf64_notes(fd_write_vmcore, s, errp);
> + } else {
> + write_elf32_notes(fd_write_vmcore, s, errp);
> + }
> +}
> +
> /* write elf header, PT_NOTE and elf note to vmcore. */
> static void dump_begin(DumpState *s, Error **errp)
> {
> @@ -569,13 +578,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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2402 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 8/9] dump: Cleanup dump_begin write functions
2022-03-30 12:36 ` [PATCH v3 8/9] dump: Cleanup dump_begin write functions Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]
On Wed, Mar 30, 2022 at 4:52 PM Janosch Frank <frankja@linux.ibm.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 42 +++++++++++-------------------------------
> 1 file changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 365798f5a1..92acd9eb5c 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3337 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/9] dump: Consolidate phdr note writes
2022-03-30 12:36 ` [PATCH v3 7/9] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]
On Wed, Mar 30, 2022 at 4:49 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 94 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 48 insertions(+), 46 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a7cf112d8f..365798f5a1 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6621 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function
2022-03-30 12:36 ` [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]
On Wed, Mar 30, 2022 at 4:44 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 4be4dcab24..a7cf112d8f 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,10 +1012,10 @@ out:
>
> static void write_dump_header(DumpState *s, Error **errp)
> {
> - if (s->dump_info.d_class == ELFCLASS32) {
> - create_header32(s, errp);
> - } else {
> + if (dump_is_64bit(s)) {
> create_header64(s, errp);
> + } else {
> + create_header32(s, errp);
> }
> }
>
> @@ -1697,8 +1702,8 @@ 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 ?
> - sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
> + note_head_size = dump_is_64bit(s) ?
> + sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);
>
> format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> size = le32_to_cpu(vmci->vmcoreinfo.size);
> @@ -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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 5074 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset
2022-03-30 12:35 ` [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]
On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index cd11dec0f4..bdff651da4 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3392 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity
2022-03-30 12:35 ` [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-31 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:56 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 5088 bytes --]
On Wed, Mar 30, 2022 at 4:45 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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 0e6187c962..cd11dec0f4 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6627 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/9] dump: Remove the sh_info variable
2022-03-30 12:35 ` [PATCH v3 2/9] dump: Remove the sh_info variable Janosch Frank
@ 2022-03-31 8:58 ` Marc-André Lureau
2022-03-31 9:47 ` Janosch Frank
0 siblings, 1 reply; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:58 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 6498 bytes --]
Hi
On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <frankja@linux.ibm.com> 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.
>
I am not fond of this change tbh, mixing variables, casting a u32 to u16..
Could you provide more motivation? This seems to contradict also your
cleanup approach in the later patch "Add more offset variables".
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 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 58c4923fce..0e6187c962 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;
>
Please use MIN()
> 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;
>
same
> 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 8464 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
@ 2022-03-31 8:59 ` Marc-André Lureau
2022-03-31 9:48 ` Janosch Frank
0 siblings, 1 reply; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 8:59 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 14160 bytes --]
Hi
On Wed, Mar 30, 2022 at 4:42 PM Janosch Frank <frankja@linux.ibm.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
nice
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
While you are at it, would you add a patch (at the end of this series, to
avoid the churn) to return a bool for success/failure (as recommended in
qapi/error.h)?
thanks
> ---
> dump/dump.c | 144 ++++++++++++++++++++++------------------------------
> 1 file changed, 61 insertions(+), 83 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index f57ed76fa7..58c4923fce 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
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 17400 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/9] dump: Remove the sh_info variable
2022-03-31 8:58 ` Marc-André Lureau
@ 2022-03-31 9:47 ` Janosch Frank
2022-03-31 13:49 ` Marc-André Lureau
0 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-31 9:47 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, Richard Henderson, QEMU
On 3/31/22 10:58, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <frankja@linux.ibm.com> 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.
>>
>
> I am not fond of this change tbh, mixing variables, casting a u32 to u16..
>
> Could you provide more motivation? This seems to contradict also your
> cleanup approach in the later patch "Add more offset variables".
I can't fully follow you there. Where do I cast to u16 or mix variables?
My idea for this change:
ph_num is made independent of the data structure that it ends up in. We
use ph_num as the only source for decisions and elf structure
manipulation down the line since it contains the maximum possible number
of section headers.
This way we move the extra handling to the locations where it belongs
and where an explanation for that behavior makes most sense:
The ehdr write function and the section write function
Without knowing the ELF spec, could you tell me what sh_info stores when
first reading this code? Going from the name it might be section header
information, whatever that would mean.
I'd be happy to add comments to write_elf(32|64)_header() though so it's
a bit more clear why we need to set PN_XNUM. I've already added one to
dump_begin but that's not where we use PN_XNUM.
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> 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 58c4923fce..0e6187c962 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;
>>
>
> Please use MIN()
>
>
>> 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;
>>
>
> same
>
>
>> 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 [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()
2022-03-31 8:59 ` Marc-André Lureau
@ 2022-03-31 9:48 ` Janosch Frank
2022-03-31 10:11 ` Marc-André Lureau
0 siblings, 1 reply; 23+ messages in thread
From: Janosch Frank @ 2022-03-31 9:48 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, Richard Henderson, QEMU
On 3/31/22 10:59, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 30, 2022 at 4:42 PM Janosch Frank <frankja@linux.ibm.com> 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>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>
> nice
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Thanks!
>
> While you are at it, would you add a patch (at the end of this series, to
> avoid the churn) to return a bool for success/failure (as recommended in
> qapi/error.h)?
I knew it was a mistake to touch this file :)
Sure, it will be churn anyway since I have two more patch sets that
follow on this one.
>
> thanks
>
>
>> ---
>> dump/dump.c | 144 ++++++++++++++++++++++------------------------------
>> 1 file changed, 61 insertions(+), 83 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index f57ed76fa7..58c4923fce 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 [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()
2022-03-31 9:48 ` Janosch Frank
@ 2022-03-31 10:11 ` Marc-André Lureau
0 siblings, 0 replies; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 10:11 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 16070 bytes --]
On Thu, Mar 31, 2022 at 1:48 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> On 3/31/22 10:59, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 30, 2022 at 4:42 PM Janosch Frank <frankja@linux.ibm.com>
> 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>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >>
> >
> > nice
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Thanks!
>
> >
> > While you are at it, would you add a patch (at the end of this series, to
> > avoid the churn) to return a bool for success/failure (as recommended in
> > qapi/error.h)?
>
> I knew it was a mistake to touch this file :)
>
> Sure, it will be churn anyway since I have two more patch sets that
> follow on this one.
>
>
Feel free to leave that cleanup for now
thanks
> >
> > thanks
> >
> >
> >> ---
> >> dump/dump.c | 144 ++++++++++++++++++++++------------------------------
> >> 1 file changed, 61 insertions(+), 83 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index f57ed76fa7..58c4923fce 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
> >>
> >>
> >>
> >
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 22132 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/9] dump: Remove the sh_info variable
2022-03-31 9:47 ` Janosch Frank
@ 2022-03-31 13:49 ` Marc-André Lureau
2022-04-07 9:48 ` [PATCH v4] " Janosch Frank
0 siblings, 1 reply; 23+ messages in thread
From: Marc-André Lureau @ 2022-03-31 13:49 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, Richard Henderson, QEMU
[-- Attachment #1: Type: text/plain, Size: 8678 bytes --]
Hi
On Thu, Mar 31, 2022 at 1:47 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> On 3/31/22 10:58, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <frankja@linux.ibm.com>
> 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.
> >>
> >
> > I am not fond of this change tbh, mixing variables, casting a u32 to
> u16..
> >
> > Could you provide more motivation? This seems to contradict also your
> > cleanup approach in the later patch "Add more offset variables".
>
> I can't fully follow you there. Where do I cast to u16 or mix variables?
>
"uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num"
It's an implicit type casting from u32.
As for mixing variables, we used to have this obvious:
shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
And now:
shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> My idea for this change:
> ph_num is made independent of the data structure that it ends up in. We
> use ph_num as the only source for decisions and elf structure
> manipulation down the line since it contains the maximum possible number
> of section headers.
>
> This way we move the extra handling to the locations where it belongs
> and where an explanation for that behavior makes most sense:
> The ehdr write function and the section write function
>
> Without knowing the ELF spec, could you tell me what sh_info stores when
> first reading this code? Going from the name it might be section header
> information, whatever that would mean.
>
> I'd be happy to add comments to write_elf(32|64)_header() though so it's
> a bit more clear why we need to set PN_XNUM. I've already added one to
> dump_begin but that's not where we use PN_XNUM.
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
The more we could clarify and document the code, the better ;) But since
you already got the change approved by Richard, I won't hold it.
thanks
> >> ---
> >> 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 58c4923fce..0e6187c962 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;
> >>
> >
> > Please use MIN()
> >
> >
> >> 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;
> >>
> >
> > same
> >
> >
> >> 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
> >>
> >>
> >>
> >
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 12017 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4] dump: Remove the sh_info variable
2022-03-31 13:49 ` Marc-André Lureau
@ 2022-04-07 9:48 ` Janosch Frank
0 siblings, 0 replies; 23+ messages in thread
From: Janosch Frank @ 2022-04-07 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, richard.henderson, marcandre.lureau
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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
A question out of general curiosity:
Is PN_XNUM a real concern anyway?
Are architectures using >65k segments in real life?
* Uses MIN()
* Added explanation for the PN_XNUM usage
---
dump/dump.c | 44 +++++++++++++++++++++++--------------------
include/sysemu/dump.h | 3 +--
2 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 58c4923fce..56cd1b2bb8 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -124,6 +124,12 @@ static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
static void write_elf64_header(DumpState *s, Error **errp)
{
+ /*
+ * phnum in the elf header is 16 bit, if we have more segments we
+ * set phnum to PN_XNUM and write the real number of segments to a
+ * special section.
+ */
+ uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
Elf64_Ehdr elf_header;
int ret;
@@ -138,9 +144,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 +161,12 @@ static void write_elf64_header(DumpState *s, Error **errp)
static void write_elf32_header(DumpState *s, Error **errp)
{
+ /*
+ * phnum in the elf header is 16 bit, if we have more segments we
+ * set phnum to PN_XNUM and write the real number of segments to a
+ * special section.
+ */
+ uint16_t phnum = MIN(s->phdr_num, PN_XNUM);
Elf32_Ehdr elf_header;
int ret;
@@ -169,9 +181,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 +370,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 +490,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 +507,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 +1806,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 +1829,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] 23+ messages in thread
end of thread, other threads:[~2022-04-07 9:50 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
2022-03-31 8:59 ` Marc-André Lureau
2022-03-31 9:48 ` Janosch Frank
2022-03-31 10:11 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 2/9] dump: Remove the sh_info variable Janosch Frank
2022-03-31 8:58 ` Marc-André Lureau
2022-03-31 9:47 ` Janosch Frank
2022-03-31 13:49 ` Marc-André Lureau
2022-04-07 9:48 ` [PATCH v4] " Janosch Frank
2022-03-30 12:35 ` [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 5/9] dump: Add more offset variables Janosch Frank
2022-03-30 12:36 ` [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 7/9] dump: Consolidate phdr note writes Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 8/9] dump: Cleanup dump_begin write functions Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 9/9] dump: Consolidate elf note function Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).