Message ID | 8bf29143b325757007186e2ab6f17c5d4d4f5c6c.1407402040.git.hutao@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hutao@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 <hutao@cn.fujitsu.com> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@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 > >
On Thu, Aug 07, 2014 at 09:24:35PM +1000, Peter Crosthwaite wrote: > On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hutao@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 <hutao@cn.fujitsu.com> > > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@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. Okay. > > > + 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(). It did but mst suggested return -1 instead. Regards, Hu > > > + } > > + 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 > > > >
On Fri, Aug 8, 2014 at 4:07 PM, Hu Tao <hutao@cn.fujitsu.com> wrote: > On Thu, Aug 07, 2014 at 09:24:35PM +1000, Peter Crosthwaite wrote: >> On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hutao@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 <hutao@cn.fujitsu.com> >> >> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@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. > > Okay. > >> >> > + 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(). > > It did but mst suggested return -1 instead. > Fair enough. Clash of personal prefs I guess. Regards, Peter
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); + 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; + } + 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; + } + 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,
Add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr so that we can handle errors. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- exec.c | 36 +++++++++++++++++++++++++++--------- include/exec/ram_addr.h | 4 ++-- memory.c | 6 +++--- 3 files changed, 32 insertions(+), 14 deletions(-)