* [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check
@ 2018-09-07 13:38 Fei Li
2018-09-07 13:38 ` [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails Fei Li
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Fei Li @ 2018-09-07 13:38 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, famz, armbru, eblake, fli
Hi,
This idea comes from BiteSizedTasks, and this patch series implement
the error checking of qemu_thread_create: make qemu_thread_create
return a flag to indicate if it succeeded rather than failing with an
error; make all callers check it.
The first three patches apply to those call traces whose further
callers already have the *errp to pass, thus add a new Error paramater
in the call trace to propagate the error and let the further caller
check it. The last patch modifies the qemu_thread_create() and makes
it return a bool to all direct callers to indicate if it succeeds.
v2:
- Pass errp straightly instead of using a local_err & error_propagate
- Return a bool: false/true to indicate if one function succeeds
- Merge v1's last two patches into one to avoid the compling error
- Fix one omitted error in patch1 and update some error messages
Please help to review. Thanks.
Fei Li (4):
Fix segmentation fault when qemu_signal_init fails
ui/vnc.c: polish vnc_init_func
qemu_init_vcpu: add a new Error parameter to propagate
qemu_thread_create: propagate the error to callers to handle
accel/tcg/user-exec-stub.c | 2 +-
cpus.c | 79 ++++++++++++++++++++++++++---------------
dump.c | 6 ++--
hw/misc/edu.c | 6 ++--
hw/ppc/spapr_hcall.c | 9 +++--
hw/rdma/rdma_backend.c | 3 +-
hw/usb/ccid-card-emulated.c | 13 ++++---
include/qemu/osdep.h | 2 +-
include/qemu/thread.h | 4 +--
include/qom/cpu.h | 2 +-
include/ui/console.h | 2 +-
io/task.c | 3 +-
iothread.c | 16 ++++++---
migration/migration.c | 49 +++++++++++++++++--------
migration/postcopy-ram.c | 12 +++++--
migration/ram.c | 40 +++++++++++++++------
migration/savevm.c | 11 ++++--
target/alpha/cpu.c | 4 ++-
target/arm/cpu.c | 4 ++-
target/cris/cpu.c | 4 ++-
target/hppa/cpu.c | 4 ++-
target/i386/cpu.c | 4 ++-
target/lm32/cpu.c | 4 ++-
target/m68k/cpu.c | 4 ++-
target/microblaze/cpu.c | 4 ++-
target/mips/cpu.c | 4 ++-
target/moxie/cpu.c | 4 ++-
target/nios2/cpu.c | 4 ++-
target/openrisc/cpu.c | 4 ++-
target/ppc/translate_init.inc.c | 4 ++-
target/riscv/cpu.c | 4 ++-
target/s390x/cpu.c | 4 ++-
target/sh4/cpu.c | 4 ++-
target/sparc/cpu.c | 4 ++-
target/tilegx/cpu.c | 4 ++-
target/tricore/cpu.c | 4 ++-
target/unicore32/cpu.c | 4 ++-
target/xtensa/cpu.c | 4 ++-
tests/atomic_add-bench.c | 3 +-
tests/iothread.c | 2 +-
tests/qht-bench.c | 3 +-
tests/rcutorture.c | 3 +-
tests/test-aio.c | 2 +-
tests/test-rcu-list.c | 3 +-
ui/vnc-jobs.c | 17 ++++++---
ui/vnc-jobs.h | 2 +-
ui/vnc.c | 12 +++++--
util/compatfd.c | 16 ++++++---
util/main-loop.c | 10 +++---
util/oslib-posix.c | 12 +++++--
util/qemu-thread-posix.c | 18 ++++++----
util/qemu-thread-win32.c | 15 +++++---
util/rcu.c | 3 +-
util/thread-pool.c | 4 ++-
54 files changed, 325 insertions(+), 143 deletions(-)
--
2.13.7
^ permalink raw reply [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails 2018-09-07 13:38 [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check Fei Li @ 2018-09-07 13:38 ` Fei Li 2018-09-12 7:55 ` Fam Zheng 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func Fei Li ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Fei Li @ 2018-09-07 13:38 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, famz, armbru, eblake, fli Currently, when qemu_signal_init() fails it only returns a non-zero value but without propagating any Error. But its callers need a non-null err when runs error_report_err(err), or else 0->msg occurs. To avoid such segmentation fault, add a new Error parameter to make the call trace to propagate the err to the final caller. This patch also adds the omitted error handling when creating signalfd pipe fails in qemu_signalfd_compat(). Signed-off-by: Fei Li <fli@suse.com> --- include/qemu/osdep.h | 2 +- util/compatfd.c | 9 ++++++--- util/main-loop.c | 10 +++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index a91068df0e..09ed85fcb8 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo { additional fields in the future) */ }; -int qemu_signalfd(const sigset_t *mask); +int qemu_signalfd(const sigset_t *mask, Error **errp); void sigaction_invoke(struct sigaction *action, struct qemu_signalfd_siginfo *info); #endif diff --git a/util/compatfd.c b/util/compatfd.c index 980bd33e52..d3ed890405 100644 --- a/util/compatfd.c +++ b/util/compatfd.c @@ -16,6 +16,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" #include "qemu/thread.h" +#include "qapi/error.h" #include <sys/syscall.h> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque) } } -static int qemu_signalfd_compat(const sigset_t *mask) +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) { struct sigfd_compat_info *info; QemuThread thread; @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) info = malloc(sizeof(*info)); if (info == NULL) { + error_setg(errp, "Failed to allocate signalfd memory"); errno = ENOMEM; return -1; } if (pipe(fds) == -1) { + error_setg(errp, "Failed to create signalfd pipe"); free(info); return -1; } @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) return fds[0]; } -int qemu_signalfd(const sigset_t *mask) +int qemu_signalfd(const sigset_t *mask, Error **errp) { #if defined(CONFIG_SIGNALFD) int ret; @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask) } #endif - return qemu_signalfd_compat(mask); + return qemu_signalfd_compat(mask, errp); } diff --git a/util/main-loop.c b/util/main-loop.c index affe0403c5..22aa2b1007 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque) } } -static int qemu_signal_init(void) +static int qemu_signal_init(Error **errp) { int sigfd; sigset_t set; @@ -94,10 +94,10 @@ static int qemu_signal_init(void) pthread_sigmask(SIG_BLOCK, &set, NULL); sigdelset(&set, SIG_IPI); - sigfd = qemu_signalfd(&set); + sigfd = qemu_signalfd(&set, errp); if (sigfd == -1) { fprintf(stderr, "failed to create signalfd\n"); - return -errno; + return -1; } fcntl_setfl(sigfd, O_NONBLOCK); @@ -109,7 +109,7 @@ static int qemu_signal_init(void) #else /* _WIN32 */ -static int qemu_signal_init(void) +static int qemu_signal_init(Error **errp) { return 0; } @@ -148,7 +148,7 @@ int qemu_init_main_loop(Error **errp) init_clocks(qemu_timer_notify_cb); - ret = qemu_signal_init(); + ret = qemu_signal_init(errp); if (ret) { return ret; } -- 2.13.7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails 2018-09-07 13:38 ` [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails Fei Li @ 2018-09-12 7:55 ` Fam Zheng 2018-09-13 8:46 ` Fei Li 0 siblings, 1 reply; 12+ messages in thread From: Fam Zheng @ 2018-09-12 7:55 UTC (permalink / raw) To: Fei Li; +Cc: qemu-devel, berrange, armbru, eblake On Fri, 09/07 21:38, Fei Li wrote: > Currently, when qemu_signal_init() fails it only returns a non-zero > value but without propagating any Error. But its callers need a > non-null err when runs error_report_err(err), or else 0->msg occurs. > > To avoid such segmentation fault, add a new Error parameter to make > the call trace to propagate the err to the final caller. > > This patch also adds the omitted error handling when creating signalfd > pipe fails in qemu_signalfd_compat(). > > Signed-off-by: Fei Li <fli@suse.com> > --- > include/qemu/osdep.h | 2 +- > util/compatfd.c | 9 ++++++--- > util/main-loop.c | 10 +++++----- > 3 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index a91068df0e..09ed85fcb8 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo { > additional fields in the future) */ > }; > > -int qemu_signalfd(const sigset_t *mask); > +int qemu_signalfd(const sigset_t *mask, Error **errp); > void sigaction_invoke(struct sigaction *action, > struct qemu_signalfd_siginfo *info); > #endif > diff --git a/util/compatfd.c b/util/compatfd.c > index 980bd33e52..d3ed890405 100644 > --- a/util/compatfd.c > +++ b/util/compatfd.c > @@ -16,6 +16,7 @@ > #include "qemu/osdep.h" > #include "qemu-common.h" > #include "qemu/thread.h" > +#include "qapi/error.h" > > #include <sys/syscall.h> > > @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque) > } > } > > -static int qemu_signalfd_compat(const sigset_t *mask) > +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) > { > struct sigfd_compat_info *info; > QemuThread thread; > @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) > > info = malloc(sizeof(*info)); > if (info == NULL) { > + error_setg(errp, "Failed to allocate signalfd memory"); > errno = ENOMEM; > return -1; > } > > if (pipe(fds) == -1) { > + error_setg(errp, "Failed to create signalfd pipe"); > free(info); > return -1; > } > @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) > return fds[0]; > } > > -int qemu_signalfd(const sigset_t *mask) > +int qemu_signalfd(const sigset_t *mask, Error **errp) > { > #if defined(CONFIG_SIGNALFD) > int ret; Extend the context: ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); if (ret != -1) { qemu_set_cloexec(ret); return ret; } This error path need error_setg() as well. > @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask) > } > #endif > > - return qemu_signalfd_compat(mask); > + return qemu_signalfd_compat(mask, errp); > } > diff --git a/util/main-loop.c b/util/main-loop.c > index affe0403c5..22aa2b1007 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque) > } > } > > -static int qemu_signal_init(void) > +static int qemu_signal_init(Error **errp) > { > int sigfd; > sigset_t set; > @@ -94,10 +94,10 @@ static int qemu_signal_init(void) > pthread_sigmask(SIG_BLOCK, &set, NULL); > > sigdelset(&set, SIG_IPI); > - sigfd = qemu_signalfd(&set); > + sigfd = qemu_signalfd(&set, errp); > if (sigfd == -1) { > fprintf(stderr, "failed to create signalfd\n"); This fprintf is not needed any more. > - return -errno; > + return -1; > } > > fcntl_setfl(sigfd, O_NONBLOCK); > @@ -109,7 +109,7 @@ static int qemu_signal_init(void) > > #else /* _WIN32 */ > > -static int qemu_signal_init(void) > +static int qemu_signal_init(Error **errp) > { > return 0; > } > @@ -148,7 +148,7 @@ int qemu_init_main_loop(Error **errp) > > init_clocks(qemu_timer_notify_cb); > > - ret = qemu_signal_init(); > + ret = qemu_signal_init(errp); > if (ret) { > return ret; > } > -- > 2.13.7 > Fam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails 2018-09-12 7:55 ` Fam Zheng @ 2018-09-13 8:46 ` Fei Li 2018-09-13 8:58 ` Fam Zheng 0 siblings, 1 reply; 12+ messages in thread From: Fei Li @ 2018-09-13 8:46 UTC (permalink / raw) To: Fam Zheng; +Cc: qemu-devel, berrange, armbru, eblake On 09/12/2018 03:55 PM, Fam Zheng wrote: > On Fri, 09/07 21:38, Fei Li wrote: >> Currently, when qemu_signal_init() fails it only returns a non-zero >> value but without propagating any Error. But its callers need a >> non-null err when runs error_report_err(err), or else 0->msg occurs. >> >> To avoid such segmentation fault, add a new Error parameter to make >> the call trace to propagate the err to the final caller. >> >> This patch also adds the omitted error handling when creating signalfd >> pipe fails in qemu_signalfd_compat(). >> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> include/qemu/osdep.h | 2 +- >> util/compatfd.c | 9 ++++++--- >> util/main-loop.c | 10 +++++----- >> 3 files changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h >> index a91068df0e..09ed85fcb8 100644 >> --- a/include/qemu/osdep.h >> +++ b/include/qemu/osdep.h >> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo { >> additional fields in the future) */ >> }; >> >> -int qemu_signalfd(const sigset_t *mask); >> +int qemu_signalfd(const sigset_t *mask, Error **errp); >> void sigaction_invoke(struct sigaction *action, >> struct qemu_signalfd_siginfo *info); >> #endif >> diff --git a/util/compatfd.c b/util/compatfd.c >> index 980bd33e52..d3ed890405 100644 >> --- a/util/compatfd.c >> +++ b/util/compatfd.c >> @@ -16,6 +16,7 @@ >> #include "qemu/osdep.h" >> #include "qemu-common.h" >> #include "qemu/thread.h" >> +#include "qapi/error.h" >> >> #include <sys/syscall.h> >> >> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque) >> } >> } >> >> -static int qemu_signalfd_compat(const sigset_t *mask) >> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) >> { >> struct sigfd_compat_info *info; >> QemuThread thread; >> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) >> >> info = malloc(sizeof(*info)); >> if (info == NULL) { >> + error_setg(errp, "Failed to allocate signalfd memory"); >> errno = ENOMEM; >> return -1; >> } >> >> if (pipe(fds) == -1) { >> + error_setg(errp, "Failed to create signalfd pipe"); >> free(info); >> return -1; >> } >> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) >> return fds[0]; >> } >> >> -int qemu_signalfd(const sigset_t *mask) >> +int qemu_signalfd(const sigset_t *mask, Error **errp) >> { >> #if defined(CONFIG_SIGNALFD) >> int ret; > Extend the context: > > ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); > if (ret != -1) { > qemu_set_cloexec(ret); > return ret; > } > > This error path need error_setg() as well. Thanks for the comment. :) Like adding as below? if (ret != -1) { qemu_set_cloexec(ret); return ret; + } else { + error_setg(errp, "syscall SYS_signalfd failed: %s", strerror(errno)); } #endif return qemu_signalfd_compat(mask, errp); If yes, I'd like to confirm the logic: - 1.1 if the syscall() succeeds, return its value which is not -1: none error; - if fails[1], continue to run the below "qemu_signalfd_compat(mask, errp);" =2.1 if qemu_signalfd_compat() succeeds, the error msg of [1] will be kept in **errp, although the return value indicates a success; =2.2 if qemu_signalfd_compat() fails, the error msg[1] is replaced by another error[2] in the failing function which indicates a failure. Do you mean for the above 2.1, we still need keep the error message? > >> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask) >> } >> #endif >> >> - return qemu_signalfd_compat(mask); >> + return qemu_signalfd_compat(mask, errp); >> } >> diff --git a/util/main-loop.c b/util/main-loop.c >> index affe0403c5..22aa2b1007 100644 >> --- a/util/main-loop.c >> +++ b/util/main-loop.c >> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque) >> } >> } >> >> -static int qemu_signal_init(void) >> +static int qemu_signal_init(Error **errp) >> { >> int sigfd; >> sigset_t set; >> @@ -94,10 +94,10 @@ static int qemu_signal_init(void) >> pthread_sigmask(SIG_BLOCK, &set, NULL); >> >> sigdelset(&set, SIG_IPI); >> - sigfd = qemu_signalfd(&set); >> + sigfd = qemu_signalfd(&set, errp); >> if (sigfd == -1) { >> fprintf(stderr, "failed to create signalfd\n"); > This fprintf is not needed any more. Ok. Have a nice day Fei > >> - return -errno; >> + return -1; >> } >> >> fcntl_setfl(sigfd, O_NONBLOCK); >> @@ -109,7 +109,7 @@ static int qemu_signal_init(void) >> >> #else /* _WIN32 */ >> >> -static int qemu_signal_init(void) >> +static int qemu_signal_init(Error **errp) >> { >> return 0; >> } >> @@ -148,7 +148,7 @@ int qemu_init_main_loop(Error **errp) >> >> init_clocks(qemu_timer_notify_cb); >> >> - ret = qemu_signal_init(); >> + ret = qemu_signal_init(errp); >> if (ret) { >> return ret; >> } >> -- >> 2.13.7 >> > Fam > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails 2018-09-13 8:46 ` Fei Li @ 2018-09-13 8:58 ` Fam Zheng 0 siblings, 0 replies; 12+ messages in thread From: Fam Zheng @ 2018-09-13 8:58 UTC (permalink / raw) To: Fei Li; +Cc: qemu-devel, berrange, armbru, eblake On Thu, 09/13 16:46, Fei Li wrote: > > > On 09/12/2018 03:55 PM, Fam Zheng wrote: > > On Fri, 09/07 21:38, Fei Li wrote: > > > Currently, when qemu_signal_init() fails it only returns a non-zero > > > value but without propagating any Error. But its callers need a > > > non-null err when runs error_report_err(err), or else 0->msg occurs. > > > > > > To avoid such segmentation fault, add a new Error parameter to make > > > the call trace to propagate the err to the final caller. > > > > > > This patch also adds the omitted error handling when creating signalfd > > > pipe fails in qemu_signalfd_compat(). > > > > > > Signed-off-by: Fei Li <fli@suse.com> > > > --- > > > include/qemu/osdep.h | 2 +- > > > util/compatfd.c | 9 ++++++--- > > > util/main-loop.c | 10 +++++----- > > > 3 files changed, 12 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > index a91068df0e..09ed85fcb8 100644 > > > --- a/include/qemu/osdep.h > > > +++ b/include/qemu/osdep.h > > > @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo { > > > additional fields in the future) */ > > > }; > > > -int qemu_signalfd(const sigset_t *mask); > > > +int qemu_signalfd(const sigset_t *mask, Error **errp); > > > void sigaction_invoke(struct sigaction *action, > > > struct qemu_signalfd_siginfo *info); > > > #endif > > > diff --git a/util/compatfd.c b/util/compatfd.c > > > index 980bd33e52..d3ed890405 100644 > > > --- a/util/compatfd.c > > > +++ b/util/compatfd.c > > > @@ -16,6 +16,7 @@ > > > #include "qemu/osdep.h" > > > #include "qemu-common.h" > > > #include "qemu/thread.h" > > > +#include "qapi/error.h" > > > #include <sys/syscall.h> > > > @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque) > > > } > > > } > > > -static int qemu_signalfd_compat(const sigset_t *mask) > > > +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) > > > { > > > struct sigfd_compat_info *info; > > > QemuThread thread; > > > @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) > > > info = malloc(sizeof(*info)); > > > if (info == NULL) { > > > + error_setg(errp, "Failed to allocate signalfd memory"); > > > errno = ENOMEM; > > > return -1; > > > } > > > if (pipe(fds) == -1) { > > > + error_setg(errp, "Failed to create signalfd pipe"); > > > free(info); > > > return -1; > > > } > > > @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) > > > return fds[0]; > > > } > > > -int qemu_signalfd(const sigset_t *mask) > > > +int qemu_signalfd(const sigset_t *mask, Error **errp) > > > { > > > #if defined(CONFIG_SIGNALFD) > > > int ret; > > Extend the context: > > > > ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); > > if (ret != -1) { > > qemu_set_cloexec(ret); > > return ret; > > } > > > > This error path need error_setg() as well. > Thanks for the comment. :) > Like adding as below? > > if (ret != -1) { > qemu_set_cloexec(ret); > return ret; > + } else { > + error_setg(errp, "syscall SYS_signalfd failed: %s", > strerror(errno)); > } > #endif > > return qemu_signalfd_compat(mask, errp); > > > If yes, I'd like to confirm the logic: Oh, I missed the fact that qemu_signalfd_compat is a fallback if qemu_set_cloexec() fails. So no, your original patch is good, ignore what I said, there's no need to add the extra error_setg here. If you do so and qemu_signalfd_compat() fails, error_setg() in it will hit the assertion failure. Fam ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func 2018-09-07 13:38 [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check Fei Li 2018-09-07 13:38 ` [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails Fei Li @ 2018-09-07 13:39 ` Fei Li 2018-09-12 7:57 ` Fam Zheng 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 3/4] qemu_init_vcpu: add a new Error parameter to propagate Fei Li 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle Fei Li 3 siblings, 1 reply; 12+ messages in thread From: Fei Li @ 2018-09-07 13:39 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, famz, armbru, eblake, fli Add a new Error parameter for vnc_display_init() to handle errors in its caller: vnc_init_func(), just like vnc_display_open() does. And let the call trace propagate the Error. Besides, make vnc_start_worker_thread() return a bool to indicate whether it succeeds instead of returning nothing. Signed-off-by: Fei Li <fli@suse.com> --- include/ui/console.h | 2 +- ui/vnc-jobs.c | 9 ++++++--- ui/vnc-jobs.h | 2 +- ui/vnc.c | 12 +++++++++--- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/include/ui/console.h b/include/ui/console.h index fb969caf70..c17803c530 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts); void qemu_display_init(DisplayState *ds, DisplayOptions *opts); /* vnc.c */ -void vnc_display_init(const char *id); +void vnc_display_init(const char *id, Error **errp); void vnc_display_open(const char *id, Error **errp); void vnc_display_add_client(const char *id, int csock, bool skipauth); int vnc_display_password(const char *id, const char *password); diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c index 929391f85d..8807d7217c 100644 --- a/ui/vnc-jobs.c +++ b/ui/vnc-jobs.c @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void) return queue; /* Check global queue */ } -void vnc_start_worker_thread(void) +bool vnc_start_worker_thread(Error **errp) { VncJobQueue *q; - if (vnc_worker_thread_running()) - return ; + if (vnc_worker_thread_running()) { + goto out; + } q = vnc_queue_init(); qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, QEMU_THREAD_DETACHED); queue = q; /* Set global queue */ +out: + return true; } diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h index 59f66bcc35..14640593db 100644 --- a/ui/vnc-jobs.h +++ b/ui/vnc-jobs.h @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job); void vnc_jobs_join(VncState *vs); void vnc_jobs_consume_buffer(VncState *vs); -void vnc_start_worker_thread(void); +bool vnc_start_worker_thread(Error **errp); /* Locks */ static inline int vnc_trylock_display(VncDisplay *vd) diff --git a/ui/vnc.c b/ui/vnc.c index ccb1335d86..f632635ea4 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3206,7 +3206,7 @@ static const DisplayChangeListenerOps dcl_ops = { .dpy_cursor_define = vnc_dpy_cursor_define, }; -void vnc_display_init(const char *id) +void vnc_display_init(const char *id, Error **errp) { VncDisplay *vd; @@ -3236,7 +3236,9 @@ void vnc_display_init(const char *id) vd->connections_limit = 32; qemu_mutex_init(&vd->mutex); - vnc_start_worker_thread(); + if (!vnc_start_worker_thread(errp)) { + return; + } vd->dcl.ops = &dcl_ops; register_displaychangelistener(&vd->dcl); @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) char *id = (char *)qemu_opts_id(opts); assert(id); - vnc_display_init(id); + vnc_display_init(id, &local_err); + if (local_err) { + error_reportf_err(local_err, "Failed to init VNC server: "); + exit(1); + } vnc_display_open(id, &local_err); if (local_err != NULL) { error_reportf_err(local_err, "Failed to start VNC server: "); -- 2.13.7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func Fei Li @ 2018-09-12 7:57 ` Fam Zheng 2018-09-13 9:03 ` Fei Li 0 siblings, 1 reply; 12+ messages in thread From: Fam Zheng @ 2018-09-12 7:57 UTC (permalink / raw) To: Fei Li; +Cc: qemu-devel, berrange, armbru, eblake On Fri, 09/07 21:39, Fei Li wrote: > Add a new Error parameter for vnc_display_init() to handle errors > in its caller: vnc_init_func(), just like vnc_display_open() does. > And let the call trace propagate the Error. > > Besides, make vnc_start_worker_thread() return a bool to indicate > whether it succeeds instead of returning nothing. > > Signed-off-by: Fei Li <fli@suse.com> > --- > include/ui/console.h | 2 +- > ui/vnc-jobs.c | 9 ++++++--- > ui/vnc-jobs.h | 2 +- > ui/vnc.c | 12 +++++++++--- > 4 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/include/ui/console.h b/include/ui/console.h > index fb969caf70..c17803c530 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts); > void qemu_display_init(DisplayState *ds, DisplayOptions *opts); > > /* vnc.c */ > -void vnc_display_init(const char *id); > +void vnc_display_init(const char *id, Error **errp); > void vnc_display_open(const char *id, Error **errp); > void vnc_display_add_client(const char *id, int csock, bool skipauth); > int vnc_display_password(const char *id, const char *password); > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c > index 929391f85d..8807d7217c 100644 > --- a/ui/vnc-jobs.c > +++ b/ui/vnc-jobs.c > @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void) > return queue; /* Check global queue */ > } > > -void vnc_start_worker_thread(void) > +bool vnc_start_worker_thread(Error **errp) > { > VncJobQueue *q; > > - if (vnc_worker_thread_running()) > - return ; > + if (vnc_worker_thread_running()) { > + goto out; > + } > > q = vnc_queue_init(); > qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, > QEMU_THREAD_DETACHED); > queue = q; /* Set global queue */ > +out: > + return true; This function always returns true? > } > diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h > index 59f66bcc35..14640593db 100644 > --- a/ui/vnc-jobs.h > +++ b/ui/vnc-jobs.h > @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job); > void vnc_jobs_join(VncState *vs); > > void vnc_jobs_consume_buffer(VncState *vs); > -void vnc_start_worker_thread(void); > +bool vnc_start_worker_thread(Error **errp); > > /* Locks */ > static inline int vnc_trylock_display(VncDisplay *vd) > diff --git a/ui/vnc.c b/ui/vnc.c > index ccb1335d86..f632635ea4 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -3206,7 +3206,7 @@ static const DisplayChangeListenerOps dcl_ops = { > .dpy_cursor_define = vnc_dpy_cursor_define, > }; > > -void vnc_display_init(const char *id) > +void vnc_display_init(const char *id, Error **errp) > { > VncDisplay *vd; > > @@ -3236,7 +3236,9 @@ void vnc_display_init(const char *id) > vd->connections_limit = 32; > > qemu_mutex_init(&vd->mutex); > - vnc_start_worker_thread(); > + if (!vnc_start_worker_thread(errp)) { > + return; > + } > > vd->dcl.ops = &dcl_ops; > register_displaychangelistener(&vd->dcl); > @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) > char *id = (char *)qemu_opts_id(opts); > > assert(id); > - vnc_display_init(id); > + vnc_display_init(id, &local_err); > + if (local_err) { > + error_reportf_err(local_err, "Failed to init VNC server: "); > + exit(1); > + } > vnc_display_open(id, &local_err); > if (local_err != NULL) { > error_reportf_err(local_err, "Failed to start VNC server: "); > -- > 2.13.7 > Fam ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func 2018-09-12 7:57 ` Fam Zheng @ 2018-09-13 9:03 ` Fei Li 0 siblings, 0 replies; 12+ messages in thread From: Fei Li @ 2018-09-13 9:03 UTC (permalink / raw) To: Fam Zheng; +Cc: qemu-devel, berrange, armbru, eblake On 09/12/2018 03:57 PM, Fam Zheng wrote: > On Fri, 09/07 21:39, Fei Li wrote: >> Add a new Error parameter for vnc_display_init() to handle errors >> in its caller: vnc_init_func(), just like vnc_display_open() does. >> And let the call trace propagate the Error. >> >> Besides, make vnc_start_worker_thread() return a bool to indicate >> whether it succeeds instead of returning nothing. >> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> include/ui/console.h | 2 +- >> ui/vnc-jobs.c | 9 ++++++--- >> ui/vnc-jobs.h | 2 +- >> ui/vnc.c | 12 +++++++++--- >> 4 files changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/include/ui/console.h b/include/ui/console.h >> index fb969caf70..c17803c530 100644 >> --- a/include/ui/console.h >> +++ b/include/ui/console.h >> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts); >> void qemu_display_init(DisplayState *ds, DisplayOptions *opts); >> >> /* vnc.c */ >> -void vnc_display_init(const char *id); >> +void vnc_display_init(const char *id, Error **errp); >> void vnc_display_open(const char *id, Error **errp); >> void vnc_display_add_client(const char *id, int csock, bool skipauth); >> int vnc_display_password(const char *id, const char *password); >> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c >> index 929391f85d..8807d7217c 100644 >> --- a/ui/vnc-jobs.c >> +++ b/ui/vnc-jobs.c >> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void) >> return queue; /* Check global queue */ >> } >> >> -void vnc_start_worker_thread(void) >> +bool vnc_start_worker_thread(Error **errp) >> { >> VncJobQueue *q; >> >> - if (vnc_worker_thread_running()) >> - return ; >> + if (vnc_worker_thread_running()) { >> + goto out; >> + } >> >> q = vnc_queue_init(); >> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, >> QEMU_THREAD_DETACHED); >> queue = q; /* Set global queue */ >> +out: >> + return true; > This function always returns true? Yes, until checking qemu_thread_create()'s return value. Before applying the checking code, qemu_thread_create still keeps abort() on failure. Have a nice day, thanks Fei > >> } >> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h >> index 59f66bcc35..14640593db 100644 >> --- a/ui/vnc-jobs.h >> +++ b/ui/vnc-jobs.h >> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job); >> void vnc_jobs_join(VncState *vs); >> >> void vnc_jobs_consume_buffer(VncState *vs); >> -void vnc_start_worker_thread(void); >> +bool vnc_start_worker_thread(Error **errp); >> >> /* Locks */ >> static inline int vnc_trylock_display(VncDisplay *vd) >> diff --git a/ui/vnc.c b/ui/vnc.c >> index ccb1335d86..f632635ea4 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -3206,7 +3206,7 @@ static const DisplayChangeListenerOps dcl_ops = { >> .dpy_cursor_define = vnc_dpy_cursor_define, >> }; >> >> -void vnc_display_init(const char *id) >> +void vnc_display_init(const char *id, Error **errp) >> { >> VncDisplay *vd; >> >> @@ -3236,7 +3236,9 @@ void vnc_display_init(const char *id) >> vd->connections_limit = 32; >> >> qemu_mutex_init(&vd->mutex); >> - vnc_start_worker_thread(); >> + if (!vnc_start_worker_thread(errp)) { >> + return; >> + } >> >> vd->dcl.ops = &dcl_ops; >> register_displaychangelistener(&vd->dcl); >> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp) >> char *id = (char *)qemu_opts_id(opts); >> >> assert(id); >> - vnc_display_init(id); >> + vnc_display_init(id, &local_err); >> + if (local_err) { >> + error_reportf_err(local_err, "Failed to init VNC server: "); >> + exit(1); >> + } >> vnc_display_open(id, &local_err); >> if (local_err != NULL) { >> error_reportf_err(local_err, "Failed to start VNC server: "); >> -- >> 2.13.7 >> > Fam > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] qemu_init_vcpu: add a new Error parameter to propagate 2018-09-07 13:38 [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check Fei Li 2018-09-07 13:38 ` [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails Fei Li 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func Fei Li @ 2018-09-07 13:39 ` Fei Li 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle Fei Li 3 siblings, 0 replies; 12+ messages in thread From: Fei Li @ 2018-09-07 13:39 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, famz, armbru, eblake, fli The caller of qemu_init_vcpu() already passed the **errp to handle errors. In view of this, add a new Error parameter to the following call trace to propagate the error and let the further caller check it. Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds. Signed-off-by: Fei Li <fli@suse.com> --- accel/tcg/user-exec-stub.c | 2 +- cpus.c | 34 +++++++++++++++++++++------------- include/qom/cpu.h | 2 +- target/alpha/cpu.c | 4 +++- target/arm/cpu.c | 4 +++- target/cris/cpu.c | 4 +++- target/hppa/cpu.c | 4 +++- target/i386/cpu.c | 4 +++- target/lm32/cpu.c | 4 +++- target/m68k/cpu.c | 4 +++- target/microblaze/cpu.c | 4 +++- target/mips/cpu.c | 4 +++- target/moxie/cpu.c | 4 +++- target/nios2/cpu.c | 4 +++- target/openrisc/cpu.c | 4 +++- target/ppc/translate_init.inc.c | 4 +++- target/riscv/cpu.c | 4 +++- target/s390x/cpu.c | 4 +++- target/sh4/cpu.c | 4 +++- target/sparc/cpu.c | 4 +++- target/tilegx/cpu.c | 4 +++- target/tricore/cpu.c | 4 +++- target/unicore32/cpu.c | 4 +++- target/xtensa/cpu.c | 4 +++- 24 files changed, 86 insertions(+), 36 deletions(-) diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c index a32b4496af..38f6b928d4 100644 --- a/accel/tcg/user-exec-stub.c +++ b/accel/tcg/user-exec-stub.c @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu) { } -void qemu_init_vcpu(CPUState *cpu) +bool qemu_init_vcpu(CPUState *cpu, Error **errp) { } diff --git a/cpus.c b/cpus.c index 8ee6e5db93..1feb308123 100644 --- a/cpus.c +++ b/cpus.c @@ -1898,7 +1898,7 @@ void cpu_remove_sync(CPUState *cpu) /* For temporary buffers for forming a name */ #define VCPU_THREAD_NAME_SIZE 16 -static void qemu_tcg_init_vcpu(CPUState *cpu) +static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; static QemuCond *single_tcg_halt_cond; @@ -1954,7 +1954,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu) } } -static void qemu_hax_start_vcpu(CPUState *cpu) +static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -1971,7 +1971,7 @@ static void qemu_hax_start_vcpu(CPUState *cpu) #endif } -static void qemu_kvm_start_vcpu(CPUState *cpu) +static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -1984,7 +1984,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu) cpu, QEMU_THREAD_JOINABLE); } -static void qemu_hvf_start_vcpu(CPUState *cpu) +static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -2002,7 +2002,7 @@ static void qemu_hvf_start_vcpu(CPUState *cpu) cpu, QEMU_THREAD_JOINABLE); } -static void qemu_whpx_start_vcpu(CPUState *cpu) +static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -2018,7 +2018,7 @@ static void qemu_whpx_start_vcpu(CPUState *cpu) #endif } -static void qemu_dummy_start_vcpu(CPUState *cpu) +static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -2031,11 +2031,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu) QEMU_THREAD_JOINABLE); } -void qemu_init_vcpu(CPUState *cpu) +bool qemu_init_vcpu(CPUState *cpu, Error **errp) { cpu->nr_cores = smp_cores; cpu->nr_threads = smp_threads; cpu->stopped = true; + Error *local_err = NULL; if (!cpu->as) { /* If the target cpu hasn't set up any address spaces itself, @@ -2046,22 +2047,29 @@ void qemu_init_vcpu(CPUState *cpu) } if (kvm_enabled()) { - qemu_kvm_start_vcpu(cpu); + qemu_kvm_start_vcpu(cpu, &local_err); } else if (hax_enabled()) { - qemu_hax_start_vcpu(cpu); + qemu_hax_start_vcpu(cpu, &local_err); } else if (hvf_enabled()) { - qemu_hvf_start_vcpu(cpu); + qemu_hvf_start_vcpu(cpu, &local_err); } else if (tcg_enabled()) { - qemu_tcg_init_vcpu(cpu); + qemu_tcg_init_vcpu(cpu, &local_err); } else if (whpx_enabled()) { - qemu_whpx_start_vcpu(cpu); + qemu_whpx_start_vcpu(cpu, &local_err); } else { - qemu_dummy_start_vcpu(cpu); + qemu_dummy_start_vcpu(cpu, &local_err); + } + + if (local_err) { + error_propagate(errp, local_err); + return false; } while (!cpu->created) { qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); } + + return true; } void cpu_stop_current(void) diff --git a/include/qom/cpu.h b/include/qom/cpu.h index dc130cd307..4d85dda175 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -1012,7 +1012,7 @@ void end_exclusive(void); * * Initializes a vCPU. */ -void qemu_init_vcpu(CPUState *cpu); +bool qemu_init_vcpu(CPUState *cpu, Error **errp); #define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */ #define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */ diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c index b08078e7fc..d531bd4f7e 100644 --- a/target/alpha/cpu.c +++ b/target/alpha/cpu.c @@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } acc->parent_realize(dev, errp); } diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 258ba6dcaa..d2ad17fbca 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1028,7 +1028,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } #endif - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } cpu_reset(cs); acc->parent_realize(dev, errp); diff --git a/target/cris/cpu.c b/target/cris/cpu.c index a23aba2688..ec92d69781 100644 --- a/target/cris/cpu.c +++ b/target/cris/cpu.c @@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) } cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } ccc->parent_realize(dev, errp); } diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c index 00bf444620..08f600ced9 100644 --- a/target/hppa/cpu.c +++ b/target/hppa/cpu.c @@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } acc->parent_realize(dev, errp); #ifndef CONFIG_USER_ONLY diff --git a/target/i386/cpu.c b/target/i386/cpu.c index f24295e6e4..fad65766a4 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) } #endif - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } /* * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c index b7499cb627..d50b1e4a43 100644 --- a/target/lm32/cpu.c +++ b/target/lm32/cpu.c @@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp) cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } lcc->parent_realize(dev, errp); } diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c index 582e3a73b3..4ab53f2d58 100644 --- a/target/m68k/cpu.c +++ b/target/m68k/cpu.c @@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp) m68k_cpu_init_gdb(cpu); cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } mcc->parent_realize(dev, errp); } diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index 9b546a2c18..3906c864a3 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } env->pvr.regs[0] = PVR0_USE_EXC_MASK \ | PVR0_USE_ICACHE_MASK \ diff --git a/target/mips/cpu.c b/target/mips/cpu.c index 497706b669..62be84af76 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) cpu_mips_realize_env(&cpu->env); cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } mcc->parent_realize(dev, errp); } diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c index 8d67eb6727..8581a6d922 100644 --- a/target/moxie/cpu.c +++ b/target/moxie/cpu.c @@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } cpu_reset(cs); mcc->parent_realize(dev, errp); diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c index fbfaa2ce26..5c7b4b486e 100644 --- a/target/nios2/cpu.c +++ b/target/nios2/cpu.c @@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } cpu_reset(cs); ncc->parent_realize(dev, errp); diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c index fb7cb5c507..a6dcdb9df9 100644 --- a/target/openrisc/cpu.c +++ b/target/openrisc/cpu.c @@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } cpu_reset(cs); occ->parent_realize(dev, errp); diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c index d920d3e538..7b47066599 100644 --- a/target/ppc/translate_init.inc.c +++ b/target/ppc/translate_init.inc.c @@ -9707,7 +9707,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp) 32, "power-vsx.xml", 0); } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + goto unrealize; + } pcc->parent_realize(dev, errp); diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index d630e8fd6c..ef56215e92 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } cpu_reset(cs); mcc->parent_realize(dev, errp); diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 8ed4823d6e..3a665c4df8 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -217,7 +217,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) qemu_register_reset(s390_cpu_machine_reset_cb, cpu); #endif s390_cpu_gdb_init(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } /* * KVM requires the initial CPU reset ioctl to be executed on the target diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c index b9f393b7c7..d32ef2e1cb 100644 --- a/target/sh4/cpu.c +++ b/target/sh4/cpu.c @@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) } cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } scc->parent_realize(dev, errp); } diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index 0f090ece54..9c22f6a7df 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } scc->parent_realize(dev, errp); } diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c index bfe9be59b5..234148fabd 100644 --- a/target/tilegx/cpu.c +++ b/target/tilegx/cpu.c @@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp) } cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } tcc->parent_realize(dev, errp); } diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c index 2edaef1aef..5482d6ea3f 100644 --- a/target/tricore/cpu.c +++ b/target/tricore/cpu.c @@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp) set_feature(env, TRICORE_FEATURE_13); } cpu_reset(cs); - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } tcc->parent_realize(dev, errp); } diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c index 68f978d80b..1f6a33b6f3 100644 --- a/target/unicore32/cpu.c +++ b/target/unicore32/cpu.c @@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } ucc->parent_realize(dev, errp); } diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c index 590813d4f7..177694454f 100644 --- a/target/xtensa/cpu.c +++ b/target/xtensa/cpu.c @@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp) cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs; - qemu_init_vcpu(cs); + if (!qemu_init_vcpu(cs, errp)) { + return; + } xcc->parent_realize(dev, errp); } -- 2.13.7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle 2018-09-07 13:38 [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check Fei Li ` (2 preceding siblings ...) 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 3/4] qemu_init_vcpu: add a new Error parameter to propagate Fei Li @ 2018-09-07 13:39 ` Fei Li 2018-09-12 8:20 ` Fam Zheng 3 siblings, 1 reply; 12+ messages in thread From: Fei Li @ 2018-09-07 13:39 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, famz, armbru, eblake, fli Make qemu_thread_create() return a Boolean to indicate if it succeeds rather than failing with an error. And add an Error parameter to hold the error message and let the callers handle it. Besides, directly return if thread->data is NULL to avoid the segmentation fault in qemu_thread_join in qemu-thread-win32.c. Signed-off-by: Fei Li <fli@suse.com> --- cpus.c | 45 +++++++++++++++++++++++++++-------------- dump.c | 6 ++++-- hw/misc/edu.c | 6 ++++-- hw/ppc/spapr_hcall.c | 9 +++++++-- hw/rdma/rdma_backend.c | 3 ++- hw/usb/ccid-card-emulated.c | 13 ++++++++---- include/qemu/thread.h | 4 ++-- io/task.c | 3 ++- iothread.c | 16 ++++++++++----- migration/migration.c | 49 +++++++++++++++++++++++++++++++-------------- migration/postcopy-ram.c | 12 +++++++++-- migration/ram.c | 40 +++++++++++++++++++++++++++--------- migration/savevm.c | 11 +++++++--- tests/atomic_add-bench.c | 3 ++- tests/iothread.c | 2 +- tests/qht-bench.c | 3 ++- tests/rcutorture.c | 3 ++- tests/test-aio.c | 2 +- tests/test-rcu-list.c | 3 ++- ui/vnc-jobs.c | 8 ++++++-- util/compatfd.c | 7 +++++-- util/oslib-posix.c | 12 ++++++++--- util/qemu-thread-posix.c | 18 +++++++++++------ util/qemu-thread-win32.c | 15 +++++++++----- util/rcu.c | 3 ++- util/thread-pool.c | 4 +++- 26 files changed, 210 insertions(+), 90 deletions(-) diff --git a/cpus.c b/cpus.c index 1feb308123..40db5c378f 100644 --- a/cpus.c +++ b/cpus.c @@ -1928,15 +1928,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, + qemu_tcg_cpu_thread_fn, cpu, + QEMU_THREAD_JOINABLE, errp)) { + return; + } } else { /* share a single thread for all cpus with TCG */ snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG"); - qemu_thread_create(cpu->thread, thread_name, - qemu_tcg_rr_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, + qemu_tcg_rr_cpu_thread_fn, cpu, + QEMU_THREAD_JOINABLE, errp)) { + return; + } single_tcg_halt_cond = cpu->halt_cond; single_tcg_cpu_thread = cpu->thread; @@ -1964,8 +1969,10 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp) snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } #ifdef _WIN32 cpu->hThread = qemu_thread_get_handle(cpu->thread); #endif @@ -1980,8 +1987,10 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } } static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp) @@ -1998,8 +2007,10 @@ static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp) snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } } static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) @@ -2011,8 +2022,10 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } #ifdef _WIN32 cpu->hThread = qemu_thread_get_handle(cpu->thread); #endif @@ -2027,8 +2040,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } } bool qemu_init_vcpu(CPUState *cpu, Error **errp) diff --git a/dump.c b/dump.c index 500b554523..4175b95d12 100644 --- a/dump.c +++ b/dump.c @@ -2021,8 +2021,10 @@ void qmp_dump_guest_memory(bool paging, const char *file, if (detach_p) { /* detached dump */ s->detached = true; - qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, - s, QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, + s, QEMU_THREAD_DETACHED, errp)) { + return; + } } else { /* sync dump */ dump_process(s, errp); diff --git a/hw/misc/edu.c b/hw/misc/edu.c index df26a4d046..2810192b1f 100644 --- a/hw/misc/edu.c +++ b/hw/misc/edu.c @@ -354,8 +354,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp) qemu_mutex_init(&edu->thr_mutex); qemu_cond_init(&edu->thr_cond); - qemu_thread_create(&edu->thread, "edu", edu_fact_thread, - edu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread, + edu, QEMU_THREAD_JOINABLE, errp)) { + return; + } memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu, "edu-mmio", 1 * MiB); diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index ae913d070f..86eb22ea55 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, sPAPRPendingHPT *pending = spapr->pending_hpt; uint64_t current_ram_size; int rc; + Error *local_err = NULL; if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) { return H_AUTHORITY; @@ -538,8 +539,12 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, pending->shift = shift; pending->ret = H_HARDWARE; - qemu_thread_create(&pending->thread, "sPAPR HPT prepare", - hpt_prepare_thread, pending, QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare", + hpt_prepare_thread, pending, + QEMU_THREAD_DETACHED, &local_err)) { + error_reportf_err(local_err, "Failed to create hpt_prepare_thread."); + return H_RESOURCE; + } spapr->pending_hpt = pending; diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c index d7a4bbd91f..e7cbb0c368 100644 --- a/hw/rdma/rdma_backend.c +++ b/hw/rdma/rdma_backend.c @@ -165,7 +165,8 @@ static void start_comp_thread(RdmaBackendDev *backend_dev) ibv_get_device_name(backend_dev->ib_dev)); backend_dev->comp_thread.run = true; qemu_thread_create(&backend_dev->comp_thread.thread, thread_name, - comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED); + comp_handler_thread, backend_dev, + QEMU_THREAD_DETACHED, &error_abort); } void rdma_backend_register_comp_handler(void (*handler)(int status, diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c index 5c8b3c9907..0d630c27db 100644 --- a/hw/usb/ccid-card-emulated.c +++ b/hw/usb/ccid-card-emulated.c @@ -538,10 +538,15 @@ static void emulated_realize(CCIDCardState *base, Error **errp) error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); return; } - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, - card, QEMU_THREAD_JOINABLE); - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, - card, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, + card, QEMU_THREAD_JOINABLE, errp)) { + return; + } + if (!qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", + handle_apdu_thread, card, + QEMU_THREAD_JOINABLE, errp)) { + return; + } } static void emulated_unrealize(CCIDCardState *base, Error **errp) diff --git a/include/qemu/thread.h b/include/qemu/thread.h index dacebcfff0..1fb84a07d2 100644 --- a/include/qemu/thread.h +++ b/include/qemu/thread.h @@ -135,9 +135,9 @@ void qemu_event_reset(QemuEvent *ev); void qemu_event_wait(QemuEvent *ev); void qemu_event_destroy(QemuEvent *ev); -void qemu_thread_create(QemuThread *thread, const char *name, +bool qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void *), - void *arg, int mode); + void *arg, int mode, Error **errp); void *qemu_thread_join(QemuThread *thread); void qemu_thread_get_self(QemuThread *thread); bool qemu_thread_is_self(QemuThread *thread); diff --git a/io/task.c b/io/task.c index 2886a2c1bc..6d3a18ab80 100644 --- a/io/task.c +++ b/io/task.c @@ -149,7 +149,8 @@ void qio_task_run_in_thread(QIOTask *task, "io-task-worker", qio_task_thread_worker, data, - QEMU_THREAD_DETACHED); + QEMU_THREAD_DETACHED, + &error_abort); } diff --git a/iothread.c b/iothread.c index aff1281257..5b2a1df36d 100644 --- a/iothread.c +++ b/iothread.c @@ -161,9 +161,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &local_error); if (local_error) { error_propagate(errp, local_error); - aio_context_unref(iothread->ctx); - iothread->ctx = NULL; - return; + goto fail; } qemu_mutex_init(&iothread->init_done_lock); @@ -175,8 +173,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) */ name = object_get_canonical_path_component(OBJECT(obj)); thread_name = g_strdup_printf("IO %s", name); - qemu_thread_create(&iothread->thread, thread_name, iothread_run, - iothread, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, + iothread, QEMU_THREAD_JOINABLE, errp)) { + g_free(thread_name); + g_free(name); + goto fail; + } g_free(thread_name); g_free(name); @@ -187,6 +189,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &iothread->init_done_lock); } qemu_mutex_unlock(&iothread->init_done_lock); + return; +fail: + aio_context_unref(iothread->ctx); + iothread->ctx = NULL; } typedef struct { diff --git a/migration/migration.c b/migration/migration.c index 4b316ec343..52e476a8f3 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -388,6 +388,7 @@ static void process_incoming_migration_co(void *opaque) MigrationIncomingState *mis = migration_incoming_get_current(); PostcopyState ps; int ret; + Error *local_err = NULL; assert(mis->from_src_file); mis->migration_incoming_co = qemu_coroutine_self(); @@ -420,8 +421,13 @@ static void process_incoming_migration_co(void *opaque) /* we get COLO info, and know if we are in COLO mode */ if (!ret && migration_incoming_enable_colo()) { - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", - colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", + colo_process_incoming_thread, mis, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "Failed to create " + "colo_process_incoming_thread."); + goto fail; + } mis->have_colo_incoming_thread = true; qemu_coroutine_yield(); @@ -430,20 +436,22 @@ static void process_incoming_migration_co(void *opaque) } if (ret < 0) { - Error *local_err = NULL; - - migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, - MIGRATION_STATUS_FAILED); error_report("load of migration failed: %s", strerror(-ret)); - qemu_fclose(mis->from_src_file); - if (multifd_load_cleanup(&local_err) != 0) { - error_report_err(local_err); - } - exit(EXIT_FAILURE); + goto fail; } mis->bh = qemu_bh_new(process_incoming_migration_bh, mis); qemu_bh_schedule(mis->bh); mis->migration_incoming_co = NULL; + return; +fail: + local_err = NULL; + migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, + MIGRATION_STATUS_FAILED); + qemu_fclose(mis->from_src_file); + if (multifd_load_cleanup(&local_err) != 0) { + error_report_err(local_err); + } + exit(EXIT_FAILURE); } static void migration_incoming_setup(QEMUFile *f) @@ -2288,6 +2296,7 @@ out: static int open_return_path_on_source(MigrationState *ms, bool create_thread) { + Error *local_err = NULL; ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file); if (!ms->rp_state.from_dst_file) { @@ -2301,8 +2310,13 @@ static int open_return_path_on_source(MigrationState *ms, return 0; } - qemu_thread_create(&ms->rp_state.rp_thread, "return path", - source_return_path_thread, ms, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&ms->rp_state.rp_thread, "return path", + source_return_path_thread, ms, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "Failed to create source_return_path_thread."); + return -1; + } trace_open_return_path_on_source_continue(); @@ -3127,8 +3141,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) migrate_fd_cleanup(s); return; } - qemu_thread_create(&s->thread, "live_migration", migration_thread, s, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&s->thread, "live_migration", migration_thread, + s, QEMU_THREAD_JOINABLE, &error_in)) { + error_reportf_err(error_in, "Failed to create migration_thread."); + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); + migrate_fd_cleanup(s); + return; + } s->migration_thread_running = true; } diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 853d8b32ca..ebc6213c5e 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1082,6 +1082,8 @@ retry: int postcopy_ram_enable_notify(MigrationIncomingState *mis) { + Error *local_err = NULL; + /* Open the fd for the kernel to give us userfaults */ mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK); if (mis->userfault_fd == -1) { @@ -1108,8 +1110,14 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis) } qemu_sem_init(&mis->fault_thread_sem, 0); - qemu_thread_create(&mis->fault_thread, "postcopy/fault", - postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&mis->fault_thread, "postcopy/fault", + postcopy_ram_fault_thread, mis, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "Failed to create postcopy_ram_fault_thread."); + qemu_sem_destroy(&mis->fault_thread_sem); + return -1; + } qemu_sem_wait(&mis->fault_thread_sem); qemu_sem_destroy(&mis->fault_thread_sem); mis->have_fault_thread = true; diff --git a/migration/ram.c b/migration/ram.c index 79c89425a3..7bf7901d9c 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -470,6 +470,7 @@ static void compress_threads_save_cleanup(void) static int compress_threads_save_setup(void) { int i, thread_count; + Error *local_err = NULL; if (!migrate_use_compression()) { return 0; @@ -499,9 +500,12 @@ static int compress_threads_save_setup(void) comp_param[i].quit = false; qemu_mutex_init(&comp_param[i].mutex); qemu_cond_init(&comp_param[i].cond); - qemu_thread_create(compress_threads + i, "compress", - do_data_compress, comp_param + i, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(compress_threads + i, "compress", + do_data_compress, comp_param + i, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "Failed to create do_data_compress."); + goto exit; + } } return 0; @@ -1075,8 +1079,15 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque) p->c = QIO_CHANNEL(sioc); qio_channel_set_delay(p->c, false); p->running = true; - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "Failed to create multifd_send_thread."); + if (multifd_save_cleanup(&local_err) != 0) { + migrate_set_error(migrate_get_current(), local_err); + } + return; + } atomic_inc(&multifd_send_state->count); } @@ -1345,8 +1356,12 @@ bool multifd_recv_new_channel(QIOChannel *ioc) p->num_packets = 1; p->running = true; - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&p->thread, p->name, multifd_recv_thread, + p, QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "Failed to create multifd_recv_thread."); + multifd_recv_terminate_threads(local_err); + return false; + } atomic_inc(&multifd_recv_state->count); return multifd_recv_state->count == migrate_multifd_channels(); } @@ -3542,6 +3557,7 @@ static void compress_threads_load_cleanup(void) static int compress_threads_load_setup(QEMUFile *f) { int i, thread_count; + Error *local_err = NULL; if (!migrate_use_compression()) { return 0; @@ -3563,9 +3579,13 @@ static int compress_threads_load_setup(QEMUFile *f) qemu_cond_init(&decomp_param[i].cond); decomp_param[i].done = true; decomp_param[i].quit = false; - qemu_thread_create(decompress_threads + i, "decompress", - do_data_decompress, decomp_param + i, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(decompress_threads + i, "decompress", + do_data_decompress, decomp_param + i, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "Failed to create do_data_decompress."); + goto exit; + } } return 0; exit: diff --git a/migration/savevm.c b/migration/savevm.c index 13e51f0e34..f35a1c5f01 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1727,9 +1727,14 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) mis->have_listen_thread = true; /* Start up the listening thread and wait for it to signal ready */ qemu_sem_init(&mis->listen_thread_sem, 0); - qemu_thread_create(&mis->listen_thread, "postcopy/listen", - postcopy_ram_listen_thread, NULL, - QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&mis->listen_thread, "postcopy/listen", + postcopy_ram_listen_thread, NULL, + QEMU_THREAD_DETACHED, &local_err)) { + error_reportf_err(local_err, + "Failed to create postcopy_ram_listen_thread."); + qemu_sem_destroy(&mis->listen_thread_sem); + return -1; + } qemu_sem_wait(&mis->listen_thread_sem); qemu_sem_destroy(&mis->listen_thread_sem); diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c index 2f6c72f63a..338b9563e3 100644 --- a/tests/atomic_add-bench.c +++ b/tests/atomic_add-bench.c @@ -2,6 +2,7 @@ #include "qemu/thread.h" #include "qemu/host-utils.h" #include "qemu/processor.h" +#include "qapi/error.h" struct thread_info { uint64_t r; @@ -110,7 +111,7 @@ static void create_threads(void) info->r = (i + 1) ^ time(NULL); qemu_thread_create(&threads[i], NULL, thread_func, info, - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); } } diff --git a/tests/iothread.c b/tests/iothread.c index 777d9eea46..f4ad992e61 100644 --- a/tests/iothread.c +++ b/tests/iothread.c @@ -73,7 +73,7 @@ IOThread *iothread_new(void) qemu_mutex_init(&iothread->init_done_lock); qemu_cond_init(&iothread->init_done_cond); qemu_thread_create(&iothread->thread, NULL, iothread_run, - iothread, QEMU_THREAD_JOINABLE); + iothread, QEMU_THREAD_JOINABLE, &error_abort); /* Wait for initialization to complete */ qemu_mutex_lock(&iothread->init_done_lock); diff --git a/tests/qht-bench.c b/tests/qht-bench.c index f492b3a20a..20a4101a17 100644 --- a/tests/qht-bench.c +++ b/tests/qht-bench.c @@ -9,6 +9,7 @@ #include "qemu/atomic.h" #include "qemu/qht.h" #include "qemu/rcu.h" +#include "qapi/error.h" #include "exec/tb-hash-xx.h" struct thread_stats { @@ -239,7 +240,7 @@ th_create_n(QemuThread **threads, struct thread_info **infos, const char *name, prepare_thread_info(&info[i], offset + i); info[i].func = func; qemu_thread_create(&th[i], name, thread_func, &info[i], - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); } } diff --git a/tests/rcutorture.c b/tests/rcutorture.c index 49311c82ea..0e799ff256 100644 --- a/tests/rcutorture.c +++ b/tests/rcutorture.c @@ -64,6 +64,7 @@ #include "qemu/atomic.h" #include "qemu/rcu.h" #include "qemu/thread.h" +#include "qapi/error.h" long long n_reads = 0LL; long n_updates = 0L; @@ -90,7 +91,7 @@ static void create_thread(void *(*func)(void *)) exit(-1); } qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads], - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); n_threads++; } diff --git a/tests/test-aio.c b/tests/test-aio.c index 86fb73b3d5..b3ac261724 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -154,7 +154,7 @@ static void test_acquire(void) qemu_thread_create(&thread, "test_acquire_thread", test_acquire_thread, - &data, QEMU_THREAD_JOINABLE); + &data, QEMU_THREAD_JOINABLE, &error_abort); /* Block in aio_poll(), let other thread kick us and acquire context */ aio_context_acquire(ctx); diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c index 192bfbf02e..9ea35a3dad 100644 --- a/tests/test-rcu-list.c +++ b/tests/test-rcu-list.c @@ -25,6 +25,7 @@ #include "qemu/rcu.h" #include "qemu/thread.h" #include "qemu/rcu_queue.h" +#include "qapi/error.h" /* * Test variables. @@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *)) exit(-1); } qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads], - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); n_threads++; } diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c index 8807d7217c..35a652d1fd 100644 --- a/ui/vnc-jobs.c +++ b/ui/vnc-jobs.c @@ -31,6 +31,7 @@ #include "vnc-jobs.h" #include "qemu/sockets.h" #include "qemu/main-loop.h" +#include "qapi/error.h" #include "block/aio.h" /* @@ -340,8 +341,11 @@ bool vnc_start_worker_thread(Error **errp) } q = vnc_queue_init(); - qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, - QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, + q, QEMU_THREAD_DETACHED, errp)) { + vnc_queue_clear(q); + return false; + } queue = q; /* Set global queue */ out: return true; diff --git a/util/compatfd.c b/util/compatfd.c index d3ed890405..2532fa4d4d 100644 --- a/util/compatfd.c +++ b/util/compatfd.c @@ -91,8 +91,11 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) memcpy(&info->mask, mask, sizeof(*mask)); info->fd = fds[1]; - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info, - QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, + info, QEMU_THREAD_DETACHED, errp)) { + free(info); + return -1; + } return fds[0]; } diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 13b6f8d776..f34ba7ac9a 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -364,6 +364,7 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, size_t size_per_thread; char *addr = area; int i = 0; + Error *local_err = NULL; memset_thread_failed = false; memset_num_threads = get_memset_num_threads(smp_cpus); @@ -375,15 +376,20 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, memset_thread[i].numpages = (i == (memset_num_threads - 1)) ? numpages : numpages_per_thread; memset_thread[i].hpagesize = hpagesize; - qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", - do_touch_pages, &memset_thread[i], - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", + do_touch_pages, &memset_thread[i], + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "Failed to create do_touch_pages."); + memset_thread_failed = true; + goto out; + } addr += size_per_thread; numpages -= numpages_per_thread; } for (i = 0; i < memset_num_threads; i++) { qemu_thread_join(&memset_thread[i].pgthread); } +out: g_free(memset_thread); memset_thread = NULL; diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index dfa66ff2fb..8e493bd653 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -15,6 +15,7 @@ #include "qemu/atomic.h" #include "qemu/notify.h" #include "qemu-thread-common.h" +#include "qapi/error.h" static bool name_threads; @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args) return start_routine(arg); } -void qemu_thread_create(QemuThread *thread, const char *name, - void *(*start_routine)(void*), - void *arg, int mode) +bool qemu_thread_create(QemuThread *thread, const char *name, + void *(*start_routine)(void *), + void *arg, int mode, Error **errp) { sigset_t set, oldset; int err; @@ -515,7 +516,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, err = pthread_attr_init(&attr); if (err) { - error_exit(err, __func__); + goto fail; } if (mode == QEMU_THREAD_DETACHED) { @@ -534,12 +535,17 @@ void qemu_thread_create(QemuThread *thread, const char *name, err = pthread_create(&thread->thread, &attr, qemu_thread_start, qemu_thread_args); - if (err) - error_exit(err, __func__); + if (err) { + goto fail; + } pthread_sigmask(SIG_SETMASK, &oldset, NULL); pthread_attr_destroy(&attr); + return true; +fail: + error_setg(errp, "qemu_thread_create failed: %s", strerror(err)); + return false; } void qemu_thread_get_self(QemuThread *thread) diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c index 4a363ca675..178a69ed40 100644 --- a/util/qemu-thread-win32.c +++ b/util/qemu-thread-win32.c @@ -20,6 +20,7 @@ #include "qemu/thread.h" #include "qemu/notify.h" #include "qemu-thread-common.h" +#include "qapi/error.h" #include <process.h> static bool name_threads; @@ -366,7 +367,7 @@ void *qemu_thread_join(QemuThread *thread) HANDLE handle; data = thread->data; - if (data->mode == QEMU_THREAD_DETACHED) { + if (data == NULL || data->mode == QEMU_THREAD_DETACHED) { return NULL; } @@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread) return ret; } -void qemu_thread_create(QemuThread *thread, const char *name, - void *(*start_routine)(void *), - void *arg, int mode) +bool qemu_thread_create(QemuThread *thread, const char *name, + void *(*start_routine)(void *), + void *arg, int mode, Error **errp) { HANDLE hThread; struct QemuThreadData *data; @@ -409,10 +410,14 @@ void qemu_thread_create(QemuThread *thread, const char *name, hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) { - error_exit(GetLastError(), __func__); + g_free(data); + error_setg_win32(errp, GetLastError(), + "failed to creat win32_start_routine"); + return false; } CloseHandle(hThread); thread->data = data; + return true; } void qemu_thread_get_self(QemuThread *thread) diff --git a/util/rcu.c b/util/rcu.c index 5676c22bd1..145dcdb0c6 100644 --- a/util/rcu.c +++ b/util/rcu.c @@ -32,6 +32,7 @@ #include "qemu/atomic.h" #include "qemu/thread.h" #include "qemu/main-loop.h" +#include "qapi/error.h" #if defined(CONFIG_MALLOC_TRIM) #include <malloc.h> #endif @@ -325,7 +326,7 @@ static void rcu_init_complete(void) * must have been quiescent even after forking, just recreate it. */ qemu_thread_create(&thread, "call_rcu", call_rcu_thread, - NULL, QEMU_THREAD_DETACHED); + NULL, QEMU_THREAD_DETACHED, &error_abort); rcu_register_thread(); } diff --git a/util/thread-pool.c b/util/thread-pool.c index 610646d131..ad0f980783 100644 --- a/util/thread-pool.c +++ b/util/thread-pool.c @@ -22,6 +22,7 @@ #include "trace.h" #include "block/thread-pool.h" #include "qemu/main-loop.h" +#include "qapi/error.h" static void do_spawn_thread(ThreadPool *pool); @@ -132,7 +133,8 @@ static void do_spawn_thread(ThreadPool *pool) pool->new_threads--; pool->pending_threads++; - qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED); + qemu_thread_create(&t, "worker", worker_thread, pool, + QEMU_THREAD_DETACHED, &error_abort); } static void spawn_thread_bh_fn(void *opaque) -- 2.13.7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle Fei Li @ 2018-09-12 8:20 ` Fam Zheng 2018-09-13 10:14 ` Fei Li 0 siblings, 1 reply; 12+ messages in thread From: Fam Zheng @ 2018-09-12 8:20 UTC (permalink / raw) To: Fei Li; +Cc: qemu-devel, berrange, armbru, eblake On Fri, 09/07 21:39, Fei Li wrote: > Make qemu_thread_create() return a Boolean to indicate if it succeeds > rather than failing with an error. And add an Error parameter to hold > the error message and let the callers handle it. > > Besides, directly return if thread->data is NULL to avoid the > segmentation fault in qemu_thread_join in qemu-thread-win32.c. Please pay special attention to cleaning up code of the touched functions since you are adding new error paths to them. I haven't reviewed the full patch after pointing out a couple two places. Please audit the resource cleaning up more carefully in v3. > > Signed-off-by: Fei Li <fli@suse.com> > --- > cpus.c | 45 +++++++++++++++++++++++++++-------------- > dump.c | 6 ++++-- > hw/misc/edu.c | 6 ++++-- > hw/ppc/spapr_hcall.c | 9 +++++++-- > hw/rdma/rdma_backend.c | 3 ++- > hw/usb/ccid-card-emulated.c | 13 ++++++++---- > include/qemu/thread.h | 4 ++-- > io/task.c | 3 ++- > iothread.c | 16 ++++++++++----- > migration/migration.c | 49 +++++++++++++++++++++++++++++++-------------- > migration/postcopy-ram.c | 12 +++++++++-- > migration/ram.c | 40 +++++++++++++++++++++++++++--------- > migration/savevm.c | 11 +++++++--- > tests/atomic_add-bench.c | 3 ++- > tests/iothread.c | 2 +- > tests/qht-bench.c | 3 ++- > tests/rcutorture.c | 3 ++- > tests/test-aio.c | 2 +- > tests/test-rcu-list.c | 3 ++- > ui/vnc-jobs.c | 8 ++++++-- > util/compatfd.c | 7 +++++-- > util/oslib-posix.c | 12 ++++++++--- > util/qemu-thread-posix.c | 18 +++++++++++------ > util/qemu-thread-win32.c | 15 +++++++++----- > util/rcu.c | 3 ++- > util/thread-pool.c | 4 +++- > 26 files changed, 210 insertions(+), 90 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 1feb308123..40db5c378f 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1928,15 +1928,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG", > cpu->cpu_index); > > - qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, > + qemu_tcg_cpu_thread_fn, cpu, > + QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > > } else { > /* share a single thread for all cpus with TCG */ > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG"); > - qemu_thread_create(cpu->thread, thread_name, > - qemu_tcg_rr_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, > + qemu_tcg_rr_cpu_thread_fn, cpu, > + QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > > single_tcg_halt_cond = cpu->halt_cond; > single_tcg_cpu_thread = cpu->thread; > @@ -1964,8 +1969,10 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp) > > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX", > cpu->cpu_index); > - qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > #ifdef _WIN32 > cpu->hThread = qemu_thread_get_handle(cpu->thread); > #endif > @@ -1980,8 +1987,10 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp) > qemu_cond_init(cpu->halt_cond); > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", > cpu->cpu_index); > - qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > } > > static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp) > @@ -1998,8 +2007,10 @@ static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp) > > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF", > cpu->cpu_index); > - qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > } > > static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) > @@ -2011,8 +2022,10 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) > qemu_cond_init(cpu->halt_cond); > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX", > cpu->cpu_index); > - qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > #ifdef _WIN32 > cpu->hThread = qemu_thread_get_handle(cpu->thread); > #endif > @@ -2027,8 +2040,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp) > qemu_cond_init(cpu->halt_cond); > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY", > cpu->cpu_index); > - qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, > + cpu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > } > > bool qemu_init_vcpu(CPUState *cpu, Error **errp) > diff --git a/dump.c b/dump.c > index 500b554523..4175b95d12 100644 > --- a/dump.c > +++ b/dump.c > @@ -2021,8 +2021,10 @@ void qmp_dump_guest_memory(bool paging, const char *file, > if (detach_p) { > /* detached dump */ > s->detached = true; > - qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, > - s, QEMU_THREAD_DETACHED); > + if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, > + s, QEMU_THREAD_DETACHED, errp)) { > + return; > + } > } else { > /* sync dump */ > dump_process(s, errp); > diff --git a/hw/misc/edu.c b/hw/misc/edu.c > index df26a4d046..2810192b1f 100644 > --- a/hw/misc/edu.c > +++ b/hw/misc/edu.c > @@ -354,8 +354,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp) > > qemu_mutex_init(&edu->thr_mutex); > qemu_cond_init(&edu->thr_cond); > - qemu_thread_create(&edu->thread, "edu", edu_fact_thread, > - edu, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread, > + edu, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > > memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu, > "edu-mmio", 1 * MiB); > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index ae913d070f..86eb22ea55 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, > sPAPRPendingHPT *pending = spapr->pending_hpt; > uint64_t current_ram_size; > int rc; > + Error *local_err = NULL; > > if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) { > return H_AUTHORITY; > @@ -538,8 +539,12 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, > pending->shift = shift; > pending->ret = H_HARDWARE; > > - qemu_thread_create(&pending->thread, "sPAPR HPT prepare", > - hpt_prepare_thread, pending, QEMU_THREAD_DETACHED); > + if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare", > + hpt_prepare_thread, pending, > + QEMU_THREAD_DETACHED, &local_err)) { > + error_reportf_err(local_err, "Failed to create hpt_prepare_thread."); > + return H_RESOURCE; > + } > > spapr->pending_hpt = pending; > > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c > index d7a4bbd91f..e7cbb0c368 100644 > --- a/hw/rdma/rdma_backend.c > +++ b/hw/rdma/rdma_backend.c > @@ -165,7 +165,8 @@ static void start_comp_thread(RdmaBackendDev *backend_dev) > ibv_get_device_name(backend_dev->ib_dev)); > backend_dev->comp_thread.run = true; > qemu_thread_create(&backend_dev->comp_thread.thread, thread_name, > - comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED); > + comp_handler_thread, backend_dev, > + QEMU_THREAD_DETACHED, &error_abort); > } > > void rdma_backend_register_comp_handler(void (*handler)(int status, > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c > index 5c8b3c9907..0d630c27db 100644 > --- a/hw/usb/ccid-card-emulated.c > +++ b/hw/usb/ccid-card-emulated.c > @@ -538,10 +538,15 @@ static void emulated_realize(CCIDCardState *base, Error **errp) > error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); > return; > } > - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, > - card, QEMU_THREAD_JOINABLE); > - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, > - card, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, > + card, QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > + if (!qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", > + handle_apdu_thread, card, > + QEMU_THREAD_JOINABLE, errp)) { > + return; > + } > } > > static void emulated_unrealize(CCIDCardState *base, Error **errp) > diff --git a/include/qemu/thread.h b/include/qemu/thread.h > index dacebcfff0..1fb84a07d2 100644 > --- a/include/qemu/thread.h > +++ b/include/qemu/thread.h > @@ -135,9 +135,9 @@ void qemu_event_reset(QemuEvent *ev); > void qemu_event_wait(QemuEvent *ev); > void qemu_event_destroy(QemuEvent *ev); > > -void qemu_thread_create(QemuThread *thread, const char *name, > +bool qemu_thread_create(QemuThread *thread, const char *name, > void *(*start_routine)(void *), > - void *arg, int mode); > + void *arg, int mode, Error **errp); > void *qemu_thread_join(QemuThread *thread); > void qemu_thread_get_self(QemuThread *thread); > bool qemu_thread_is_self(QemuThread *thread); > diff --git a/io/task.c b/io/task.c > index 2886a2c1bc..6d3a18ab80 100644 > --- a/io/task.c > +++ b/io/task.c > @@ -149,7 +149,8 @@ void qio_task_run_in_thread(QIOTask *task, > "io-task-worker", > qio_task_thread_worker, > data, > - QEMU_THREAD_DETACHED); > + QEMU_THREAD_DETACHED, > + &error_abort); > } > > > diff --git a/iothread.c b/iothread.c > index aff1281257..5b2a1df36d 100644 > --- a/iothread.c > +++ b/iothread.c > @@ -161,9 +161,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > &local_error); > if (local_error) { > error_propagate(errp, local_error); > - aio_context_unref(iothread->ctx); > - iothread->ctx = NULL; > - return; > + goto fail; > } > > qemu_mutex_init(&iothread->init_done_lock); > @@ -175,8 +173,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > */ > name = object_get_canonical_path_component(OBJECT(obj)); > thread_name = g_strdup_printf("IO %s", name); > - qemu_thread_create(&iothread->thread, thread_name, iothread_run, > - iothread, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, > + iothread, QEMU_THREAD_JOINABLE, errp)) { > + g_free(thread_name); > + g_free(name); > + goto fail; > + } > g_free(thread_name); > g_free(name); > > @@ -187,6 +189,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > &iothread->init_done_lock); > } > qemu_mutex_unlock(&iothread->init_done_lock); > + return; > +fail: > + aio_context_unref(iothread->ctx); > + iothread->ctx = NULL; > } > > typedef struct { > diff --git a/migration/migration.c b/migration/migration.c > index 4b316ec343..52e476a8f3 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -388,6 +388,7 @@ static void process_incoming_migration_co(void *opaque) > MigrationIncomingState *mis = migration_incoming_get_current(); > PostcopyState ps; > int ret; > + Error *local_err = NULL; > > assert(mis->from_src_file); > mis->migration_incoming_co = qemu_coroutine_self(); > @@ -420,8 +421,13 @@ static void process_incoming_migration_co(void *opaque) > > /* we get COLO info, and know if we are in COLO mode */ > if (!ret && migration_incoming_enable_colo()) { > - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", > - colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", > + colo_process_incoming_thread, mis, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, "Failed to create " > + "colo_process_incoming_thread."); > + goto fail; > + } > mis->have_colo_incoming_thread = true; > qemu_coroutine_yield(); > > @@ -430,20 +436,22 @@ static void process_incoming_migration_co(void *opaque) > } > > if (ret < 0) { > - Error *local_err = NULL; > - > - migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, > - MIGRATION_STATUS_FAILED); > error_report("load of migration failed: %s", strerror(-ret)); > - qemu_fclose(mis->from_src_file); > - if (multifd_load_cleanup(&local_err) != 0) { > - error_report_err(local_err); > - } > - exit(EXIT_FAILURE); > + goto fail; > } > mis->bh = qemu_bh_new(process_incoming_migration_bh, mis); > qemu_bh_schedule(mis->bh); > mis->migration_incoming_co = NULL; > + return; > +fail: > + local_err = NULL; > + migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, > + MIGRATION_STATUS_FAILED); > + qemu_fclose(mis->from_src_file); > + if (multifd_load_cleanup(&local_err) != 0) { > + error_report_err(local_err); > + } > + exit(EXIT_FAILURE); > } > > static void migration_incoming_setup(QEMUFile *f) > @@ -2288,6 +2296,7 @@ out: > static int open_return_path_on_source(MigrationState *ms, > bool create_thread) > { > + Error *local_err = NULL; > > ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file); > if (!ms->rp_state.from_dst_file) { > @@ -2301,8 +2310,13 @@ static int open_return_path_on_source(MigrationState *ms, > return 0; > } > > - qemu_thread_create(&ms->rp_state.rp_thread, "return path", > - source_return_path_thread, ms, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&ms->rp_state.rp_thread, "return path", > + source_return_path_thread, ms, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, > + "Failed to create source_return_path_thread."); > + return -1; > + } > > trace_open_return_path_on_source_continue(); > > @@ -3127,8 +3141,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) > migrate_fd_cleanup(s); > return; > } > - qemu_thread_create(&s->thread, "live_migration", migration_thread, s, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&s->thread, "live_migration", migration_thread, > + s, QEMU_THREAD_JOINABLE, &error_in)) { > + error_reportf_err(error_in, "Failed to create migration_thread."); > + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); > + migrate_fd_cleanup(s); > + return; > + } > s->migration_thread_running = true; > } > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index 853d8b32ca..ebc6213c5e 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -1082,6 +1082,8 @@ retry: > > int postcopy_ram_enable_notify(MigrationIncomingState *mis) > { > + Error *local_err = NULL; > + > /* Open the fd for the kernel to give us userfaults */ > mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK); > if (mis->userfault_fd == -1) { > @@ -1108,8 +1110,14 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis) > } > > qemu_sem_init(&mis->fault_thread_sem, 0); > - qemu_thread_create(&mis->fault_thread, "postcopy/fault", > - postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&mis->fault_thread, "postcopy/fault", > + postcopy_ram_fault_thread, mis, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, > + "Failed to create postcopy_ram_fault_thread."); > + qemu_sem_destroy(&mis->fault_thread_sem); > + return -1; > + } > qemu_sem_wait(&mis->fault_thread_sem); > qemu_sem_destroy(&mis->fault_thread_sem); > mis->have_fault_thread = true; > diff --git a/migration/ram.c b/migration/ram.c > index 79c89425a3..7bf7901d9c 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -470,6 +470,7 @@ static void compress_threads_save_cleanup(void) > static int compress_threads_save_setup(void) > { > int i, thread_count; > + Error *local_err = NULL; > > if (!migrate_use_compression()) { > return 0; > @@ -499,9 +500,12 @@ static int compress_threads_save_setup(void) > comp_param[i].quit = false; > qemu_mutex_init(&comp_param[i].mutex); > qemu_cond_init(&comp_param[i].cond); > - qemu_thread_create(compress_threads + i, "compress", > - do_data_compress, comp_param + i, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(compress_threads + i, "compress", > + do_data_compress, comp_param + i, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, "Failed to create do_data_compress."); > + goto exit; > + } > } > return 0; > > @@ -1075,8 +1079,15 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque) > p->c = QIO_CHANNEL(sioc); > qio_channel_set_delay(p->c, false); > p->running = true; > - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, > + "Failed to create multifd_send_thread."); > + if (multifd_save_cleanup(&local_err) != 0) { > + migrate_set_error(migrate_get_current(), local_err); > + } > + return; > + } > > atomic_inc(&multifd_send_state->count); > } > @@ -1345,8 +1356,12 @@ bool multifd_recv_new_channel(QIOChannel *ioc) > p->num_packets = 1; > > p->running = true; > - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&p->thread, p->name, multifd_recv_thread, > + p, QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, "Failed to create multifd_recv_thread."); > + multifd_recv_terminate_threads(local_err); > + return false; > + } > atomic_inc(&multifd_recv_state->count); > return multifd_recv_state->count == migrate_multifd_channels(); > } > @@ -3542,6 +3557,7 @@ static void compress_threads_load_cleanup(void) > static int compress_threads_load_setup(QEMUFile *f) > { > int i, thread_count; > + Error *local_err = NULL; > > if (!migrate_use_compression()) { > return 0; > @@ -3563,9 +3579,13 @@ static int compress_threads_load_setup(QEMUFile *f) > qemu_cond_init(&decomp_param[i].cond); > decomp_param[i].done = true; > decomp_param[i].quit = false; > - qemu_thread_create(decompress_threads + i, "decompress", > - do_data_decompress, decomp_param + i, > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(decompress_threads + i, "decompress", > + do_data_decompress, decomp_param + i, > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, > + "Failed to create do_data_decompress."); > + goto exit; > + } > } > return 0; > exit: > diff --git a/migration/savevm.c b/migration/savevm.c > index 13e51f0e34..f35a1c5f01 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1727,9 +1727,14 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) > mis->have_listen_thread = true; > /* Start up the listening thread and wait for it to signal ready */ > qemu_sem_init(&mis->listen_thread_sem, 0); > - qemu_thread_create(&mis->listen_thread, "postcopy/listen", > - postcopy_ram_listen_thread, NULL, > - QEMU_THREAD_DETACHED); > + if (!qemu_thread_create(&mis->listen_thread, "postcopy/listen", > + postcopy_ram_listen_thread, NULL, > + QEMU_THREAD_DETACHED, &local_err)) { > + error_reportf_err(local_err, > + "Failed to create postcopy_ram_listen_thread."); > + qemu_sem_destroy(&mis->listen_thread_sem); > + return -1; > + } > qemu_sem_wait(&mis->listen_thread_sem); > qemu_sem_destroy(&mis->listen_thread_sem); > > diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c > index 2f6c72f63a..338b9563e3 100644 > --- a/tests/atomic_add-bench.c > +++ b/tests/atomic_add-bench.c > @@ -2,6 +2,7 @@ > #include "qemu/thread.h" > #include "qemu/host-utils.h" > #include "qemu/processor.h" > +#include "qapi/error.h" > > struct thread_info { > uint64_t r; > @@ -110,7 +111,7 @@ static void create_threads(void) > > info->r = (i + 1) ^ time(NULL); > qemu_thread_create(&threads[i], NULL, thread_func, info, > - QEMU_THREAD_JOINABLE); > + QEMU_THREAD_JOINABLE, &error_abort); > } > } > > diff --git a/tests/iothread.c b/tests/iothread.c > index 777d9eea46..f4ad992e61 100644 > --- a/tests/iothread.c > +++ b/tests/iothread.c > @@ -73,7 +73,7 @@ IOThread *iothread_new(void) > qemu_mutex_init(&iothread->init_done_lock); > qemu_cond_init(&iothread->init_done_cond); > qemu_thread_create(&iothread->thread, NULL, iothread_run, > - iothread, QEMU_THREAD_JOINABLE); > + iothread, QEMU_THREAD_JOINABLE, &error_abort); > > /* Wait for initialization to complete */ > qemu_mutex_lock(&iothread->init_done_lock); > diff --git a/tests/qht-bench.c b/tests/qht-bench.c > index f492b3a20a..20a4101a17 100644 > --- a/tests/qht-bench.c > +++ b/tests/qht-bench.c > @@ -9,6 +9,7 @@ > #include "qemu/atomic.h" > #include "qemu/qht.h" > #include "qemu/rcu.h" > +#include "qapi/error.h" > #include "exec/tb-hash-xx.h" > > struct thread_stats { > @@ -239,7 +240,7 @@ th_create_n(QemuThread **threads, struct thread_info **infos, const char *name, > prepare_thread_info(&info[i], offset + i); > info[i].func = func; > qemu_thread_create(&th[i], name, thread_func, &info[i], > - QEMU_THREAD_JOINABLE); > + QEMU_THREAD_JOINABLE, &error_abort); > } > } > > diff --git a/tests/rcutorture.c b/tests/rcutorture.c > index 49311c82ea..0e799ff256 100644 > --- a/tests/rcutorture.c > +++ b/tests/rcutorture.c > @@ -64,6 +64,7 @@ > #include "qemu/atomic.h" > #include "qemu/rcu.h" > #include "qemu/thread.h" > +#include "qapi/error.h" > > long long n_reads = 0LL; > long n_updates = 0L; > @@ -90,7 +91,7 @@ static void create_thread(void *(*func)(void *)) > exit(-1); > } > qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads], > - QEMU_THREAD_JOINABLE); > + QEMU_THREAD_JOINABLE, &error_abort); > n_threads++; > } > > diff --git a/tests/test-aio.c b/tests/test-aio.c > index 86fb73b3d5..b3ac261724 100644 > --- a/tests/test-aio.c > +++ b/tests/test-aio.c > @@ -154,7 +154,7 @@ static void test_acquire(void) > > qemu_thread_create(&thread, "test_acquire_thread", > test_acquire_thread, > - &data, QEMU_THREAD_JOINABLE); > + &data, QEMU_THREAD_JOINABLE, &error_abort); > > /* Block in aio_poll(), let other thread kick us and acquire context */ > aio_context_acquire(ctx); > diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c > index 192bfbf02e..9ea35a3dad 100644 > --- a/tests/test-rcu-list.c > +++ b/tests/test-rcu-list.c > @@ -25,6 +25,7 @@ > #include "qemu/rcu.h" > #include "qemu/thread.h" > #include "qemu/rcu_queue.h" > +#include "qapi/error.h" > > /* > * Test variables. > @@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *)) > exit(-1); > } > qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads], > - QEMU_THREAD_JOINABLE); > + QEMU_THREAD_JOINABLE, &error_abort); > n_threads++; > } > > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c > index 8807d7217c..35a652d1fd 100644 > --- a/ui/vnc-jobs.c > +++ b/ui/vnc-jobs.c > @@ -31,6 +31,7 @@ > #include "vnc-jobs.h" > #include "qemu/sockets.h" > #include "qemu/main-loop.h" > +#include "qapi/error.h" > #include "block/aio.h" > > /* > @@ -340,8 +341,11 @@ bool vnc_start_worker_thread(Error **errp) > } > > q = vnc_queue_init(); > - qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, > - QEMU_THREAD_DETACHED); > + if (!qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, > + q, QEMU_THREAD_DETACHED, errp)) { > + vnc_queue_clear(q); > + return false; > + } > queue = q; /* Set global queue */ > out: > return true; > diff --git a/util/compatfd.c b/util/compatfd.c > index d3ed890405..2532fa4d4d 100644 > --- a/util/compatfd.c > +++ b/util/compatfd.c > @@ -91,8 +91,11 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) > memcpy(&info->mask, mask, sizeof(*mask)); > info->fd = fds[1]; > > - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info, > - QEMU_THREAD_DETACHED); > + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, > + info, QEMU_THREAD_DETACHED, errp)) { > + free(info); Clean up fds? > + return -1; > + } > > return fds[0]; > } > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 13b6f8d776..f34ba7ac9a 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -364,6 +364,7 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, > size_t size_per_thread; > char *addr = area; > int i = 0; > + Error *local_err = NULL; > > memset_thread_failed = false; > memset_num_threads = get_memset_num_threads(smp_cpus); > @@ -375,15 +376,20 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, > memset_thread[i].numpages = (i == (memset_num_threads - 1)) ? > numpages : numpages_per_thread; > memset_thread[i].hpagesize = hpagesize; > - qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", > - do_touch_pages, &memset_thread[i], > - QEMU_THREAD_JOINABLE); > + if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", > + do_touch_pages, &memset_thread[i], > + QEMU_THREAD_JOINABLE, &local_err)) { > + error_reportf_err(local_err, "Failed to create do_touch_pages."); > + memset_thread_failed = true; > + goto out; Before freeing memset_thread, you need to join all spawned threads. Which means you have to remember the current value of i (as 'started_thread') here and ... > + } > addr += size_per_thread; > numpages -= numpages_per_thread; > } the 'out' label should be here, and the loop condition should be changed to 'i < started_thread'. > for (i = 0; i < memset_num_threads; i++) { > qemu_thread_join(&memset_thread[i].pgthread); > } > +out: Otherwise the threads can cause use-after-free. > g_free(memset_thread); > memset_thread = NULL; > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > index dfa66ff2fb..8e493bd653 100644 > --- a/util/qemu-thread-posix.c > +++ b/util/qemu-thread-posix.c > @@ -15,6 +15,7 @@ > #include "qemu/atomic.h" > #include "qemu/notify.h" > #include "qemu-thread-common.h" > +#include "qapi/error.h" > > static bool name_threads; > > @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args) > return start_routine(arg); > } > > -void qemu_thread_create(QemuThread *thread, const char *name, > - void *(*start_routine)(void*), > - void *arg, int mode) > +bool qemu_thread_create(QemuThread *thread, const char *name, > + void *(*start_routine)(void *), > + void *arg, int mode, Error **errp) > { > sigset_t set, oldset; > int err; > @@ -515,7 +516,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, > > err = pthread_attr_init(&attr); > if (err) { > - error_exit(err, __func__); > + goto fail; > } > > if (mode == QEMU_THREAD_DETACHED) { > @@ -534,12 +535,17 @@ void qemu_thread_create(QemuThread *thread, const char *name, > err = pthread_create(&thread->thread, &attr, > qemu_thread_start, qemu_thread_args); > > - if (err) > - error_exit(err, __func__); > + if (err) { > + goto fail; > + } > > pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > pthread_attr_destroy(&attr); > + return true; > +fail: > + error_setg(errp, "qemu_thread_create failed: %s", strerror(err)); It's better to do error_setg before 'goto fail', so that different error conditions have different error descriptions. > + return false; > } > > void qemu_thread_get_self(QemuThread *thread) > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c > index 4a363ca675..178a69ed40 100644 > --- a/util/qemu-thread-win32.c > +++ b/util/qemu-thread-win32.c > @@ -20,6 +20,7 @@ > #include "qemu/thread.h" > #include "qemu/notify.h" > #include "qemu-thread-common.h" > +#include "qapi/error.h" > #include <process.h> > > static bool name_threads; > @@ -366,7 +367,7 @@ void *qemu_thread_join(QemuThread *thread) > HANDLE handle; > > data = thread->data; > - if (data->mode == QEMU_THREAD_DETACHED) { > + if (data == NULL || data->mode == QEMU_THREAD_DETACHED) { > return NULL; > } > I think this could be a separate patch. > @@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread) > return ret; > } > > -void qemu_thread_create(QemuThread *thread, const char *name, > - void *(*start_routine)(void *), > - void *arg, int mode) > +bool qemu_thread_create(QemuThread *thread, const char *name, > + void *(*start_routine)(void *), > + void *arg, int mode, Error **errp) > { > HANDLE hThread; > struct QemuThreadData *data; > @@ -409,10 +410,14 @@ void qemu_thread_create(QemuThread *thread, const char *name, > hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, > data, 0, &thread->tid); > if (!hThread) { > - error_exit(GetLastError(), __func__); > + g_free(data); > + error_setg_win32(errp, GetLastError(), > + "failed to creat win32_start_routine"); "create". > + return false; > } > CloseHandle(hThread); > thread->data = data; > + return true; > } > > void qemu_thread_get_self(QemuThread *thread) > diff --git a/util/rcu.c b/util/rcu.c > index 5676c22bd1..145dcdb0c6 100644 > --- a/util/rcu.c > +++ b/util/rcu.c > @@ -32,6 +32,7 @@ > #include "qemu/atomic.h" > #include "qemu/thread.h" > #include "qemu/main-loop.h" > +#include "qapi/error.h" > #if defined(CONFIG_MALLOC_TRIM) > #include <malloc.h> > #endif > @@ -325,7 +326,7 @@ static void rcu_init_complete(void) > * must have been quiescent even after forking, just recreate it. > */ > qemu_thread_create(&thread, "call_rcu", call_rcu_thread, > - NULL, QEMU_THREAD_DETACHED); > + NULL, QEMU_THREAD_DETACHED, &error_abort); > > rcu_register_thread(); > } > diff --git a/util/thread-pool.c b/util/thread-pool.c > index 610646d131..ad0f980783 100644 > --- a/util/thread-pool.c > +++ b/util/thread-pool.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "block/thread-pool.h" > #include "qemu/main-loop.h" > +#include "qapi/error.h" > > static void do_spawn_thread(ThreadPool *pool); > > @@ -132,7 +133,8 @@ static void do_spawn_thread(ThreadPool *pool) > pool->new_threads--; > pool->pending_threads++; > > - qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED); > + qemu_thread_create(&t, "worker", worker_thread, pool, > + QEMU_THREAD_DETACHED, &error_abort); > } > > static void spawn_thread_bh_fn(void *opaque) > -- > 2.13.7 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle 2018-09-12 8:20 ` Fam Zheng @ 2018-09-13 10:14 ` Fei Li 0 siblings, 0 replies; 12+ messages in thread From: Fei Li @ 2018-09-13 10:14 UTC (permalink / raw) To: Fam Zheng; +Cc: qemu-devel, berrange, armbru, eblake On 09/12/2018 04:20 PM, Fam Zheng wrote: > On Fri, 09/07 21:39, Fei Li wrote: >> Make qemu_thread_create() return a Boolean to indicate if it succeeds >> rather than failing with an error. And add an Error parameter to hold >> the error message and let the callers handle it. >> >> Besides, directly return if thread->data is NULL to avoid the >> segmentation fault in qemu_thread_join in qemu-thread-win32.c. > Please pay special attention to cleaning up code of the touched functions since > you are adding new error paths to them. I haven't reviewed the full patch after > pointing out a couple two places. Please audit the resource cleaning up more > carefully in v3. Ok, thanks for all the comments. Will pay special attention to the cleaning ups in next version. > >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> cpus.c | 45 +++++++++++++++++++++++++++-------------- >> dump.c | 6 ++++-- >> hw/misc/edu.c | 6 ++++-- >> hw/ppc/spapr_hcall.c | 9 +++++++-- >> hw/rdma/rdma_backend.c | 3 ++- >> hw/usb/ccid-card-emulated.c | 13 ++++++++---- >> include/qemu/thread.h | 4 ++-- >> io/task.c | 3 ++- >> iothread.c | 16 ++++++++++----- >> migration/migration.c | 49 +++++++++++++++++++++++++++++++-------------- >> migration/postcopy-ram.c | 12 +++++++++-- >> migration/ram.c | 40 +++++++++++++++++++++++++++--------- >> migration/savevm.c | 11 +++++++--- >> tests/atomic_add-bench.c | 3 ++- >> tests/iothread.c | 2 +- >> tests/qht-bench.c | 3 ++- >> tests/rcutorture.c | 3 ++- >> tests/test-aio.c | 2 +- >> tests/test-rcu-list.c | 3 ++- >> ui/vnc-jobs.c | 8 ++++++-- >> util/compatfd.c | 7 +++++-- >> util/oslib-posix.c | 12 ++++++++--- >> util/qemu-thread-posix.c | 18 +++++++++++------ >> util/qemu-thread-win32.c | 15 +++++++++----- >> util/rcu.c | 3 ++- >> util/thread-pool.c | 4 +++- >> 26 files changed, 210 insertions(+), 90 deletions(-) >> ...snip... >> return true; >> diff --git a/util/compatfd.c b/util/compatfd.c >> index d3ed890405..2532fa4d4d 100644 >> --- a/util/compatfd.c >> +++ b/util/compatfd.c >> @@ -91,8 +91,11 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) >> memcpy(&info->mask, mask, sizeof(*mask)); >> info->fd = fds[1]; >> >> - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info, >> - QEMU_THREAD_DETACHED); >> + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, >> + info, QEMU_THREAD_DETACHED, errp)) { >> + free(info); > Clean up fds? Thanks for pointing this out. > >> + return -1; >> + } >> >> return fds[0]; >> } >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 13b6f8d776..f34ba7ac9a 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -364,6 +364,7 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, >> size_t size_per_thread; >> char *addr = area; >> int i = 0; >> + Error *local_err = NULL; >> >> memset_thread_failed = false; >> memset_num_threads = get_memset_num_threads(smp_cpus); >> @@ -375,15 +376,20 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, >> memset_thread[i].numpages = (i == (memset_num_threads - 1)) ? >> numpages : numpages_per_thread; >> memset_thread[i].hpagesize = hpagesize; >> - qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", >> - do_touch_pages, &memset_thread[i], >> - QEMU_THREAD_JOINABLE); >> + if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", >> + do_touch_pages, &memset_thread[i], >> + QEMU_THREAD_JOINABLE, &local_err)) { >> + error_reportf_err(local_err, "Failed to create do_touch_pages."); >> + memset_thread_failed = true; >> + goto out; > Before freeing memset_thread, you need to join all spawned threads. Which means > you have to remember the current value of i (as 'started_thread') here and ... > >> + } >> addr += size_per_thread; >> numpages -= numpages_per_thread; >> } > the 'out' label should be here, and the loop condition should be changed to 'i < > started_thread'. > >> for (i = 0; i < memset_num_threads; i++) { >> qemu_thread_join(&memset_thread[i].pgthread); >> } >> +out: > Otherwise the threads can cause use-after-free. Right! Somehow overlooked the loop.. > >> g_free(memset_thread); >> memset_thread = NULL; >> >> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c >> index dfa66ff2fb..8e493bd653 100644 >> --- a/util/qemu-thread-posix.c >> +++ b/util/qemu-thread-posix.c >> @@ -15,6 +15,7 @@ >> #include "qemu/atomic.h" >> #include "qemu/notify.h" >> #include "qemu-thread-common.h" >> +#include "qapi/error.h" >> >> static bool name_threads; >> >> @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args) >> return start_routine(arg); >> } >> >> -void qemu_thread_create(QemuThread *thread, const char *name, >> - void *(*start_routine)(void*), >> - void *arg, int mode) >> +bool qemu_thread_create(QemuThread *thread, const char *name, >> + void *(*start_routine)(void *), >> + void *arg, int mode, Error **errp) >> { >> sigset_t set, oldset; >> int err; >> @@ -515,7 +516,7 @@ void qemu_thread_create(QemuThread *thread, const char *name, >> >> err = pthread_attr_init(&attr); >> if (err) { >> - error_exit(err, __func__); >> + goto fail; >> } >> >> if (mode == QEMU_THREAD_DETACHED) { >> @@ -534,12 +535,17 @@ void qemu_thread_create(QemuThread *thread, const char *name, >> err = pthread_create(&thread->thread, &attr, >> qemu_thread_start, qemu_thread_args); >> >> - if (err) >> - error_exit(err, __func__); >> + if (err) { >> + goto fail; >> + } >> >> pthread_sigmask(SIG_SETMASK, &oldset, NULL); >> >> pthread_attr_destroy(&attr); >> + return true; >> +fail: >> + error_setg(errp, "qemu_thread_create failed: %s", strerror(err)); > It's better to do error_setg before 'goto fail', so that different error > conditions have different error descriptions. In this function, I think the returned value: err can be used to differentiate the failing reason. Maybe just make the error_setg stay after 'goto fail' in this function? I will take care in other cases. > >> + return false; >> } >> >> void qemu_thread_get_self(QemuThread *thread) >> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c >> index 4a363ca675..178a69ed40 100644 >> --- a/util/qemu-thread-win32.c >> +++ b/util/qemu-thread-win32.c >> @@ -20,6 +20,7 @@ >> #include "qemu/thread.h" >> #include "qemu/notify.h" >> #include "qemu-thread-common.h" >> +#include "qapi/error.h" >> #include <process.h> >> >> static bool name_threads; >> @@ -366,7 +367,7 @@ void *qemu_thread_join(QemuThread *thread) >> HANDLE handle; >> >> data = thread->data; >> - if (data->mode == QEMU_THREAD_DETACHED) { >> + if (data == NULL || data->mode == QEMU_THREAD_DETACHED) { >> return NULL; >> } >> > I think this could be a separate patch. Ok, thanks for the advice. > >> @@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread) >> return ret; >> } >> >> -void qemu_thread_create(QemuThread *thread, const char *name, >> - void *(*start_routine)(void *), >> - void *arg, int mode) >> +bool qemu_thread_create(QemuThread *thread, const char *name, >> + void *(*start_routine)(void *), >> + void *arg, int mode, Error **errp) >> { >> HANDLE hThread; >> struct QemuThreadData *data; >> @@ -409,10 +410,14 @@ void qemu_thread_create(QemuThread *thread, const char *name, >> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, >> data, 0, &thread->tid); >> if (!hThread) { >> - error_exit(GetLastError(), __func__); >> + g_free(data); >> + error_setg_win32(errp, GetLastError(), >> + "failed to creat win32_start_routine"); > "create". Thanks! Have a nice day :) Fei > >> + return false; >> } >> CloseHandle(hThread); >> thread->data = data; >> + return true; >> } >> >> void qemu_thread_get_self(QemuThread *thread) >> diff --git a/util/rcu.c b/util/rcu.c >> index 5676c22bd1..145dcdb0c6 100644 >> --- a/util/rcu.c >> +++ b/util/rcu.c >> @@ -32,6 +32,7 @@ >> #include "qemu/atomic.h" >> #include "qemu/thread.h" >> #include "qemu/main-loop.h" >> +#include "qapi/error.h" >> #if defined(CONFIG_MALLOC_TRIM) >> #include <malloc.h> >> #endif >> @@ -325,7 +326,7 @@ static void rcu_init_complete(void) >> * must have been quiescent even after forking, just recreate it. >> */ >> qemu_thread_create(&thread, "call_rcu", call_rcu_thread, >> - NULL, QEMU_THREAD_DETACHED); >> + NULL, QEMU_THREAD_DETACHED, &error_abort); >> >> rcu_register_thread(); >> } >> diff --git a/util/thread-pool.c b/util/thread-pool.c >> index 610646d131..ad0f980783 100644 >> --- a/util/thread-pool.c >> +++ b/util/thread-pool.c >> @@ -22,6 +22,7 @@ >> #include "trace.h" >> #include "block/thread-pool.h" >> #include "qemu/main-loop.h" >> +#include "qapi/error.h" >> >> static void do_spawn_thread(ThreadPool *pool); >> >> @@ -132,7 +133,8 @@ static void do_spawn_thread(ThreadPool *pool) >> pool->new_threads--; >> pool->pending_threads++; >> >> - qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED); >> + qemu_thread_create(&t, "worker", worker_thread, pool, >> + QEMU_THREAD_DETACHED, &error_abort); >> } >> >> static void spawn_thread_bh_fn(void *opaque) >> -- >> 2.13.7 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-09-13 10:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-07 13:38 [Qemu-devel] [PATCH v2 0/4] qemu_thread_create: propagate errors to callers to check Fei Li 2018-09-07 13:38 ` [Qemu-devel] [PATCH v2 1/4] Fix segmentation fault when qemu_signal_init fails Fei Li 2018-09-12 7:55 ` Fam Zheng 2018-09-13 8:46 ` Fei Li 2018-09-13 8:58 ` Fam Zheng 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 2/4] ui/vnc.c: polish vnc_init_func Fei Li 2018-09-12 7:57 ` Fam Zheng 2018-09-13 9:03 ` Fei Li 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 3/4] qemu_init_vcpu: add a new Error parameter to propagate Fei Li 2018-09-07 13:39 ` [Qemu-devel] [PATCH v2 4/4] qemu_thread_create: propagate the error to callers to handle Fei Li 2018-09-12 8:20 ` Fam Zheng 2018-09-13 10:14 ` Fei Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).