Patchwork [v3,for,1.6,7/8] exec: Don't abort when we can't allocate guest memory

login
register
mail settings
Submitter Markus Armbruster
Date July 31, 2013, 1:11 p.m.
Message ID <1375276272-15988-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/263710/
State New
Headers show

Comments

Markus Armbruster - July 31, 2013, 1:11 p.m.
We abort() on memory allocation failure.  abort() is appropriate for
programming errors.  Maybe most memory allocation failures are
programming errors, maybe not.  But guest memory allocation failure
isn't, and aborting when the user asks for more memory than we can
provide is not nice.  exit(1) instead, and do it in just one place, so
the error message is consistent.

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c             | 5 +++++
 target-s390x/kvm.c | 6 +-----
 util/oslib-posix.c | 4 +---
 util/oslib-win32.c | 5 +----
 4 files changed, 8 insertions(+), 12 deletions(-)
Andreas Färber - July 31, 2013, 1:51 p.m.
Am 31.07.2013 15:11, schrieb Markus Armbruster:
> We abort() on memory allocation failure.  abort() is appropriate for
> programming errors.  Maybe most memory allocation failures are
> programming errors, maybe not.  But guest memory allocation failure
> isn't, and aborting when the user asks for more memory than we can
> provide is not nice.  exit(1) instead, and do it in just one place, so
> the error message is consistent.
> 
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  exec.c             | 5 +++++
>  target-s390x/kvm.c | 6 +-----
>  util/oslib-posix.c | 4 +---
>  util/oslib-win32.c | 5 +----
>  4 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 231d04e..0cfca3a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1162,6 +1162,11 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>          }
>          if (!new_block->host) {
>              new_block->host = phys_mem_alloc(size);
> +            if (!new_block->host) {
> +                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> +                        new_block->mr->name, strerror(errno));

This could use error_report() while at it, but still

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thought I had ack'ed it long ago, but I guess something minor changed.

Cheers,
Andreas

> +                exit(1);
> +            }
>              memory_try_enable_merging(new_block->host, size);
>          }
>      }
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index b5351e6..c4a0fdd 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -331,11 +331,7 @@ static void *legacy_s390_alloc(ram_addr_t size)
>      mem = mmap((void *) 0x800000000ULL, size,
>                 PROT_EXEC|PROT_READ|PROT_WRITE,
>                 MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> -    if (mem == MAP_FAILED) {
> -        fprintf(stderr, "Allocating RAM failed\n");
> -        abort();
> -    }
> -    return mem;
> +    return mem == MAP_FAILED ? NULL : mem;
>  }
>  
>  int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 3dc8b1b..253bc3d 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -112,9 +112,7 @@ void *qemu_anon_ram_alloc(size_t size)
>      size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>  
>      if (ptr == MAP_FAILED) {
> -        fprintf(stderr, "Failed to allocate %zu B: %s\n",
> -                size, strerror(errno));
> -        abort();
> +        return NULL;
>      }
>  
>      ptr += offset;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 961fbf5..983b7a2 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -65,10 +65,7 @@ void *qemu_anon_ram_alloc(size_t size)
>      /* FIXME: this is not exactly optimal solution since VirtualAlloc
>         has 64Kb granularity, but at least it guarantees us that the
>         memory is page aligned. */
> -    if (!size) {
> -        abort();
> -    }
> -    ptr = qemu_oom_check(VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE));
> +    ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
>      trace_qemu_anon_ram_alloc(size, ptr);
>      return ptr;
>  }
>

Patch

diff --git a/exec.c b/exec.c
index 231d04e..0cfca3a 100644
--- a/exec.c
+++ b/exec.c
@@ -1162,6 +1162,11 @@  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         }
         if (!new_block->host) {
             new_block->host = phys_mem_alloc(size);
+            if (!new_block->host) {
+                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
+                        new_block->mr->name, strerror(errno));
+                exit(1);
+            }
             memory_try_enable_merging(new_block->host, size);
         }
     }
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index b5351e6..c4a0fdd 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -331,11 +331,7 @@  static void *legacy_s390_alloc(ram_addr_t size)
     mem = mmap((void *) 0x800000000ULL, size,
                PROT_EXEC|PROT_READ|PROT_WRITE,
                MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
-    if (mem == MAP_FAILED) {
-        fprintf(stderr, "Allocating RAM failed\n");
-        abort();
-    }
-    return mem;
+    return mem == MAP_FAILED ? NULL : mem;
 }
 
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 3dc8b1b..253bc3d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -112,9 +112,7 @@  void *qemu_anon_ram_alloc(size_t size)
     size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
 
     if (ptr == MAP_FAILED) {
-        fprintf(stderr, "Failed to allocate %zu B: %s\n",
-                size, strerror(errno));
-        abort();
+        return NULL;
     }
 
     ptr += offset;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 961fbf5..983b7a2 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -65,10 +65,7 @@  void *qemu_anon_ram_alloc(size_t size)
     /* FIXME: this is not exactly optimal solution since VirtualAlloc
        has 64Kb granularity, but at least it guarantees us that the
        memory is page aligned. */
-    if (!size) {
-        abort();
-    }
-    ptr = qemu_oom_check(VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE));
+    ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
     trace_qemu_anon_ram_alloc(size, ptr);
     return ptr;
 }