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>
