Re: [Qemu-devel] [PATCH v5 1/6] exec: add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr

[Hu Tao]

On Wed, Aug 06, 2014 at 10:15:17PM +1000, Peter Crosthwaite wrote:
> On Wed, Aug 6, 2014 at 3:36 PM, Hu Tao <address@hidden> wrote:
> > Add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr so that
> > we can handler errors.
> 
> "handle"
> 
> >
> > Signed-off-by: Hu Tao <address@hidden>
> > ---
> >  exec.c                  | 32 +++++++++++++++++++++++---------
> >  include/exec/ram_addr.h |  4 ++--
> >  memory.c                |  6 +++---
> >  3 files changed, 28 insertions(+), 14 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 765bd94..7e60a44 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1224,7 +1224,7 @@ static int memory_try_enable_merging(void *addr, 
> > size_t len)
> >      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
> >  }
> >
> > -static ram_addr_t ram_block_add(RAMBlock *new_block)
> > +static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
> >  {
> >      RAMBlock *block;
> >      ram_addr_t old_ram_size, new_ram_size;
> > @@ -1241,9 +1241,11 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
> >          } else {
> >              new_block->host = phys_mem_alloc(new_block->length);
> >              if (!new_block->host) {
> > -                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> > -                        new_block->mr->name, strerror(errno));
> > -                exit(1);
> > +                error_setg_errno(errp, errno,
> > +                                 "cannot set up guest memory '%s'",
> > +                                 new_block->mr->name);
> > +                qemu_mutex_unlock_ramlist();
> > +                return -1;
> >              }
> >              memory_try_enable_merging(new_block->host, new_block->length);
> >          }
> > @@ -1294,6 +1296,7 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, 
> > MemoryRegion *mr,
> >                                      Error **errp)
> >  {
> >      RAMBlock *new_block;
> > +    ram_addr_t addr;
> >
> >      if (xen_enabled()) {
> >          error_setg(errp, "-mem-path not supported with Xen");
> > @@ -1323,14 +1326,20 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t 
> > size, MemoryRegion *mr,
> >          return -1;
> >      }
> >
> > -    return ram_block_add(new_block);
> > +    addr = ram_block_add(new_block, errp);
> > +    if (errp && *errp) {
> > +        g_free(new_block);
> 
> The free being conditional on errp will cause a leak if clients
> (validly) pass a NULL errp in. This free needs to be unconditional.
> The way to achieve that is the local_err error_propagate pattern.

Yes.

Regards,
Hu