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

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

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
>>
>>
>>
>