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

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 01:50:58PM +0200, Paolo Bonzini wrote:
> On 24/07/19 13:25, Stefano Garzarella wrote:
> > @@ -582,7 +596,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> >          *highaddr = (uint64_t)(elf_sword)high;
> >      return total_size;
> 
> Isn't the success case missing a g_mapped_file_unref?  It has to be done
> unconditionally since now rom_add_elf_program adds a separate reference.

Sure, I had this in mind, since I initialized mapped_file to NULL, but
I didn't see the return before "fail:" label!
Maybe I'll change the end of load_elf() in this way:

-    g_free(phdr);
     if (lowaddr)
         *lowaddr = (uint64_t)(elf_sword)low;
     if (highaddr)
         *highaddr = (uint64_t)(elf_sword)high;
-    return total_size;
+    ret = total_size;
  fail:
-    g_free(data);
+    g_mapped_file_unref(mapped_file);
     g_free(phdr);
     return ret;
 }


> 
> Related to this, the comment
> 
>             /* rom_add_elf_program() seize the ownership of 'data' */
> 
> refers to the g_free(data) that you are removing and is best changed to just
>             /*
>              * rom_add_elf_program() takes its own reference to
>              * mapped_file.
>              */

I'll update the comment.

Thanks,
Stefano