Igor Mammedov <imamm...@redhat.com> writes: > When adding hostmem backend at runtime, QEMU might exit with error: > "os_mem_prealloc: Insufficient free host memory pages available to allocate > guest RAM" > > It happens due to os_mem_prealloc() not handling errors gracefully. > > Fix it by passing errp argument so that os_mem_prealloc() could > report error to callers and undo performed allocation when > os_mem_prealloc() fails. > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > --- > include/qemu/osdep.h | 2 +- > backends/hostmem.c | 18 ++++++++++++++---- > exec.c | 10 ++++++++-- > util/oslib-posix.c | 25 ++++++++++++------------- > util/oslib-win32.c | 2 +- > 5 files changed, 36 insertions(+), 21 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index fbb8759..d7c111d 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -379,7 +379,7 @@ unsigned long qemu_getauxval(unsigned long type); > > void qemu_set_tty_echo(int fd, bool echo); > > -void os_mem_prealloc(int fd, char *area, size_t sz); > +void os_mem_prealloc(int fd, char *area, size_t sz, Error **errp); > > int qemu_read_password(char *buf, int buf_size); > > diff --git a/backends/hostmem.c b/backends/hostmem.c > index ac80257..b7a208d 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -203,6 +203,7 @@ static bool host_memory_backend_get_prealloc(Object *obj, > Error **errp) > static void host_memory_backend_set_prealloc(Object *obj, bool value, > Error **errp) > { > + Error *local_err = NULL; > HostMemoryBackend *backend = MEMORY_BACKEND(obj); > > if (backend->force_prealloc) { > @@ -223,7 +224,11 @@ static void host_memory_backend_set_prealloc(Object > *obj, bool value, > void *ptr = memory_region_get_ram_ptr(&backend->mr); > uint64_t sz = memory_region_size(&backend->mr); > > - os_mem_prealloc(fd, ptr, sz); > + os_mem_prealloc(fd, ptr, sz, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > backend->prealloc = true; > } > } > @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, > Error **errp) > if (bc->alloc) { > bc->alloc(backend, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + goto out;
I'd leave the tail merging optimization to the compiler, i.e. don't touch the code here, and ... > } > > ptr = memory_region_get_ram_ptr(&backend->mr); > @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, > Error **errp) > * specified NUMA policy in place. > */ > if (backend->prealloc) { > - os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz); > + os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz, > + &local_err); > + if (local_err) { > + goto out; ... have the obvious error_propagate() + return here. Matter of taste, between you and the maintainer. Except there is none. Inexcusable for a file created in 2014. Suggest you appoint yourself. > + } > } > } > +out: > + error_propagate(errp, local_err); > } > > static bool > diff --git a/exec.c b/exec.c > index 60cf46a..bf1446c 100644 > --- a/exec.c > +++ b/exec.c > @@ -1268,7 +1268,7 @@ static void *file_ram_alloc(RAMBlock *block, > char *filename; > char *sanitized_name; > char *c; > - void *area; > + void *area = MAP_FAILED; > int fd = -1; > int64_t page_size; > > @@ -1356,13 +1356,19 @@ static void *file_ram_alloc(RAMBlock *block, > } > > if (mem_prealloc) { > - os_mem_prealloc(fd, area, memory); > + os_mem_prealloc(fd, area, memory, errp); > + if (errp && *errp) { > + goto error; > + } > } > > block->fd = fd; > return area; > > error: > + if (area != MAP_FAILED) { > + qemu_ram_munmap(area, memory); > + } > if (unlink_on_error) { > unlink(path); > } > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 6d70d9a..a60ac97 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -318,7 +318,7 @@ static void sigbus_handler(int signal) > siglongjmp(sigjump, 1); > } > > -void os_mem_prealloc(int fd, char *area, size_t memory) > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > { > int ret; > struct sigaction act, oldact; > @@ -330,8 +330,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > > ret = sigaction(SIGBUS, &act, &oldact); > if (ret) { > - perror("os_mem_prealloc: failed to install signal handler"); > - exit(1); > + error_setg_errno(errp, errno, > + "os_mem_prealloc: failed to install signal handler"); > + return; > } > > /* unblock SIGBUS */ > @@ -340,9 +341,8 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > pthread_sigmask(SIG_UNBLOCK, &set, &oldset); > > if (sigsetjmp(sigjump, 1)) { > - fprintf(stderr, "os_mem_prealloc: Insufficient free host memory " > - "pages available to allocate guest RAM\n"); > - exit(1); > + error_setg(errp, "os_mem_prealloc: Insufficient free host memory " > + "pages available to allocate guest RAM\n"); > } else { > int i; > size_t hpagesize = qemu_fd_getpagesize(fd); > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > for (i = 0; i < numpages; i++) { > memset(area + (hpagesize * i), 0, 1); > } > + } > > - ret = sigaction(SIGBUS, &oldact, NULL); > - if (ret) { > - perror("os_mem_prealloc: failed to reinstall signal handler"); > - exit(1); > - } > - > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); > + ret = sigaction(SIGBUS, &oldact, NULL); > + if (ret) { > + perror("os_mem_prealloc: failed to reinstall signal handler"); > + exit(1); I guess you're keeping the exit() here, because you can't recover cleanly from this error. I should never happen anyway. Worth a comment, I think. > } > + pthread_sigmask(SIG_SETMASK, &oldset, NULL); > } > > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index 6debc2b..4c1dcf1 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -539,7 +539,7 @@ int getpagesize(void) > return system_info.dwPageSize; > } > > -void os_mem_prealloc(int fd, char *area, size_t memory) > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > { > int i; > size_t pagesize = getpagesize(); Reviewed-by: Markus Armbruster <arm...@redhat.com>