Re: [Qemu-devel] [PATCH v2 1/2] elf-ops.h: Map into memory the ELF to load

[Paolo Bonzini <pbonzini@redhat.com>]

On 23/07/19 16:04, Stefano Garzarella wrote:
> +                    /* Increments the reference count to avoid the unmap */
> +                    g_mapped_file_ref(gmf);
>                      /* rom_add_elf_program() seize the ownership of 'data' */
>                      rom_add_elf_program(label, data, file_size, mem_size,
>                                          addr, as);

I'm a bit worried about rom_reset g_free'ing rom->data, which goes
against the comment on top of rom_free:

/* rom->data must be heap-allocated (do not use with
   rom_add_elf_program()) */


Since this is the only call to rom_add_elf_program, what about adding a
GMappedFile* field to struct Rom and passing it here instead of
data+file_size?

Then the g_mapped_file_ref can be in rom_add_elf_program, and you can
have a nice

static void rom_free_data(Rom *rom)
{
    if (rom->mapped_file) {
        g_mapped_file_unref(rom->mapped_file);
        rom->mapped_file = NULL;
    } else {
        g_free(rom->data);
    }
    rom->data = NULL;
}

that is called from both rom_free and rom_reset.

Thanks,

Paolo

> @@ -531,7 +540,6 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      address_space_write(as ? as : &address_space_memory,
>                                          addr, MEMTXATTRS_UNSPECIFIED,
>                                          data, file_size);
> -                    g_free(data);
>                  }
>              }
>  
> @@ -547,16 +555,15 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>              struct elf_note *nhdr = NULL;
>  
>              file_size = ph->p_filesz; /* Size of the range of ELF notes */
> -            data = g_malloc0(file_size);
> -            if (ph->p_filesz > 0) {
> -                if (lseek(fd, ph->p_offset, SEEK_SET) < 0) {
> -                    goto fail;
> -                }
> -                if (read(fd, data, file_size) != file_size) {
> -                    goto fail;
> -                }
> +            data_offset = ph->p_offset; /* Offset where the notes are located */
> +
> +            if (g_mapped_file_get_length(gmf) < file_size + data_offset) {
> +                goto fail;
>              }
>  
> +            data = (uint8_t *)g_mapped_file_get_contents(gmf);
> +            data += data_offset;
> +
>              /*
>               * Search the ELF notes to find one with a type matching the
>               * value passed in via 'translate_opaque'
> @@ -570,7 +577,6 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      sizeof(struct elf_note) == sizeof(struct elf64_note);
>                  elf_note_fn((void *)nhdr, (void *)&ph->p_align, is64);
>              }
> -            g_free(data);
>              data = NULL;
>          }
>      }
> @@ -582,7 +588,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>          *highaddr = (uint64_t)(elf_sword)high;
>      return total_size;
>   fail:
> -    g_free(data);
> +    g_mapped_file_unref(gmf);
>      g_free(phdr);
>      return ret;
>  }
>