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

[Peter Crosthwaite]

On Fri, Aug 8, 2014 at 4:07 PM, Hu Tao <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>
>> Reviewed-by: Peter Crosthwaite <address@hidden>
>>
>> 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