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

[Stefano Garzarella]

On Tue, Jul 23, 2019 at 04:33:44PM +0200, Paolo Bonzini wrote:
> 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()) */

Thanks for pointing it out. We definitely have to avoid that free in this case.

> 
> 
> 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?

Ehm, we currently use a subsection of the mmapped file as a ROM.
Should we keep the data+file_size parameter? (plus the new GMappedFile*)

At this point, is it better to call 'g_mapped_file_ref(gmf)' in the
'rom_add_elf_program()'?

> 
> 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.

I'll add this in v3.

Thanks,
Stefano