Patchwork [RFC,3/4] exec: don't exit unconditionally if failed to allocate memory

login
register
mail settings
Submitter Hu Tao
Date June 14, 2014, 4:48 a.m.
Message ID <0c6badd4ed1f0f3af9de36e4b5a17952922fdd05.1402720673.git.hutao@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/359750/
State New
Headers show

Comments

Hu Tao - June 14, 2014, 4:48 a.m.
return -1 instead.

Now user can add objects memory-backend-ram on-the-fly, fail it if
cannot allocate memory rather than quit qemu.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 backends/hostmem-ram.c | 3 +++
 exec.c                 | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
Paolo Bonzini - June 14, 2014, 5:07 p.m.
Il 14/06/2014 06:48, Hu Tao ha scritto:
> return -1 instead.
>
> Now user can add objects memory-backend-ram on-the-fly, fail it if
> cannot allocate memory rather than quit qemu.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

This needs an audit of all callers or, alternatively, we need to add 
memory_region_init_ram_nofail.  Better leave it for after the merge.

Paolo

> ---
>  backends/hostmem-ram.c | 3 +++
>  exec.c                 | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> index d9a8290..afb305d 100644
> --- a/backends/hostmem-ram.c
> +++ b/backends/hostmem-ram.c
> @@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>      path = object_get_canonical_path_component(OBJECT(backend));
>      memory_region_init_ram(&backend->mr, OBJECT(backend), path,
>                             backend->size);
> +    if (backend->mr.ram_addr == -1) {
> +        error_setg(errp, "can't allocate memory");
> +    }
>      g_free(path);
>  }
>
> diff --git a/exec.c b/exec.c
> index 8705cc5..74560e5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
>              if (!new_block->host) {
>                  fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
>                          new_block->mr->name, strerror(errno));
> -                exit(1);
> +                return -1;
>              }
>              memory_try_enable_merging(new_block->host, new_block->length);
>          }
> @@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
>  {
>      RAMBlock *block;
>
> +    if (addr == -1) {
> +        return;
> +    }
> +
>      /* This assumes the iothread lock is taken here too.  */
>      qemu_mutex_lock_ramlist();
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>
Michael S. Tsirkin - June 15, 2014, 9:58 a.m.
On Sat, Jun 14, 2014 at 07:07:39PM +0200, Paolo Bonzini wrote:
> Il 14/06/2014 06:48, Hu Tao ha scritto:
> >return -1 instead.
> >
> >Now user can add objects memory-backend-ram on-the-fly, fail it if
> >cannot allocate memory rather than quit qemu.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> 
> This needs an audit of all callers or, alternatively, we need to add
> memory_region_init_ram_nofail.  Better leave it for after the merge.
> 
> Paolo

Specifically memory_region_init_ram_from_file does not seem to
handle failures.

qemu_ram_free chunk also looks weird. Can we not avoid calling
free on invalid addresses?

> >---
> > backends/hostmem-ram.c | 3 +++
> > exec.c                 | 6 +++++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> >diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> >index d9a8290..afb305d 100644
> >--- a/backends/hostmem-ram.c
> >+++ b/backends/hostmem-ram.c
> >@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >     path = object_get_canonical_path_component(OBJECT(backend));
> >     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
> >                            backend->size);
> >+    if (backend->mr.ram_addr == -1) {
> >+        error_setg(errp, "can't allocate memory");
> >+    }
> >     g_free(path);
> > }
> >
> >diff --git a/exec.c b/exec.c
> >index 8705cc5..74560e5 100644
> >--- a/exec.c
> >+++ b/exec.c
> >@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
> >             if (!new_block->host) {
> >                 fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> >                         new_block->mr->name, strerror(errno));
> >-                exit(1);
> >+                return -1;
> >             }
> >             memory_try_enable_merging(new_block->host, new_block->length);
> >         }
> >@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
> > {
> >     RAMBlock *block;
> >
> >+    if (addr == -1) {
> >+        return;
> >+    }
> >+
> >     /* This assumes the iothread lock is taken here too.  */
> >     qemu_mutex_lock_ramlist();
> >     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> >
Hu Tao - June 16, 2014, 9:54 a.m.
On Sun, Jun 15, 2014 at 12:58:47PM +0300, Michael S. Tsirkin wrote:
> On Sat, Jun 14, 2014 at 07:07:39PM +0200, Paolo Bonzini wrote:
> > Il 14/06/2014 06:48, Hu Tao ha scritto:
> > >return -1 instead.
> > >
> > >Now user can add objects memory-backend-ram on-the-fly, fail it if
> > >cannot allocate memory rather than quit qemu.
> > >
> > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > 
> > This needs an audit of all callers or, alternatively, we need to add
> > memory_region_init_ram_nofail.  Better leave it for after the merge.

Paolo, IIUC you suggested we fix it after merge.

> > 
> > Paolo
> 
> Specifically memory_region_init_ram_from_file does not seem to
> handle failures.

memory_region_init_ram_from_file has an errp. Also see my updated patch.

> 
> qemu_ram_free chunk also looks weird. Can we not avoid calling
> free on invalid addresses?

but we still need to check the address somewhere if not in
qemu_ram_free.

> 
> > >---
> > > backends/hostmem-ram.c | 3 +++
> > > exec.c                 | 6 +++++-
> > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> > >index d9a8290..afb305d 100644
> > >--- a/backends/hostmem-ram.c
> > >+++ b/backends/hostmem-ram.c
> > >@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> > >     path = object_get_canonical_path_component(OBJECT(backend));
> > >     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
> > >                            backend->size);
> > >+    if (backend->mr.ram_addr == -1) {
> > >+        error_setg(errp, "can't allocate memory");
> > >+    }
> > >     g_free(path);
> > > }
> > >
> > >diff --git a/exec.c b/exec.c
> > >index 8705cc5..74560e5 100644
> > >--- a/exec.c
> > >+++ b/exec.c
> > >@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
> > >             if (!new_block->host) {
> > >                 fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> > >                         new_block->mr->name, strerror(errno));
> > >-                exit(1);
> > >+                return -1;
> > >             }
> > >             memory_try_enable_merging(new_block->host, new_block->length);
> > >         }
> > >@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
> > > {
> > >     RAMBlock *block;
> > >
> > >+    if (addr == -1) {
> > >+        return;
> > >+    }
> > >+
> > >     /* This assumes the iothread lock is taken here too.  */
> > >     qemu_mutex_lock_ramlist();
> > >     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > >
Paolo Bonzini - June 16, 2014, 10:07 a.m.
Il 16/06/2014 11:54, Hu Tao ha scritto:
>>> > > This needs an audit of all callers or, alternatively, we need to add
>>> > > memory_region_init_ram_nofail.  Better leave it for after the merge.
> Paolo, IIUC you suggested we fix it after merge.

Yes.

>> > Specifically memory_region_init_ram_from_file does not seem to
>> > handle failures.
> memory_region_init_ram_from_file has an errp. Also see my updated patch.

Yes, memory_region_init_ram_from_file is okay.  memory_region_init_ram 
is the one that doesn't handle failures.

> > qemu_ram_free chunk also looks weird. Can we not avoid calling
> > free on invalid addresses?
>
> but we still need to check the address somewhere if not in
> qemu_ram_free.

If we add an errp to memory_region_init_ram, the qemu_ram_free hunk 
probably will not be necessary anymore.

Paolo

Patch

diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index d9a8290..afb305d 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -28,6 +28,9 @@  ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     path = object_get_canonical_path_component(OBJECT(backend));
     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                            backend->size);
+    if (backend->mr.ram_addr == -1) {
+        error_setg(errp, "can't allocate memory");
+    }
     g_free(path);
 }
 
diff --git a/exec.c b/exec.c
index 8705cc5..74560e5 100644
--- a/exec.c
+++ b/exec.c
@@ -1228,7 +1228,7 @@  static ram_addr_t ram_block_add(RAMBlock *new_block)
             if (!new_block->host) {
                 fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
                         new_block->mr->name, strerror(errno));
-                exit(1);
+                return -1;
             }
             memory_try_enable_merging(new_block->host, new_block->length);
         }
@@ -1356,6 +1356,10 @@  void qemu_ram_free(ram_addr_t addr)
 {
     RAMBlock *block;
 
+    if (addr == -1) {
+        return;
+    }
+
     /* This assumes the iothread lock is taken here too.  */
     qemu_mutex_lock_ramlist();
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {