Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Yin Olivia-R63875]

Hi Stuart & Alex,

"syms" and "str" could not be free since "symbols" is a global variable which
need stay in the memory during the whole life of VM. So it will not be free and 
reload when reset.

The only change is to the previous patch of elf loader (hw/elf_ops.h) as below:

@@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     if (pentry)
        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;

-    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    if (!reset) {
+        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    }

     size = ehdr.e_phnum * sizeof(phdr[0]);
     lseek(fd, ehdr.e_phoff, SEEK_SET); 

Do you think it is reasonable?

Best Regards,
Olivia

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Thursday, November 22, 2012 2:42 AM
> To: Stuart Yoder
> Cc: Yin Olivia-R63875; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
> load_at()
> 
> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> > On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<address@hidden>
> wrote:
> >> Signed-off-by: Olivia Yin<address@hidden>
> >> ---
> >>   hw/elf_ops.h |    2 ++
> >>   1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
> >> 100644
> >> --- a/hw/elf_ops.h
> >> +++ b/hw/elf_ops.h
> >> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
> *ehdr, int fd, int must_swab,
> >>       s->disas_strtab = str;
> >>       s->next = syminfos;
> >>       syminfos = s;
> >> +    g_free(syms);
> >> +    g_free(str);
> >>       g_free(shdr_table);
> >>       return 0;
> >>    fail:
> > Olivia, as Alex pointed out there are references to syms and str in
> > the struct "s"....so you can't just free those I don't think.
> >
> > The problem that leaves us with is that on every reset when we call
> > load_elf() that we re-load and re-malloc space for the symbols.
> >
> > I think the solution may be to factor out the call to load_symbols()
> > from load_elf().   It looks like what load_symbols does in the end is
> > set the variable syminfos to point to the loaded symbol info.
> >
> > If you factor load_symbols() out then in load_elf_32/64() you would do
> > something like:
> >        elf_phy_loader_32/64()
> >        load_symbols_32/64().
> >
> > We don't need to be reloading symbols on every reset.
> >
> > Alex, does that make sense?
> 
> We can also mandate the caller of load_symbols to free the respective
> data :)
> 
> 
> Alex
>