* [PATCH 1/7] dump: Introduce shdr_num to decrease complexity
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-01 14:42 ` Marc-André Lureau
2022-03-01 14:22 ` [PATCH 2/7] dump: Remove the sh_info variable Janosch Frank
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
Let's move from a boolean to a int variable which will later enable us
to store the number of sections that are in the dump file.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 43 ++++++++++++++++++-------------------------
include/sysemu/dump.h | 2 +-
2 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index a84d8b1598..6696d9819a 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error **errp)
elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
- if (s->have_section) {
+ if (s->shdr_num) {
uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->sh_info;
elf_header.e_shoff = cpu_to_dump64(s, shoff);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
- elf_header.e_shnum = cpu_to_dump16(s, 1);
+ elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
}
ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error **errp)
elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
- if (s->have_section) {
+ if (s->shdr_num) {
uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->sh_info;
elf_header.e_shoff = cpu_to_dump32(s, shoff);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
- elf_header.e_shnum = cpu_to_dump16(s, 1);
+ elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
}
ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
@@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
uint32_t max_index;
Error *local_err = NULL;
- if (s->have_section) {
+ if (s->shdr_num) {
max_index = s->sh_info;
} else {
max_index = s->phdr_num;
@@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
}
/* write section to vmcore */
- if (s->have_section) {
+ if (s->shdr_num) {
write_elf_section(s, 1, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
}
/* write section to vmcore */
- if (s->have_section) {
+ if (s->shdr_num) {
write_elf_section(s, 0, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool has_format,
*/
s->phdr_num = 1; /* PT_NOTE */
if (s->list.num < UINT16_MAX - 2) {
+ s->shdr_num = 0;
s->phdr_num += s->list.num;
- s->have_section = false;
} else {
- s->have_section = true;
+ /* sh_info of section 0 holds the real number of phdrs */
s->phdr_num = PN_XNUM;
+ s->shdr_num = 1;
s->sh_info = 1; /* PT_NOTE */
/* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
@@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
}
if (s->dump_info.d_class == ELFCLASS64) {
- if (s->have_section) {
- s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->sh_info +
- sizeof(Elf64_Shdr) + s->note_size;
- } else {
- s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->phdr_num + s->note_size;
- }
+ s->memory_offset = sizeof(Elf64_Ehdr) +
+ sizeof(Elf64_Phdr) * s->sh_info +
+ sizeof(Elf64_Shdr) * s->shdr_num +
+ s->note_size;
} else {
- if (s->have_section) {
- s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->sh_info +
- sizeof(Elf32_Shdr) + s->note_size;
- } else {
- s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->phdr_num + s->note_size;
- }
+ s->memory_offset = sizeof(Elf32_Ehdr) +
+ sizeof(Elf32_Phdr) * s->sh_info +
+ sizeof(Elf32_Shdr) * s->shdr_num +
+ s->note_size;
}
return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 250143cb5a..854341da0d 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -155,8 +155,8 @@ typedef struct DumpState {
ArchDumpInfo dump_info;
MemoryMappingList list;
uint16_t phdr_num;
+ uint32_t shdr_num;
uint32_t sh_info;
- bool have_section;
bool resume;
bool detached;
ssize_t note_size;
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/7] dump: Introduce shdr_num to decrease complexity
2022-03-01 14:22 ` [PATCH 1/7] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-01 14:42 ` Marc-André Lureau
2022-03-08 13:16 ` Janosch Frank
0 siblings, 1 reply; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-01 14:42 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, QEMU
[-- Attachment #1: Type: text/plain, Size: 5823 bytes --]
Hi
On Tue, Mar 1, 2022 at 6:33 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> Let's move from a boolean to a int variable which will later enable us
> to store the number of sections that are in the dump file.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> dump/dump.c | 43 ++++++++++++++++++-------------------------
> include/sysemu/dump.h | 2 +-
> 2 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a84d8b1598..6696d9819a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error
> **errp)
> elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> - if (s->have_section) {
> + if (s->shdr_num) {
> uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> s->sh_info;
>
> elf_header.e_shoff = cpu_to_dump64(s, shoff);
> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> - elf_header.e_shnum = cpu_to_dump16(s, 1);
> + elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> }
>
> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> @@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error
> **errp)
> elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> - if (s->have_section) {
> + if (s->shdr_num) {
> uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> s->sh_info;
>
> elf_header.e_shoff = cpu_to_dump32(s, shoff);
> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> - elf_header.e_shnum = cpu_to_dump16(s, 1);
> + elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> }
>
> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> @@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
> uint32_t max_index;
> Error *local_err = NULL;
>
> - if (s->have_section) {
> + if (s->shdr_num) {
> max_index = s->sh_info;
> } else {
> max_index = s->phdr_num;
> @@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
> }
>
> /* write section to vmcore */
> - if (s->have_section) {
> + if (s->shdr_num) {
> write_elf_section(s, 1, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
> }
>
> /* write section to vmcore */
> - if (s->have_section) {
> + if (s->shdr_num) {
> write_elf_section(s, 0, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> */
> s->phdr_num = 1; /* PT_NOTE */
> if (s->list.num < UINT16_MAX - 2) {
> + s->shdr_num = 0;
> s->phdr_num += s->list.num;
> - s->have_section = false;
> } else {
> - s->have_section = true;
> + /* sh_info of section 0 holds the real number of phdrs */
>
...
> s->phdr_num = PN_XNUM;
> + s->shdr_num = 1;
> s->sh_info = 1; /* PT_NOTE */
>
> /* the type of shdr->sh_info is uint32_t, so we should avoid
> overflow */
> @@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> }
>
> if (s->dump_info.d_class == ELFCLASS64) {
> - if (s->have_section) {
> - s->memory_offset = sizeof(Elf64_Ehdr) +
> - sizeof(Elf64_Phdr) * s->sh_info +
> - sizeof(Elf64_Shdr) + s->note_size;
> - } else {
> - s->memory_offset = sizeof(Elf64_Ehdr) +
> - sizeof(Elf64_Phdr) * s->phdr_num +
> s->note_size;
> - }
> + s->memory_offset = sizeof(Elf64_Ehdr) +
> + sizeof(Elf64_Phdr) * s->sh_info +
>
The change "removing/replacing sizeof(Elf64_Phdr) * s->phdr_num by *
s->sh_info" should be done as a preliminary step, with more comments.
+ sizeof(Elf64_Shdr) * s->shdr_num +
> + s->note_size;
> } else {
> - if (s->have_section) {
> - s->memory_offset = sizeof(Elf32_Ehdr) +
> - sizeof(Elf32_Phdr) * s->sh_info +
> - sizeof(Elf32_Shdr) + s->note_size;
> - } else {
> - s->memory_offset = sizeof(Elf32_Ehdr) +
> - sizeof(Elf32_Phdr) * s->phdr_num +
> s->note_size;
> - }
> + s->memory_offset = sizeof(Elf32_Ehdr) +
> + sizeof(Elf32_Phdr) * s->sh_info +
> + sizeof(Elf32_Shdr) * s->shdr_num +
> + s->note_size;
> }
>
> return;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 250143cb5a..854341da0d 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -155,8 +155,8 @@ typedef struct DumpState {
> ArchDumpInfo dump_info;
> MemoryMappingList list;
> uint16_t phdr_num;
> + uint32_t shdr_num;
> uint32_t sh_info;
> - bool have_section;
> bool resume;
> bool detached;
> ssize_t note_size;
> --
> 2.32.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 7571 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/7] dump: Introduce shdr_num to decrease complexity
2022-03-01 14:42 ` Marc-André Lureau
@ 2022-03-08 13:16 ` Janosch Frank
0 siblings, 0 replies; 17+ messages in thread
From: Janosch Frank @ 2022-03-08 13:16 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On 3/1/22 15:42, Marc-André Lureau wrote:
> Hi
>
> On Tue, Mar 1, 2022 at 6:33 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> Let's move from a boolean to a int variable which will later enable us
>> to store the number of sections that are in the dump file.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> dump/dump.c | 43 ++++++++++++++++++-------------------------
>> include/sysemu/dump.h | 2 +-
>> 2 files changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index a84d8b1598..6696d9819a 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -139,12 +139,12 @@ static void write_elf64_header(DumpState *s, Error
>> **errp)
>> elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
>> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
>> elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
>> - if (s->have_section) {
>> + if (s->shdr_num) {
>> uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
>> s->sh_info;
>>
>> elf_header.e_shoff = cpu_to_dump64(s, shoff);
>> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
>> - elf_header.e_shnum = cpu_to_dump16(s, 1);
>> + elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>> }
>>
>> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
>> @@ -170,12 +170,12 @@ static void write_elf32_header(DumpState *s, Error
>> **errp)
>> elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
>> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
>> elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
>> - if (s->have_section) {
>> + if (s->shdr_num) {
>> uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
>> s->sh_info;
>>
>> elf_header.e_shoff = cpu_to_dump32(s, shoff);
>> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
>> - elf_header.e_shnum = cpu_to_dump16(s, 1);
>> + elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
>> }
>>
>> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
>> @@ -482,7 +482,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
>> uint32_t max_index;
>> Error *local_err = NULL;
>>
>> - if (s->have_section) {
>> + if (s->shdr_num) {
>> max_index = s->sh_info;
>> } else {
>> max_index = s->phdr_num;
>> @@ -567,7 +567,7 @@ static void dump_begin(DumpState *s, Error **errp)
>> }
>>
>> /* write section to vmcore */
>> - if (s->have_section) {
>> + if (s->shdr_num) {
>> write_elf_section(s, 1, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> @@ -597,7 +597,7 @@ static void dump_begin(DumpState *s, Error **errp)
>> }
>>
>> /* write section to vmcore */
>> - if (s->have_section) {
>> + if (s->shdr_num) {
>> write_elf_section(s, 0, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> @@ -1818,11 +1818,12 @@ static void dump_init(DumpState *s, int fd, bool
>> has_format,
>> */
>> s->phdr_num = 1; /* PT_NOTE */
>> if (s->list.num < UINT16_MAX - 2) {
>> + s->shdr_num = 0;
>> s->phdr_num += s->list.num;
>> - s->have_section = false;
>> } else {
>> - s->have_section = true;
>> + /* sh_info of section 0 holds the real number of phdrs */
>>
>
> ...
>
>
>> s->phdr_num = PN_XNUM;
>> + s->shdr_num = 1;
>> s->sh_info = 1; /* PT_NOTE */
>>
>> /* the type of shdr->sh_info is uint32_t, so we should avoid
>> overflow */
>> @@ -1834,23 +1835,15 @@ static void dump_init(DumpState *s, int fd, bool
>> has_format,
>> }
>>
>> if (s->dump_info.d_class == ELFCLASS64) {
>> - if (s->have_section) {
>> - s->memory_offset = sizeof(Elf64_Ehdr) +
>> - sizeof(Elf64_Phdr) * s->sh_info +
>> - sizeof(Elf64_Shdr) + s->note_size;
>> - } else {
>> - s->memory_offset = sizeof(Elf64_Ehdr) +
>> - sizeof(Elf64_Phdr) * s->phdr_num +
>> s->note_size;
>> - }
>> + s->memory_offset = sizeof(Elf64_Ehdr) +
>> + sizeof(Elf64_Phdr) * s->sh_info +
>>
>
> The change "removing/replacing sizeof(Elf64_Phdr) * s->phdr_num by *
> s->sh_info" should be done as a preliminary step, with more comments.
I had another look at this and this patch won't work without the next
patch anyway. I'll try to recombine and split the patches in a way that
makes this easier to read.
>
> + sizeof(Elf64_Shdr) * s->shdr_num +
>> + s->note_size;
>> } else {
>> - if (s->have_section) {
>> - s->memory_offset = sizeof(Elf32_Ehdr) +
>> - sizeof(Elf32_Phdr) * s->sh_info +
>> - sizeof(Elf32_Shdr) + s->note_size;
>> - } else {
>> - s->memory_offset = sizeof(Elf32_Ehdr) +
>> - sizeof(Elf32_Phdr) * s->phdr_num +
>> s->note_size;
>> - }
>> + s->memory_offset = sizeof(Elf32_Ehdr) +
>> + sizeof(Elf32_Phdr) * s->sh_info +
>> + sizeof(Elf32_Shdr) * s->shdr_num +
>> + s->note_size;
>> }
>>
>> return;
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 250143cb5a..854341da0d 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -155,8 +155,8 @@ typedef struct DumpState {
>> ArchDumpInfo dump_info;
>> MemoryMappingList list;
>> uint16_t phdr_num;
>> + uint32_t shdr_num;
>> uint32_t sh_info;
>> - bool have_section;
>> bool resume;
>> bool detached;
>> ssize_t note_size;
>> --
>> 2.32.0
>>
>>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/7] dump: Remove the sh_info variable
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
2022-03-01 14:22 ` [PATCH 1/7] dump: Introduce shdr_num to decrease complexity Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-01 14:22 ` [PATCH 3/7] dump: Add more offset variables Janosch Frank
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
There's no need to have phdr_num and sh_info at the same time. We can
make phdr_num 32 bit and set PN_XNUM when we write the header if
phdr_num >= PN_XNUM.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 33 +++++++++++++--------------------
include/sysemu/dump.h | 3 +--
2 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 6696d9819a..ce3a5e7003 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -124,6 +124,7 @@ static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
static void write_elf64_header(DumpState *s, Error **errp)
{
+ uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
Elf64_Ehdr elf_header;
int ret;
@@ -138,9 +139,9 @@ static void write_elf64_header(DumpState *s, Error **errp)
elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
- elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
+ elf_header.e_phnum = cpu_to_dump16(s, phnum);
if (s->shdr_num) {
- uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->sh_info;
+ uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
elf_header.e_shoff = cpu_to_dump64(s, shoff);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
@@ -155,6 +156,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
static void write_elf32_header(DumpState *s, Error **errp)
{
+ uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
Elf32_Ehdr elf_header;
int ret;
@@ -169,9 +171,9 @@ static void write_elf32_header(DumpState *s, Error **errp)
elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
- elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
+ elf_header.e_phnum = cpu_to_dump16(s, phnum);
if (s->shdr_num) {
- uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->sh_info;
+ uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
elf_header.e_shoff = cpu_to_dump32(s, shoff);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
@@ -358,12 +360,12 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
if (type == 0) {
shdr_size = sizeof(Elf32_Shdr);
memset(&shdr32, 0, shdr_size);
- shdr32.sh_info = cpu_to_dump32(s, s->sh_info);
+ shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
shdr = &shdr32;
} else {
shdr_size = sizeof(Elf64_Shdr);
memset(&shdr64, 0, shdr_size);
- shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
+ shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
shdr = &shdr64;
}
@@ -479,15 +481,8 @@ static void write_elf_loads(DumpState *s, Error **errp)
hwaddr offset, filesz;
MemoryMapping *memory_mapping;
uint32_t phdr_index = 1;
- uint32_t max_index;
Error *local_err = NULL;
- if (s->shdr_num) {
- max_index = s->sh_info;
- } else {
- max_index = s->phdr_num;
- }
-
QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
get_offset_range(memory_mapping->phys_addr,
memory_mapping->length,
@@ -505,7 +500,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
return;
}
- if (phdr_index >= max_index) {
+ if (phdr_index >= s->phdr_num) {
break;
}
}
@@ -1822,26 +1817,24 @@ static void dump_init(DumpState *s, int fd, bool has_format,
s->phdr_num += s->list.num;
} else {
/* sh_info of section 0 holds the real number of phdrs */
- s->phdr_num = PN_XNUM;
s->shdr_num = 1;
- s->sh_info = 1; /* PT_NOTE */
/* the type of shdr->sh_info is uint32_t, so we should avoid overflow */
if (s->list.num <= UINT32_MAX - 1) {
- s->sh_info += s->list.num;
+ s->phdr_num += s->list.num;
} else {
- s->sh_info = UINT32_MAX;
+ s->phdr_num = UINT32_MAX;
}
}
if (s->dump_info.d_class == ELFCLASS64) {
s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->sh_info +
+ sizeof(Elf64_Phdr) * s->phdr_num +
sizeof(Elf64_Shdr) * s->shdr_num +
s->note_size;
} else {
s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->sh_info +
+ sizeof(Elf32_Phdr) * s->phdr_num +
sizeof(Elf32_Shdr) * s->shdr_num +
s->note_size;
}
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 854341da0d..19458bffbd 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -154,9 +154,8 @@ typedef struct DumpState {
GuestPhysBlockList guest_phys_blocks;
ArchDumpInfo dump_info;
MemoryMappingList list;
- uint16_t phdr_num;
+ uint32_t phdr_num;
uint32_t shdr_num;
- uint32_t sh_info;
bool resume;
bool detached;
ssize_t note_size;
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/7] dump: Add more offset variables
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
2022-03-01 14:22 ` [PATCH 1/7] dump: Introduce shdr_num to decrease complexity Janosch Frank
2022-03-01 14:22 ` [PATCH 2/7] dump: Remove the sh_info variable Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-02 10:20 ` Marc-André Lureau
2022-03-01 14:22 ` [PATCH 4/7] dump: Introduce dump_is_64bit() helper function Janosch Frank
` (3 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
Offset calculations are easy enough to get wrong. Let's add a few
variables to make moving around elf headers and data sections easier.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 34 ++++++++++++++--------------------
include/sysemu/dump.h | 4 ++++
2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index ce3a5e7003..242f83db95 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -137,13 +137,11 @@ static void write_elf64_header(DumpState *s, Error **errp)
elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
- elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
+ elf_header.e_phoff = cpu_to_dump64(s, s->phdr_offset);
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
elf_header.e_phnum = cpu_to_dump16(s, phnum);
if (s->shdr_num) {
- uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
-
- elf_header.e_shoff = cpu_to_dump64(s, shoff);
+ elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
}
@@ -169,13 +167,11 @@ static void write_elf32_header(DumpState *s, Error **errp)
elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
- elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
+ elf_header.e_phoff = cpu_to_dump32(s, s->phdr_offset);
elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
elf_header.e_phnum = cpu_to_dump16(s, phnum);
if (s->shdr_num) {
- uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
-
- elf_header.e_shoff = cpu_to_dump32(s, shoff);
+ elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
}
@@ -238,12 +234,11 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
static void write_elf64_note(DumpState *s, Error **errp)
{
Elf64_Phdr phdr;
- hwaddr begin = s->memory_offset - s->note_size;
int ret;
memset(&phdr, 0, sizeof(Elf64_Phdr));
phdr.p_type = cpu_to_dump32(s, PT_NOTE);
- phdr.p_offset = cpu_to_dump64(s, begin);
+ phdr.p_offset = cpu_to_dump64(s, s->note_offset);
phdr.p_paddr = 0;
phdr.p_filesz = cpu_to_dump64(s, s->note_size);
phdr.p_memsz = cpu_to_dump64(s, s->note_size);
@@ -303,13 +298,12 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
static void write_elf32_note(DumpState *s, Error **errp)
{
- hwaddr begin = s->memory_offset - s->note_size;
Elf32_Phdr phdr;
int ret;
memset(&phdr, 0, sizeof(Elf32_Phdr));
phdr.p_type = cpu_to_dump32(s, PT_NOTE);
- phdr.p_offset = cpu_to_dump32(s, begin);
+ phdr.p_offset = cpu_to_dump32(s, s->note_offset);
phdr.p_paddr = 0;
phdr.p_filesz = cpu_to_dump32(s, s->note_size);
phdr.p_memsz = cpu_to_dump32(s, s->note_size);
@@ -1828,15 +1822,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
}
if (s->dump_info.d_class == ELFCLASS64) {
- s->memory_offset = sizeof(Elf64_Ehdr) +
- sizeof(Elf64_Phdr) * s->phdr_num +
- sizeof(Elf64_Shdr) * s->shdr_num +
- s->note_size;
+ s->phdr_offset = sizeof(Elf64_Ehdr);
+ s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
+ s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
+ s->memory_offset = s->note_offset + s->note_size;
} else {
- s->memory_offset = sizeof(Elf32_Ehdr) +
- sizeof(Elf32_Phdr) * s->phdr_num +
- sizeof(Elf32_Shdr) * s->shdr_num +
- s->note_size;
+ s->phdr_offset = sizeof(Elf32_Ehdr);
+ s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
+ s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
+ s->memory_offset = s->note_offset + s->note_size;
}
return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 19458bffbd..ffc2ea1072 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -159,6 +159,10 @@ typedef struct DumpState {
bool resume;
bool detached;
ssize_t note_size;
+ hwaddr shdr_offset;
+ hwaddr phdr_offset;
+ hwaddr section_offset;
+ hwaddr note_offset;
hwaddr memory_offset;
int fd;
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/7] dump: Add more offset variables
2022-03-01 14:22 ` [PATCH 3/7] dump: Add more offset variables Janosch Frank
@ 2022-03-02 10:20 ` Marc-André Lureau
0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-02 10:20 UTC (permalink / raw)
To: Janosch Frank; +Cc: Bonzini, Paolo, qemu-devel
On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Offset calculations are easy enough to get wrong. Let's add a few
> variables to make moving around elf headers and data sections easier.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump/dump.c | 34 ++++++++++++++--------------------
> include/sysemu/dump.h | 4 ++++
> 2 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index ce3a5e7003..242f83db95 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -137,13 +137,11 @@ static void write_elf64_header(DumpState *s, Error **errp)
> elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
> elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
> elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> - elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
> + elf_header.e_phoff = cpu_to_dump64(s, s->phdr_offset);
> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> elf_header.e_phnum = cpu_to_dump16(s, phnum);
> if (s->shdr_num) {
> - uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) * s->phdr_num;
> -
> - elf_header.e_shoff = cpu_to_dump64(s, shoff);
> + elf_header.e_shoff = cpu_to_dump64(s, s->shdr_offset);
> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> }
> @@ -169,13 +167,11 @@ static void write_elf32_header(DumpState *s, Error **errp)
> elf_header.e_machine = cpu_to_dump16(s, s->dump_info.d_machine);
> elf_header.e_version = cpu_to_dump32(s, EV_CURRENT);
> elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> - elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
> + elf_header.e_phoff = cpu_to_dump32(s, s->phdr_offset);
> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> elf_header.e_phnum = cpu_to_dump16(s, phnum);
> if (s->shdr_num) {
> - uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) * s->phdr_num;
> -
> - elf_header.e_shoff = cpu_to_dump32(s, shoff);
> + elf_header.e_shoff = cpu_to_dump32(s, s->shdr_offset);
> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> elf_header.e_shnum = cpu_to_dump16(s, s->shdr_num);
> }
> @@ -238,12 +234,11 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
> static void write_elf64_note(DumpState *s, Error **errp)
> {
> Elf64_Phdr phdr;
> - hwaddr begin = s->memory_offset - s->note_size;
> int ret;
>
> memset(&phdr, 0, sizeof(Elf64_Phdr));
> phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> - phdr.p_offset = cpu_to_dump64(s, begin);
> + phdr.p_offset = cpu_to_dump64(s, s->note_offset);
> phdr.p_paddr = 0;
> phdr.p_filesz = cpu_to_dump64(s, s->note_size);
> phdr.p_memsz = cpu_to_dump64(s, s->note_size);
> @@ -303,13 +298,12 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>
> static void write_elf32_note(DumpState *s, Error **errp)
> {
> - hwaddr begin = s->memory_offset - s->note_size;
> Elf32_Phdr phdr;
> int ret;
>
> memset(&phdr, 0, sizeof(Elf32_Phdr));
> phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> - phdr.p_offset = cpu_to_dump32(s, begin);
> + phdr.p_offset = cpu_to_dump32(s, s->note_offset);
> phdr.p_paddr = 0;
> phdr.p_filesz = cpu_to_dump32(s, s->note_size);
> phdr.p_memsz = cpu_to_dump32(s, s->note_size);
> @@ -1828,15 +1822,15 @@ static void dump_init(DumpState *s, int fd, bool has_format,
> }
>
> if (s->dump_info.d_class == ELFCLASS64) {
> - s->memory_offset = sizeof(Elf64_Ehdr) +
> - sizeof(Elf64_Phdr) * s->phdr_num +
> - sizeof(Elf64_Shdr) * s->shdr_num +
> - s->note_size;
> + s->phdr_offset = sizeof(Elf64_Ehdr);
> + s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
> + s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
> + s->memory_offset = s->note_offset + s->note_size;
> } else {
> - s->memory_offset = sizeof(Elf32_Ehdr) +
> - sizeof(Elf32_Phdr) * s->phdr_num +
> - sizeof(Elf32_Shdr) * s->shdr_num +
> - s->note_size;
> + s->phdr_offset = sizeof(Elf32_Ehdr);
> + s->shdr_offset = s->phdr_offset + sizeof(Elf32_Phdr) * s->phdr_num;
> + s->note_offset = s->shdr_offset + sizeof(Elf32_Shdr) * s->shdr_num;
> + s->memory_offset = s->note_offset + s->note_size;
> }
>
> return;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 19458bffbd..ffc2ea1072 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -159,6 +159,10 @@ typedef struct DumpState {
> bool resume;
> bool detached;
> ssize_t note_size;
> + hwaddr shdr_offset;
> + hwaddr phdr_offset;
> + hwaddr section_offset;
> + hwaddr note_offset;
> hwaddr memory_offset;
> int fd;
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/7] dump: Introduce dump_is_64bit() helper function
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
` (2 preceding siblings ...)
2022-03-01 14:22 ` [PATCH 3/7] dump: Add more offset variables Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-01 14:33 ` Marc-André Lureau
2022-03-01 14:22 ` [PATCH 5/7] dump: Consolidate phdr note writes Janosch Frank
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
Checking d_class in dump_info leads to lengthy conditionals so let's
shorten things a bit by introducing a helper function.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 14 +++++++-------
include/sysemu/dump.h | 6 ++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 242f83db95..bb152bddff 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -481,7 +481,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
get_offset_range(memory_mapping->phys_addr,
memory_mapping->length,
s, &offset, &filesz);
- if (s->dump_info.d_class == ELFCLASS64) {
+ if (dump_is_64bit(s)) {
write_elf64_load(s, memory_mapping, phdr_index++, offset,
filesz, &local_err);
} else {
@@ -530,7 +530,7 @@ static void dump_begin(DumpState *s, Error **errp)
*/
/* write elf header to vmcore */
- if (s->dump_info.d_class == ELFCLASS64) {
+ if (dump_is_64bit(s)) {
write_elf64_header(s, &local_err);
} else {
write_elf32_header(s, &local_err);
@@ -540,7 +540,7 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
- if (s->dump_info.d_class == ELFCLASS64) {
+ if (dump_is_64bit(s)) {
/* write PT_NOTE to vmcore */
write_elf64_note(s, &local_err);
if (local_err) {
@@ -761,7 +761,7 @@ static void get_note_sizes(DumpState *s, const void *note,
uint64_t name_sz;
uint64_t desc_sz;
- if (s->dump_info.d_class == ELFCLASS64) {
+ if (dump_is_64bit(s)) {
const Elf64_Nhdr *hdr = note;
note_head_sz = sizeof(Elf64_Nhdr);
name_sz = tswap64(hdr->n_namesz);
@@ -1023,7 +1023,7 @@ out:
static void write_dump_header(DumpState *s, Error **errp)
{
- if (s->dump_info.d_class == ELFCLASS32) {
+ if (!dump_is_64bit(s)) {
create_header32(s, errp);
} else {
create_header64(s, errp);
@@ -1716,7 +1716,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
uint32_t size;
uint16_t format;
- note_head_size = s->dump_info.d_class == ELFCLASS32 ?
+ note_head_size = !dump_is_64bit(s) ?
sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
@@ -1821,7 +1821,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
}
}
- if (s->dump_info.d_class == ELFCLASS64) {
+ if (dump_is_64bit(s)) {
s->phdr_offset = sizeof(Elf64_Ehdr);
s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) * s->phdr_num;
s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index ffc2ea1072..078b3d57a1 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -203,4 +203,10 @@ typedef struct DumpState {
uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
+
+static inline bool dump_is_64bit(DumpState *s)
+{
+ return s->dump_info.d_class == ELFCLASS64;
+}
+
#endif
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/7] dump: Introduce dump_is_64bit() helper function
2022-03-01 14:22 ` [PATCH 4/7] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-01 14:33 ` Marc-André Lureau
0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-01 14:33 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, QEMU
[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]
Hi
On Tue, Mar 1, 2022 at 6:30 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> Checking d_class in dump_info leads to lengthy conditionals so let's
> shorten things a bit by introducing a helper function.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> dump/dump.c | 14 +++++++-------
> include/sysemu/dump.h | 6 ++++++
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 242f83db95..bb152bddff 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -481,7 +481,7 @@ static void write_elf_loads(DumpState *s, Error **errp)
> get_offset_range(memory_mapping->phys_addr,
> memory_mapping->length,
> s, &offset, &filesz);
> - if (s->dump_info.d_class == ELFCLASS64) {
> + if (dump_is_64bit(s)) {
> write_elf64_load(s, memory_mapping, phdr_index++, offset,
> filesz, &local_err);
> } else {
> @@ -530,7 +530,7 @@ static void dump_begin(DumpState *s, Error **errp)
> */
>
> /* write elf header to vmcore */
> - if (s->dump_info.d_class == ELFCLASS64) {
> + if (dump_is_64bit(s)) {
> write_elf64_header(s, &local_err);
> } else {
> write_elf32_header(s, &local_err);
> @@ -540,7 +540,7 @@ static void dump_begin(DumpState *s, Error **errp)
> return;
> }
>
> - if (s->dump_info.d_class == ELFCLASS64) {
> + if (dump_is_64bit(s)) {
> /* write PT_NOTE to vmcore */
> write_elf64_note(s, &local_err);
> if (local_err) {
> @@ -761,7 +761,7 @@ static void get_note_sizes(DumpState *s, const void
> *note,
> uint64_t name_sz;
> uint64_t desc_sz;
>
> - if (s->dump_info.d_class == ELFCLASS64) {
> + if (dump_is_64bit(s)) {
> const Elf64_Nhdr *hdr = note;
> note_head_sz = sizeof(Elf64_Nhdr);
> name_sz = tswap64(hdr->n_namesz);
> @@ -1023,7 +1023,7 @@ out:
>
> static void write_dump_header(DumpState *s, Error **errp)
> {
> - if (s->dump_info.d_class == ELFCLASS32) {
> + if (!dump_is_64bit(s)) {
> create_header32(s, errp);
> } else {
> create_header64(s, errp);
> @@ -1716,7 +1716,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> uint32_t size;
> uint16_t format;
>
> - note_head_size = s->dump_info.d_class == ELFCLASS32 ?
> + note_head_size = !dump_is_64bit(s) ?
> sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
>
> format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> @@ -1821,7 +1821,7 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> }
> }
>
> - if (s->dump_info.d_class == ELFCLASS64) {
> + if (dump_is_64bit(s)) {
> s->phdr_offset = sizeof(Elf64_Ehdr);
> s->shdr_offset = s->phdr_offset + sizeof(Elf64_Phdr) *
> s->phdr_num;
> s->note_offset = s->shdr_offset + sizeof(Elf64_Shdr) *
> s->shdr_num;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..078b3d57a1 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -203,4 +203,10 @@ typedef struct DumpState {
> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
> uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
> +
> +static inline bool dump_is_64bit(DumpState *s)
> +{
> + return s->dump_info.d_class == ELFCLASS64;
> +}
>
Since it's not used outside of dump.c (so far), I'd keep this as a regular
static function there. Otherwise, lgtm.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4683 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/7] dump: Consolidate phdr note writes
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
` (3 preceding siblings ...)
2022-03-01 14:22 ` [PATCH 4/7] dump: Introduce dump_is_64bit() helper function Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-01 14:30 ` Marc-André Lureau
2022-03-01 14:22 ` [PATCH 6/7] dump: Cleanup dump_begin write functions Janosch Frank
2022-03-01 14:22 ` [PATCH 7/7] dump: Consolidate elf note function Janosch Frank
6 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
There's no need to have two write functions. Let's rather have two
functions that set the data for elf 32/64 and then write it in a
common function.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index bb152bddff..88c2dbb466 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
}
}
-static void write_elf64_note(DumpState *s, Error **errp)
+static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
{
- Elf64_Phdr phdr;
- int ret;
-
- memset(&phdr, 0, sizeof(Elf64_Phdr));
- phdr.p_type = cpu_to_dump32(s, PT_NOTE);
- phdr.p_offset = cpu_to_dump64(s, s->note_offset);
- phdr.p_paddr = 0;
- phdr.p_filesz = cpu_to_dump64(s, s->note_size);
- phdr.p_memsz = cpu_to_dump64(s, s->note_size);
- phdr.p_vaddr = 0;
-
- ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
- if (ret < 0) {
- error_setg_errno(errp, -ret,
- "dump: failed to write program header table");
- }
+ memset(phdr, 0, sizeof(*phdr));
+ phdr->p_type = cpu_to_dump32(s, PT_NOTE);
+ phdr->p_offset = cpu_to_dump64(s, s->note_offset);
+ phdr->p_paddr = 0;
+ phdr->p_filesz = cpu_to_dump64(s, s->note_size);
+ phdr->p_memsz = cpu_to_dump64(s, s->note_size);
+ phdr->p_vaddr = 0;
}
static inline int cpu_index(CPUState *cpu)
@@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
write_guest_note(f, s, errp);
}
-static void write_elf32_note(DumpState *s, Error **errp)
+static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
{
- Elf32_Phdr phdr;
- int ret;
-
- memset(&phdr, 0, sizeof(Elf32_Phdr));
- phdr.p_type = cpu_to_dump32(s, PT_NOTE);
- phdr.p_offset = cpu_to_dump32(s, s->note_offset);
- phdr.p_paddr = 0;
- phdr.p_filesz = cpu_to_dump32(s, s->note_size);
- phdr.p_memsz = cpu_to_dump32(s, s->note_size);
- phdr.p_vaddr = 0;
-
- ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
- if (ret < 0) {
- error_setg_errno(errp, -ret,
- "dump: failed to write program header table");
- }
+ memset(phdr, 0, sizeof(*phdr));
+ phdr->p_type = cpu_to_dump32(s, PT_NOTE);
+ phdr->p_offset = cpu_to_dump32(s, s->note_offset);
+ phdr->p_paddr = 0;
+ phdr->p_filesz = cpu_to_dump32(s, s->note_size);
+ phdr->p_memsz = cpu_to_dump32(s, s->note_size);
+ phdr->p_vaddr = 0;
}
static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
@@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
write_guest_note(f, s, errp);
}
+static void write_elf_phdr_note(DumpState *s, Error **errp)
+{
+ Elf32_Phdr phdr32;
+ Elf64_Phdr phdr64;
+ void *phdr;
+ size_t size;
+ int ret;
+
+ if (dump_is_64bit(s)) {
+ write_elf64_phdr_note(s, &phdr64);
+ size = sizeof(phdr64);
+ phdr = &phdr64;
+ } else {
+ write_elf32_phdr_note(s, &phdr32);
+ size = sizeof(phdr32);
+ phdr = &phdr32;
+ }
+
+ ret = fd_write_vmcore(phdr, size, s);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret,
+ "dump: failed to write program header table");
+ }
+}
+
static void write_elf_section(DumpState *s, int type, Error **errp)
{
Elf32_Shdr shdr32;
@@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
- if (dump_is_64bit(s)) {
- /* write PT_NOTE to vmcore */
- write_elf64_note(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ /* write PT_NOTE to vmcore */
+ write_elf_phdr_note(s, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ if (dump_is_64bit(s)) {
/* write all PT_LOAD to vmcore */
write_elf_loads(s, &local_err);
if (local_err) {
@@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
} else {
- /* write PT_NOTE to vmcore */
- write_elf32_note(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
/* write all PT_LOAD to vmcore */
write_elf_loads(s, &local_err);
if (local_err) {
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/7] dump: Consolidate phdr note writes
2022-03-01 14:22 ` [PATCH 5/7] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-01 14:30 ` Marc-André Lureau
2022-03-01 16:00 ` Janosch Frank
0 siblings, 1 reply; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-01 14:30 UTC (permalink / raw)
To: Janosch Frank; +Cc: Paolo Bonzini, QEMU
[-- Attachment #1: Type: text/plain, Size: 5283 bytes --]
Hi
On Tue, Mar 1, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> There's no need to have two write functions. Let's rather have two
> functions that set the data for elf 32/64 and then write it in a
> common function.
>
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------
1 file changed, 48 insertions(+), 48 deletions(-)
>
I suppose there are other parts of the series that make use of that,
because as such, it's not a clear improvement to me.
>
>
> diff --git a/dump/dump.c b/dump/dump.c
> index bb152bddff..88c2dbb466 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s,
> MemoryMapping *memory_mapping,
> }
> }
>
> -static void write_elf64_note(DumpState *s, Error **errp)
> +static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
> {
> - Elf64_Phdr phdr;
> - int ret;
> -
> - memset(&phdr, 0, sizeof(Elf64_Phdr));
> - phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> - phdr.p_offset = cpu_to_dump64(s, s->note_offset);
> - phdr.p_paddr = 0;
> - phdr.p_filesz = cpu_to_dump64(s, s->note_size);
> - phdr.p_memsz = cpu_to_dump64(s, s->note_size);
> - phdr.p_vaddr = 0;
> -
> - ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
> - if (ret < 0) {
> - error_setg_errno(errp, -ret,
> - "dump: failed to write program header table");
> - }
> + memset(phdr, 0, sizeof(*phdr));
> + phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> + phdr->p_offset = cpu_to_dump64(s, s->note_offset);
> + phdr->p_paddr = 0;
> + phdr->p_filesz = cpu_to_dump64(s, s->note_size);
> + phdr->p_memsz = cpu_to_dump64(s, s->note_size);
> + phdr->p_vaddr = 0;
> }
>
> static inline int cpu_index(CPUState *cpu)
> @@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction
> f, DumpState *s,
> write_guest_note(f, s, errp);
> }
>
> -static void write_elf32_note(DumpState *s, Error **errp)
> +static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
> {
> - Elf32_Phdr phdr;
> - int ret;
> -
> - memset(&phdr, 0, sizeof(Elf32_Phdr));
> - phdr.p_type = cpu_to_dump32(s, PT_NOTE);
> - phdr.p_offset = cpu_to_dump32(s, s->note_offset);
> - phdr.p_paddr = 0;
> - phdr.p_filesz = cpu_to_dump32(s, s->note_size);
> - phdr.p_memsz = cpu_to_dump32(s, s->note_size);
> - phdr.p_vaddr = 0;
> -
> - ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
> - if (ret < 0) {
> - error_setg_errno(errp, -ret,
> - "dump: failed to write program header table");
> - }
> + memset(phdr, 0, sizeof(*phdr));
> + phdr->p_type = cpu_to_dump32(s, PT_NOTE);
> + phdr->p_offset = cpu_to_dump32(s, s->note_offset);
> + phdr->p_paddr = 0;
> + phdr->p_filesz = cpu_to_dump32(s, s->note_size);
> + phdr->p_memsz = cpu_to_dump32(s, s->note_size);
> + phdr->p_vaddr = 0;
> }
>
> static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
> @@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction
> f, DumpState *s,
> write_guest_note(f, s, errp);
> }
>
> +static void write_elf_phdr_note(DumpState *s, Error **errp)
> +{
> + Elf32_Phdr phdr32;
> + Elf64_Phdr phdr64;
> + void *phdr;
> + size_t size;
> + int ret;
> +
> + if (dump_is_64bit(s)) {
> + write_elf64_phdr_note(s, &phdr64);
> + size = sizeof(phdr64);
> + phdr = &phdr64;
> + } else {
> + write_elf32_phdr_note(s, &phdr32);
> + size = sizeof(phdr32);
> + phdr = &phdr32;
> + }
> +
> + ret = fd_write_vmcore(phdr, size, s);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "dump: failed to write program header table");
> + }
> +}
> +
> static void write_elf_section(DumpState *s, int type, Error **errp)
> {
> Elf32_Shdr shdr32;
> @@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
> return;
> }
>
> - if (dump_is_64bit(s)) {
> - /* write PT_NOTE to vmcore */
> - write_elf64_note(s, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> + /* write PT_NOTE to vmcore */
> + write_elf_phdr_note(s, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
>
> + if (dump_is_64bit(s)) {
> /* write all PT_LOAD to vmcore */
> write_elf_loads(s, &local_err);
> if (local_err) {
> @@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
> return;
> }
> } else {
> - /* write PT_NOTE to vmcore */
> - write_elf32_note(s, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> -
> /* write all PT_LOAD to vmcore */
> write_elf_loads(s, &local_err);
> if (local_err) {
> --
> 2.32.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 7081 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/7] dump: Consolidate phdr note writes
2022-03-01 14:30 ` Marc-André Lureau
@ 2022-03-01 16:00 ` Janosch Frank
0 siblings, 0 replies; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 16:00 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU
On 3/1/22 15:30, Marc-André Lureau wrote:
> Hi
>
> On Tue, Mar 1, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> There's no need to have two write functions. Let's rather have two
>> functions that set the data for elf 32/64 and then write it in a
>> common function.
>>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> dump/dump.c | 96 ++++++++++++++++++++++++++---------------------------
>
> 1 file changed, 48 insertions(+), 48 deletions(-)
>>
>
> I suppose there are other parts of the series that make use of that,
> because as such, it's not a clear improvement to me.
It's one piece of the effort to get rid of that big if/else in
dump_begin() and that frees the way to make re-ordering the write
functions possible.
I'll send out the other two series tomorrow.
>
>>
>
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index bb152bddff..88c2dbb466 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -231,24 +231,15 @@ static void write_elf32_load(DumpState *s,
>> MemoryMapping *memory_mapping,
>> }
>> }
>>
>> -static void write_elf64_note(DumpState *s, Error **errp)
>> +static void write_elf64_phdr_note(DumpState *s, Elf64_Phdr *phdr)
>> {
>> - Elf64_Phdr phdr;
>> - int ret;
>> -
>> - memset(&phdr, 0, sizeof(Elf64_Phdr));
>> - phdr.p_type = cpu_to_dump32(s, PT_NOTE);
>> - phdr.p_offset = cpu_to_dump64(s, s->note_offset);
>> - phdr.p_paddr = 0;
>> - phdr.p_filesz = cpu_to_dump64(s, s->note_size);
>> - phdr.p_memsz = cpu_to_dump64(s, s->note_size);
>> - phdr.p_vaddr = 0;
>> -
>> - ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
>> - if (ret < 0) {
>> - error_setg_errno(errp, -ret,
>> - "dump: failed to write program header table");
>> - }
>> + memset(phdr, 0, sizeof(*phdr));
>> + phdr->p_type = cpu_to_dump32(s, PT_NOTE);
>> + phdr->p_offset = cpu_to_dump64(s, s->note_offset);
>> + phdr->p_paddr = 0;
>> + phdr->p_filesz = cpu_to_dump64(s, s->note_size);
>> + phdr->p_memsz = cpu_to_dump64(s, s->note_size);
>> + phdr->p_vaddr = 0;
>> }
>>
>> static inline int cpu_index(CPUState *cpu)
>> @@ -296,24 +287,15 @@ static void write_elf64_notes(WriteCoreDumpFunction
>> f, DumpState *s,
>> write_guest_note(f, s, errp);
>> }
>>
>> -static void write_elf32_note(DumpState *s, Error **errp)
>> +static void write_elf32_phdr_note(DumpState *s, Elf32_Phdr *phdr)
>> {
>> - Elf32_Phdr phdr;
>> - int ret;
>> -
>> - memset(&phdr, 0, sizeof(Elf32_Phdr));
>> - phdr.p_type = cpu_to_dump32(s, PT_NOTE);
>> - phdr.p_offset = cpu_to_dump32(s, s->note_offset);
>> - phdr.p_paddr = 0;
>> - phdr.p_filesz = cpu_to_dump32(s, s->note_size);
>> - phdr.p_memsz = cpu_to_dump32(s, s->note_size);
>> - phdr.p_vaddr = 0;
>> -
>> - ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
>> - if (ret < 0) {
>> - error_setg_errno(errp, -ret,
>> - "dump: failed to write program header table");
>> - }
>> + memset(phdr, 0, sizeof(*phdr));
>> + phdr->p_type = cpu_to_dump32(s, PT_NOTE);
>> + phdr->p_offset = cpu_to_dump32(s, s->note_offset);
>> + phdr->p_paddr = 0;
>> + phdr->p_filesz = cpu_to_dump32(s, s->note_size);
>> + phdr->p_memsz = cpu_to_dump32(s, s->note_size);
>> + phdr->p_vaddr = 0;
>> }
>>
>> static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
>> @@ -343,6 +325,31 @@ static void write_elf32_notes(WriteCoreDumpFunction
>> f, DumpState *s,
>> write_guest_note(f, s, errp);
>> }
>>
>> +static void write_elf_phdr_note(DumpState *s, Error **errp)
>> +{
>> + Elf32_Phdr phdr32;
>> + Elf64_Phdr phdr64;
>> + void *phdr;
>> + size_t size;
>> + int ret;
>> +
>> + if (dump_is_64bit(s)) {
>> + write_elf64_phdr_note(s, &phdr64);
>> + size = sizeof(phdr64);
>> + phdr = &phdr64;
>> + } else {
>> + write_elf32_phdr_note(s, &phdr32);
>> + size = sizeof(phdr32);
>> + phdr = &phdr32;
>> + }
>> +
>> + ret = fd_write_vmcore(phdr, size, s);
>> + if (ret < 0) {
>> + error_setg_errno(errp, -ret,
>> + "dump: failed to write program header table");
>> + }
>> +}
>> +
>> static void write_elf_section(DumpState *s, int type, Error **errp)
>> {
>> Elf32_Shdr shdr32;
>> @@ -540,14 +547,14 @@ static void dump_begin(DumpState *s, Error **errp)
>> return;
>> }
>>
>> - if (dump_is_64bit(s)) {
>> - /* write PT_NOTE to vmcore */
>> - write_elf64_note(s, &local_err);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> + /* write PT_NOTE to vmcore */
>> + write_elf_phdr_note(s, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>>
>> + if (dump_is_64bit(s)) {
>> /* write all PT_LOAD to vmcore */
>> write_elf_loads(s, &local_err);
>> if (local_err) {
>> @@ -571,13 +578,6 @@ static void dump_begin(DumpState *s, Error **errp)
>> return;
>> }
>> } else {
>> - /* write PT_NOTE to vmcore */
>> - write_elf32_note(s, &local_err);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> -
>> /* write all PT_LOAD to vmcore */
>> write_elf_loads(s, &local_err);
>> if (local_err) {
>> --
>> 2.32.0
>>
>>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/7] dump: Cleanup dump_begin write functions
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
` (4 preceding siblings ...)
2022-03-01 14:22 ` [PATCH 5/7] dump: Consolidate phdr note writes Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-01 14:22 ` [PATCH 7/7] dump: Consolidate elf note function Janosch Frank
6 siblings, 0 replies; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
There's no need to have a gigantic if in there let's move the elf
32/64 bit logic into the section, segment or note code.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 52 ++++++++++++++++------------------------------------
1 file changed, 16 insertions(+), 36 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 88c2dbb466..78654b9c27 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -554,52 +554,32 @@ static void dump_begin(DumpState *s, Error **errp)
return;
}
- if (dump_is_64bit(s)) {
- /* write all PT_LOAD to vmcore */
- write_elf_loads(s, &local_err);
+ /* write all PT_LOAD to vmcore */
+ write_elf_loads(s, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ /* write section to vmcore */
+ if (s->shdr_num) {
+ write_elf_section(s, 1, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
+ }
- /* write section to vmcore */
- if (s->shdr_num) {
- write_elf_section(s, 1, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
- }
-
+ if (dump_is_64bit(s)) {
/* write notes to vmcore */
write_elf64_notes(fd_write_vmcore, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
} else {
- /* write all PT_LOAD to vmcore */
- write_elf_loads(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
- /* write section to vmcore */
- if (s->shdr_num) {
- write_elf_section(s, 0, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
- }
-
/* write notes to vmcore */
write_elf32_notes(fd_write_vmcore, s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
}
}
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 7/7] dump: Consolidate elf note function
2022-03-01 14:22 [PATCH 0/7] dump: Cleanup and consolidation Janosch Frank
` (5 preceding siblings ...)
2022-03-01 14:22 ` [PATCH 6/7] dump: Cleanup dump_begin write functions Janosch Frank
@ 2022-03-01 14:22 ` Janosch Frank
2022-03-02 10:30 ` Marc-André Lureau
6 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-01 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, pbonzini
Just like with the other write functions let's move the 32/64 bit elf
handling to a function to improve readability.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
dump/dump.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/dump/dump.c b/dump/dump.c
index 78654b9c27..9ba0392e00 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
}
}
+static void write_elf_notes(DumpState *s, Error **errp)
+{
+ Error *local_err = NULL;
+
+ if (dump_is_64bit(s)) {
+ write_elf64_notes(fd_write_vmcore, s, &local_err);
+ } else {
+ write_elf32_notes(fd_write_vmcore, s, &local_err);
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+}
+
/* write elf header, PT_NOTE and elf note to vmcore. */
static void dump_begin(DumpState *s, Error **errp)
{
@@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
}
}
- if (dump_is_64bit(s)) {
- /* write notes to vmcore */
- write_elf64_notes(fd_write_vmcore, s, &local_err);
- } else {
- /* write notes to vmcore */
- write_elf32_notes(fd_write_vmcore, s, &local_err);
- }
+ /* write notes to vmcore */
+ write_elf_notes(s, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 7/7] dump: Consolidate elf note function
2022-03-01 14:22 ` [PATCH 7/7] dump: Consolidate elf note function Janosch Frank
@ 2022-03-02 10:30 ` Marc-André Lureau
2022-03-02 12:44 ` Janosch Frank
0 siblings, 1 reply; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-02 10:30 UTC (permalink / raw)
To: Janosch Frank; +Cc: Bonzini, Paolo, qemu-devel
Hi
On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Just like with the other write functions let's move the 32/64 bit elf
> handling to a function to improve readability.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> dump/dump.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 78654b9c27..9ba0392e00 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
> }
> }
>
> +static void write_elf_notes(DumpState *s, Error **errp)
> +{
> + Error *local_err = NULL;
> +
> + if (dump_is_64bit(s)) {
> + write_elf64_notes(fd_write_vmcore, s, &local_err);
> + } else {
> + write_elf32_notes(fd_write_vmcore, s, &local_err);
> + }
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
Please use "modern"-style ERRP_GUARD(), and indicate failure with a
bool (see include/qapi/error.h)
(perhaps this should be preliminary to this series)
> +}
> +
> /* write elf header, PT_NOTE and elf note to vmcore. */
> static void dump_begin(DumpState *s, Error **errp)
> {
> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
> }
> }
>
> - if (dump_is_64bit(s)) {
> - /* write notes to vmcore */
> - write_elf64_notes(fd_write_vmcore, s, &local_err);
> - } else {
> - /* write notes to vmcore */
> - write_elf32_notes(fd_write_vmcore, s, &local_err);
> - }
> + /* write notes to vmcore */
> + write_elf_notes(s, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 7/7] dump: Consolidate elf note function
2022-03-02 10:30 ` Marc-André Lureau
@ 2022-03-02 12:44 ` Janosch Frank
2022-03-02 21:15 ` Marc-André Lureau
0 siblings, 1 reply; 17+ messages in thread
From: Janosch Frank @ 2022-03-02 12:44 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Bonzini, Paolo, qemu-devel
On 3/2/22 11:30, Marc-André Lureau wrote:
> Hi
>
> On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>> Just like with the other write functions let's move the 32/64 bit elf
>> handling to a function to improve readability.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> dump/dump.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 78654b9c27..9ba0392e00 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
>> }
>> }
>>
>> +static void write_elf_notes(DumpState *s, Error **errp)
>> +{
>> + Error *local_err = NULL;
>> +
>> + if (dump_is_64bit(s)) {
>> + write_elf64_notes(fd_write_vmcore, s, &local_err);
>> + } else {
>> + write_elf32_notes(fd_write_vmcore, s, &local_err);
>> + }
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>
> Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> bool (see include/qapi/error.h)
Didn't know that's a thing, I'll have a look
>
> (perhaps this should be preliminary to this series)
So you want me to change all the local_error + error_propagate()s in
this file?
>
>> +}
>> +
>> /* write elf header, PT_NOTE and elf note to vmcore. */
>> static void dump_begin(DumpState *s, Error **errp)
>> {
>> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
>> }
>> }
>>
>> - if (dump_is_64bit(s)) {
>> - /* write notes to vmcore */
>> - write_elf64_notes(fd_write_vmcore, s, &local_err);
>> - } else {
>> - /* write notes to vmcore */
>> - write_elf32_notes(fd_write_vmcore, s, &local_err);
>> - }
>> + /* write notes to vmcore */
>> + write_elf_notes(s, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> --
>> 2.32.0
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 7/7] dump: Consolidate elf note function
2022-03-02 12:44 ` Janosch Frank
@ 2022-03-02 21:15 ` Marc-André Lureau
0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2022-03-02 21:15 UTC (permalink / raw)
To: Janosch Frank; +Cc: Bonzini, Paolo, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]
Hi
On Wed, Mar 2, 2022 at 6:02 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> On 3/2/22 11:30, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com>
> wrote:
> >>
> >> Just like with the other write functions let's move the 32/64 bit elf
> >> handling to a function to improve readability.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >> dump/dump.c | 24 +++++++++++++++++-------
> >> 1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index 78654b9c27..9ba0392e00 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> >> }
> >> }
> >>
> >> +static void write_elf_notes(DumpState *s, Error **errp)
> >> +{
> >> + Error *local_err = NULL;
> >> +
> >> + if (dump_is_64bit(s)) {
> >> + write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> + } else {
> >> + write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> + }
> >> + if (local_err) {
> >> + error_propagate(errp, local_err);
> >> + return;
> >> + }
> >
> > Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> > bool (see include/qapi/error.h)
>
> Didn't know that's a thing, I'll have a look
>
> >
> > (perhaps this should be preliminary to this series)
>
> So you want me to change all the local_error + error_propagate()s in
> this file?
>
>
It's not mandatory, but if you do that would be nice. Otherwise, try to
update the code you touch.
thanks
> >
> >> +}
> >> +
> >> /* write elf header, PT_NOTE and elf note to vmcore. */
> >> static void dump_begin(DumpState *s, Error **errp)
> >> {
> >> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
> >> }
> >> }
> >>
> >> - if (dump_is_64bit(s)) {
> >> - /* write notes to vmcore */
> >> - write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> - } else {
> >> - /* write notes to vmcore */
> >> - write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> - }
> >> + /* write notes to vmcore */
> >> + write_elf_notes(s, &local_err);
> >> if (local_err) {
> >> error_propagate(errp, local_err);
> >> return;
> >> --
> >> 2.32.0
> >>
> >
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3827 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread