[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] elf-ops.h: Map into memory the ELF to lo
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] elf-ops.h: Map into memory the ELF to load |
Date: |
Tue, 23 Jul 2019 16:33:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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;
> }
>