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

From: Stefano Garzarella <sgarzare@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Sergio Lopez <slp@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Julio Montes <julio.montes@intel.com>,
	qemu-devel@nongnu.org,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH v3 2/3] elf-ops.h: Map into memory the ELF to load
Date: Wed, 24 Jul 2019 14:35:27 +0200	[thread overview]
Message-ID: <20190724123527.7jfmoisbcogoiqex@steredhat> (raw)
In-Reply-To: <08e22ee6-8216-8563-e35a-8c02fc201fc9@redhat.com>

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