On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hu...@cn.fujitsu.com> wrote:
> Add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr so that
> we can handle errors.
>
> Signed-off-by: Hu Tao <hu...@cn.fujitsu.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
Optional nit-picky suggestions below.
> ---
> exec.c | 36 +++++++++++++++++++++++++++---------
> include/exec/ram_addr.h | 4 ++--
> memory.c | 6 +++---
> 3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 765bd94..accba00 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);
Out of scope, but if you do need to respin you could change to
memory_region_name to avoid the direct struct access of private mr
fields.
> + qemu_mutex_unlock_ramlist();
> + return -1;
> }
> memory_try_enable_merging(new_block->host, new_block->length);
> }
> @@ -1294,6 +1296,8 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size,
> MemoryRegion *mr,
> Error **errp)
> {
> RAMBlock *new_block;
> + ram_addr_t addr;
> + Error *local_err = NULL;
>
> if (xen_enabled()) {
> error_setg(errp, "-mem-path not supported with Xen");
> @@ -1323,14 +1327,22 @@ 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, &local_err);
> + if (local_err) {
> + g_free(new_block);
> + error_propagate(errp, local_err);
> + return -1;
This should be redundant I think. You are unnecessarily defining the
error return code in two places when you can just propagate the return
code from the botched ram_block_add().
> + }
> + 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;
> + Error *local_err = NULL;
>
> size = TARGET_PAGE_ALIGN(size);
> new_block = g_malloc0(sizeof(*new_block));
> @@ -1341,12 +1353,18 @@ 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, &local_err);
> + if (local_err) {
> + g_free(new_block);
> + error_propagate(errp, local_err);
> + return -1;
Ditto.
Regards,
Peter
> + }
> + 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
>
>
