On Fri, Aug 8, 2014 at 4:07 PM, Hu Tao <hu...@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 <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.
>
> 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
