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

[Janosch Frank]

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

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

I'll send out the other two series tomorrow.

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