Re: [PATCH 2/5] dump: Split write of section headers and data and add a prepare step

[Richard Henderson]

On 3/10/22 03:16, Janosch Frank wrote:
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static size_t write_elf_section_hdr_zero(DumpState *s, void *buff)
>   {
> -    Elf32_Shdr shdr32;
> -    Elf64_Shdr shdr64;
> -    int shdr_size;
> -    void *shdr;
> -    int ret;
> +    Elf32_Shdr *shdr32 = buff;
> +    Elf64_Shdr *shdr64 = buff;
>   
> -    if (type == 0) {
> -        shdr_size = sizeof(Elf32_Shdr);
> -        memset(&shdr32, 0, shdr_size);
> -        shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
> -        shdr = &shdr32;
> +    if (dump_is_64bit(s)) {
> +        memset(buff, 0, sizeof(Elf64_Shdr));
> +        shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
>       } else {
> -        shdr_size = sizeof(Elf64_Shdr);
> -        memset(&shdr64, 0, shdr_size);
> -        shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> -        shdr = &shdr64;
> +        memset(buff, 0, sizeof(Elf32_Shdr));
> +        shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
>       }

I'd move those shdr* variables into the if blocks.
It looks odd to have them both in scope at once.

You're re-zeroing the data -- it was allocated with g_malloc0.

>   
> -    ret = fd_write_vmcore(shdr, shdr_size, s);
> +    return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +}

Return value here...

> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> +    uint8_t *buff_hdr;
> +    size_t len, sizeof_shdr;
> +
> +    /*
> +     * Section ordering:
> +     * - HDR zero (if needed)
> +     */
> +    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +    len = sizeof_shdr * s->shdr_num ;
> +    s->elf_section_hdrs = g_malloc0(len);

Perhaps save to s->shdr_len?

> +    buff_hdr = s->elf_section_hdrs;

Why this extra variable?

> +
> +    /* Write special section first */
> +    if (s->phdr_num == PN_XNUM) {
> +            write_elf_section_hdr_zero(s, buff_hdr);
> +    }

... but not used here.  Was there some other intended use?  You already know the size, per 
above...

> +static void write_elf_section_headers(DumpState *s, Error **errp)
> +{
> +    size_t sizeof_shdr;
> +    int ret;
> +
> +    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +
> +    ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s);

Use saved s->shdr_len?  Skip if 0?

> +static void write_elf_sections(DumpState *s, Error **errp)
> +{
> +    int ret;
> +
> +    /* Write section zero */
> +    ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s);

Again skip if 0?  Comment is misleading because section 0 should have no 
contents...


r~