diff mbox

[v5,1/6] exec: add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr

Message ID 4029745e77f041f48cbf4bbaad810cb0d03970ad.1407302379.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao Aug. 6, 2014, 5:36 a.m. UTC 
Add parameter errp to qemu_ram_alloc and qemu_ram_alloc_from_ptr so that
we can handler errors.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 exec.c                  | 32 +++++++++++++++++++++++---------
 include/exec/ram_addr.h |  4 ++--
 memory.c                |  6 +++---
 3 files changed, 28 insertions(+), 14 deletions(-)

Comments

Peter Crosthwaite Aug. 6, 2014, 12:15 p.m. UTC | #1 
On Wed, Aug 6, 2014 at 3:36 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 handler errors.

"handle"

>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  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
>
>
Hu Tao Aug. 7, 2014, 7:52 a.m. UTC | #2 
On Wed, Aug 06, 2014 at 10:15:17PM +1000, Peter Crosthwaite wrote:
> On Wed, Aug 6, 2014 at 3:36 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 handler errors.
> 
> "handle"
> 
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  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.

Yes.

Regards,
Hu
diff mbox

Patch

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);
+        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);
+        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,