Re: [Qemu-devel] [PATCH] linux-user: elf: Map empty PT_LOAD sections

[Peter Maydell]

On Thu, 18 Apr 2019 at 17:46, Giuseppe Musacchio <address@hidden> wrote:
>
> Some PT_LOAD sections may be completely zeroed out and their p_filesize
> is zero, in that case the loader should just allocate a page that's at
> least p_memsz bytes large (plus eventual alignment padding).

Thanks for this patch -- codewise it looks good, so I just have
a couple of minor tweaks I think we should make to the comment
and commit message:

The things we're iterating through here are segments, not
sections (an ELF section is a different sort of data structure):
could you fix the text in your commit message and comment, please?

> Calling zero_bss does this job for us, all we have to do is make sure we
> don't try to mmap a zero-length page.
>
> Signed-off-by: Giuseppe Musacchio <address@hidden>
> ---
>  linux-user/elfload.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index c1a2602..e9a0951 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2366,11 +2366,17 @@ static void load_elf_image(const char *image_name, 
> int image_fd,
>              vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
>              vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
>
> -            error = target_mmap(vaddr_ps, vaddr_len,
> -                                elf_prot, MAP_PRIVATE | MAP_FIXED,
> -                                image_fd, eppnt->p_offset - vaddr_po);
> -            if (error == -1) {
> -                goto exit_perror;
> +            /* Some sections may be completely empty without any backing file
> +             * segment, in that case just let zero_bss allocate an empty 
> buffer
> +             * for it. */

Our coding style says that multi-line comments should be
 /*
  * like this, with the start and
  * end markers on lines of their own
  */

(if you run your patch through scripts/checkpatch.pl it ought
to detect this sort of minor coding style nit).

> +            if (eppnt->p_filesz != 0) {
> +                error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> +                                    MAP_PRIVATE | MAP_FIXED,
> +                                    image_fd, eppnt->p_offset - vaddr_po);
> +
> +                if (error == -1) {
> +                    goto exit_perror;
> +                }
>              }
>
>              vaddr_ef = vaddr + eppnt->p_filesz;

Other than those minor things:
Reviewed-by: Peter Maydell <address@hidden>

so if you fix them and send out a v2 patch you can include
my Reviewed-by: tag in the commit message (under your
signed-off-by) in that patch email.

thanks
-- PMM