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

[Peter Crosthwaite]

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.

> +        return -1;
> +    }
> +    return addr;
>  }
>  #endif
>
>  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
> -                                   MemoryRegion *mr)
> +                                   MemoryRegion *mr, Error **errp)
>  {
>      RAMBlock *new_block;
> +    ram_addr_t addr;
>
>      size = TARGET_PAGE_ALIGN(size);
>      new_block = g_malloc0(sizeof(*new_block));
> @@ -1341,12 +1350,17 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
> void *host,
>      if (host) {
>          new_block->flags |= RAM_PREALLOC;
>      }
> -    return ram_block_add(new_block);
> +    addr = ram_block_add(new_block, errp);
> +    if (errp && *errp) {
> +        g_free(new_block);

ditto.

Regards,
Peter

> +        return -1;
> +    }
> +    return addr;
>  }
>
> -ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr)
> +ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr, Error **errp)
>  {
> -    return qemu_ram_alloc_from_ptr(size, NULL, mr);
> +    return qemu_ram_alloc_from_ptr(size, NULL, mr, errp);
>  }
>
>  void qemu_ram_free_from_ptr(ram_addr_t addr)
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 6593be1..cf1d4c7 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -26,8 +26,8 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
>                                      bool share, const char *mem_path,
>                                      Error **errp);
>  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
> -                                   MemoryRegion *mr);
> -ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
> +                                   MemoryRegion *mr, Error **errp);
> +ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr, Error **errp);
>  int qemu_get_ram_fd(ram_addr_t addr);
>  void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>  void *qemu_get_ram_ptr(ram_addr_t addr);
> diff --git a/memory.c b/memory.c
> index 64d7176..59d9935 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1169,7 +1169,7 @@ void memory_region_init_ram(MemoryRegion *mr,
>      mr->ram = true;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
> -    mr->ram_addr = qemu_ram_alloc(size, mr);
> +    mr->ram_addr = qemu_ram_alloc(size, mr, &error_abort);
>  }
>
>  #ifdef __linux__
> @@ -1199,7 +1199,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>      mr->ram = true;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram_from_ptr;
> -    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr);
> +    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
>  }
>
>  void memory_region_init_alias(MemoryRegion *mr,
> @@ -1229,7 +1229,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
>      mr->terminates = true;
>      mr->rom_device = true;
>      mr->destructor = memory_region_destructor_rom_device;
> -    mr->ram_addr = qemu_ram_alloc(size, mr);
> +    mr->ram_addr = qemu_ram_alloc(size, mr, &error_abort);
>  }
>
>  void memory_region_init_iommu(MemoryRegion *mr,
> --
> 1.9.3
>
>