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