qemu_thread_create: propagate errors to callers to check
cover qemu_thread_create: propagate errors to callers to check
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 final caller check it. The fourth patch is dedicated for qemu_thread_create(). The fifth patch updates the direct callers of qemu_thread_create.
Please help to review. Thanks.
Fei Li (5):
Fix segmentation fault when qemu_signal_init fails ui/vnc.c: polish vnc_init_func
qemu_init_vcpu: add a new Error paramater to propagate
qemu_thread_create: propagate the error to callers to check Propagate qemu_thread_create's error to all callers to handle
cpus.c | 80 ++++++++++++++++++++++++++++++----------- dump.c | 6 +++- hw/misc/edu.c | 8 ++++- 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 | 2 +- include/qom/cpu.h | 2 +- include/ui/console.h | 2 +- io/task.c | 3 +- iothread.c | 15 +++++--- migration/migration.c | 47 +++++++++++++++++------- migration/postcopy-ram.c | 11 +++++- migration/ram.c | 32 ++++++++++++++--- migration/savevm.c | 8 ++++- target/alpha/cpu.c | 6 +++- target/arm/cpu.c | 6 +++- target/cris/cpu.c | 6 +++- target/hppa/cpu.c | 6 +++- target/i386/cpu.c | 6 +++- target/lm32/cpu.c | 6 +++- target/m68k/cpu.c | 6 +++- target/microblaze/cpu.c | 6 +++- target/mips/cpu.c | 6 +++- target/moxie/cpu.c | 6 +++- target/nios2/cpu.c | 6 +++- target/openrisc/cpu.c | 6 +++- target/ppc/translate_init.inc.c | 6 +++- target/riscv/cpu.c | 6 +++- target/s390x/cpu.c | 5 ++- target/sh4/cpu.c | 6 +++- target/sparc/cpu.c | 6 +++- target/tilegx/cpu.c | 6 +++- target/tricore/cpu.c | 6 +++- target/unicore32/cpu.c | 6 +++- target/xtensa/cpu.c | 6 +++- 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 | 13 +++++-- ui/vnc-jobs.h | 2 +- ui/vnc.c | 15 ++++++-- util/compatfd.c | 25 +++++++++---- util/main-loop.c | 11 +++--- util/oslib-posix.c | 10 +++++- util/qemu-thread-posix.c | 15 +++++--- util/qemu-thread-win32.c | 12 +++++-- util/rcu.c | 3 +- util/thread-pool.c | 4 ++- 53 files changed, 386 insertions(+), 108 deletions(-)
Signature · 1 line
2.13.7
PATCH 1/5 Fix segmentation fault when qemu_signal_init fails
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.
--- include/qemu/osdep.h | 2 +- util/compatfd.c | 17 ++++++++++++----- util/main-loop.c | 11 +++++++---- 3 files changed, 20 insertions(+), 10 deletions(-)
include/qemu/osdep.h +1 -1
@@ -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
util/compatfd.c +12 -5
@@ -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 malloc in %s" , __func__); errno =ENOMEM ;return -1 ; }if (pipe (fds) ==-1 ) { +error_setg (errp,"Failed to create a pipe in %s" , __func__);free (info);return -1 ; }
@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {qemu_set_cloexec (ret);return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + } +return ret; }
util/main-loop.c +7 -4
@@ -71,10 +71,11 @@ static void sigfd_handler(void *opaque) } } -static int qemu_signal_init (void ) +static int qemu_signal_init (Error **errp) {int sigfd;sigset_t set; +Error *local_err =NULL ;/* * SIG_IPI must be blocked in the main thread and must not be caught
@@ -94,9 +95,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, &local_err);if (sigfd ==-1 ) {fprintf (stderr,"failed to create signalfd\n" ); +error_propagate (errp, local_err);return -errno; }
@@ -109,7 +111,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,8 +150,9 @@ int qemu_init_main_loop(Error **errp)init_clocks (qemu_timer_notify_cb); - ret =qemu_signal_init (); + ret =qemu_signal_init (&local_error);if (ret) { +error_propagate (errp, local_error);return ret; }
Signature · 1 line
2.13.7
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Quoted 49 lines · Fei Li
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 runserror_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.
Signed-off-by Fei Li <[email protected]>--- include/qemu/osdep.h | 2 +- util/compatfd.c | 17 ++++++++++++----- util/main-loop.c | 11 +++++++---- 3 files changed, 20 insertions(+), 10 deletions(-)include/qemu/osdep.h +1
-1@@ -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 util/compatfd.c +2
-1@@ -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;
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 77
Quoted 4 lines
util/compatfd.c +1
-0@@ -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 malloc in %s" , __func__);
I'd avoid using __func__ in error messages. Instead
"Failed to allocate signalfd memory"
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 83
Quoted 6 lines
util/compatfd.c +1
-0errno =ENOMEM ;return -1 ; }if (pipe (fds) ==-1 ) { +error_setg (errp,"Failed to create a pipe in %s" , __func__);
"Failed to create signalfd pipe"
Quoted 17 lines
util/compatfd.c +3
-2free (info);return -1 ; }@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 115
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }
Using a local_err is not required - you can just pass errp stright to qemu_signalfd_compat() and then check
if (ret < 0)
Quoted 20 lines
util/compatfd.c +1
-0+return ret; }util/main-loop.c +2
-1@@ -71,10 +71,11 @@ static void sigfd_handler(void *opaque) } } -static int qemu_signal_init (void ) +static int qemu_signal_init (Error **errp) {int sigfd;sigset_t set; +Error *local_err =NULL ;/* * SIG_IPI must be blocked in the main thread and must not be caught
util/main-loop.c static int qemu_signal_init(void) · line 103
Quoted 10 lines
util/main-loop.c +2
-1@@ -94,9 +95,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, &local_err);if (sigfd ==-1 ) {fprintf (stderr,"failed to create signalfd\n" ); +error_propagate (errp, local_err);return -errno; }
Again, no need for local_err - just pass errp stright in
Quoted 11 lines
util/main-loop.c +1
-1@@ -109,7 +111,7 @@ static int qemu_signal_init(void)#else /* _WIN32 */ -static int qemu_signal_init (void ) +static int qemu_signal_init (Error **errp) {return 0 ; }
util/main-loop.c int qemu_init_main_loop(Error **errp) · line 157
Quoted 9 lines
util/main-loop.c +2
-1@@ -148,8 +150,9 @@ int qemu_init_main_loop(Error **errp)init_clocks (qemu_timer_notify_cb); - ret =qemu_signal_init (); + ret =qemu_signal_init (&local_error);if (ret) { +error_propagate (errp, local_error);return ret; }
Again, no need for local_error.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Thanks for the review! :)
Quoted 51 lines · Daniel P. Berrangé
Quoted 50 lines · Fei Li
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 runserror_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.
Signed-off-by Fei Li <[email protected]>--- include/qemu/osdep.h | 2 +- util/compatfd.c | 17 ++++++++++++----- util/main-loop.c | 11 +++++++---- 3 files changed, 20 insertions(+), 10 deletions(-)include/qemu/osdep.h +1
-1@@ -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 util/compatfd.c +2
-1@@ -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)
Quoted 6 lines
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 77
Quoted 3 lines
util/compatfd.c +1
-0info =malloc (sizeof (*info));if (info ==NULL ) { +error_setg (errp,"Failed to malloc in %s" , __func__);I'd avoid using
__func__in error messages. Instead"Failed to allocate signalfd memory"
Ok, thanks.
Quoted 8 lines
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 83
Quoted 7 lines
util/compatfd.c +1
-0errno =ENOMEM ;return -1 ; }if (pipe (fds) ==-1 ) { +error_setg (errp,"Failed to create a pipe in %s" , __func__);"Failed to create signalfd pipe"
OK.
Quoted 31 lines
Quoted 18 lines
util/compatfd.c +3
-2free (info);return -1 ; }@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 115
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }Using a
local_erris not required - you can just pass errp stright toqemu_signalfd_compat()and then check
Quoted 1 line
if (ret < 0)
For the use of a local error object & error_propagate call, I'd like to explain here. :) In our code, the initial caller passes two kinds of Error to the call trace, one is something like &error_abort and &error_fatal, the other is NULL.
For the former, the exit() occurs in the functions where error_handle_fatal() is called (e.g. called by error_propagate/error_setg/...). The patch3: qemu_init_vcpu is the case, that means the system will exit in the final callee: qemu_thread_create(), instead of the initial caller pc_new_cpu(). In such case, I think propagating seems more reasonable.
For the latter, suppose we pass errp straightly. As NULL is passed, the error occurs in the final callee will automatically "propagating" to the initial caller, and let it handle this error. The patch1: qemu_signal_init is this case, let the caller of qemu_init_main_loop handle. In such case, the error_propagate is indeed needless.
How do you think passing errp straightly for the latter case, and use a local error object & error_propagate for the former case? This is a distinct treatment, but would shorten the code.
Have a nice day, thanks again
Fei
Quoted 57 lines
Quoted 20 lines
util/compatfd.c +1
-0+return ret; }util/main-loop.c +2
-1@@ -71,10 +71,11 @@ static void sigfd_handler(void *opaque) } } -static int qemu_signal_init (void ) +static int qemu_signal_init (Error **errp) {int sigfd;sigset_t set; +Error *local_err =NULL ;/* * SIG_IPI must be blocked in the main thread and must not be caught util/main-loop.c static int qemu_signal_init(void) · line 103
Quoted 10 lines
util/main-loop.c +2
-1@@ -94,9 +95,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, &local_err);if (sigfd ==-1 ) {fprintf (stderr,"failed to create signalfd\n" ); +error_propagate (errp, local_err);return -errno; }Again, no need for
local_err- just pass errp stright inQuoted 11 lines
util/main-loop.c +1
-1@@ -109,7 +111,7 @@ static int qemu_signal_init(void)#else /* _WIN32 */ -static int qemu_signal_init (void ) +static int qemu_signal_init (Error **errp) {return 0 ; }util/main-loop.c int qemu_init_main_loop(Error **errp) · line 157
Quoted 9 lines
util/main-loop.c +2
-1@@ -148,8 +150,9 @@ int qemu_init_main_loop(Error **errp)init_clocks (qemu_timer_notify_cb); - ret =qemu_signal_init (); + ret =qemu_signal_init (&local_error);if (ret) { +error_propagate (errp, local_error);return ret; }Again, no need for
local_error.Regards,
Daniel
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Quoted 101 lines · Fei Li
Thanks for the review! :)
Quoted 51 lines · Daniel P. Berrangé
Quoted 45 lines · Fei Li
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 runserror_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.
Signed-off-by Fei Li <[email protected]>--- include/qemu/osdep.h | 2 +- util/compatfd.c | 17 ++++++++++++----- util/main-loop.c | 11 +++++++---- 3 files changed, 20 insertions(+), 10 deletions(-)include/qemu/osdep.h +1
-1@@ -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 util/compatfd.c +2
-1@@ -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;util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 76
Quoted 3 lines
util/compatfd.c +1
-0@@ -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 malloc in %s" , __func__);I'd avoid using
__func__in error messages. InsteadQuoted 1 line
"Failed to allocate signalfd memory"
Ok, thanks.
Quoted 7 lines
util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 83
Quoted 6 lines
util/compatfd.c +1
-0errno =ENOMEM ;return -1 ; }if (pipe (fds) ==-1 ) { +error_setg (errp,"Failed to create a pipe in %s" , __func__);"Failed to create signalfd pipe"
OK.
Quoted 29 lines
Quoted 16 lines
util/compatfd.c +3
-2free (info);return -1 ; }@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 113
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }Using a
local_erris not required - you can just pass errp stright toqemu_signalfd_compat()and then checkQuoted 1 line
if (ret < 0)For the use of a local error object &
error_propagatecall, I'd like to explain here. :)
In our code, the initial caller passes two kinds of Error to the call trace, one is
something like&error_abortand&error_fatal, the other is NULL.
Quoted 8 lines
For the former, the
exit()occurs in the functions whereerror_handle_fatal()is called
(e.g. called by error_propagate/error_setg/...). The patch3:qemu_init_vcpuis the case,
that means the system will exit in the final callee:qemu_thread_create(), instead of
the initial callerpc_new_cpu(). In such case, I think propagating seems more reasonable.
I don't really agree. It is preferrable to abort immediately at the deepest place which raises the error. The stack trace will thus show the full call chain leading upto the problem.
Quoted 4 lines
How do you think passing errp straightly for the latter case, and use a local error object &
error_propagatefor the former case? This is a distinct treatment, but would shorten the code.
It is inappropriate to second-guess whether the caller is a passing in NULL or &error_abort, or another Error object. What is passed in can change at any time in the future.
We should only ever use a local error where the local method has a need to look at the error contents before returning to the caller. Any other case should just use the errp directly.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Quoted 7 lines · Daniel P. Berrangé
Quoted 6 lines · Fei Li
Thanks for the review! :)
Quoted 2 lines · Daniel P. Berrangé
On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
... snip ...
Quoted 43 lines
Quoted 35 lines
Quoted 28 lines
Quoted 15 lines
free(info); return -1; }util/compatfd.c +3
-2@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 113
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }Using a
local_erris not required - you can just pass errp stright toqemu_signalfd_compat()and then checkQuoted 1 line
if (ret < 0)For the use of a local error object &
error_propagatecall, I'd like to explain here. :)
In our code, the initial caller passes two kinds of Error to the call trace, one is
something like&error_abortand&error_fatal, the other is NULL.Quoted 8 lines
For the former, the
exit()occurs in the functions whereerror_handle_fatal()is called
(e.g. called by error_propagate/error_setg/...). The patch3:qemu_init_vcpuis the case,
that means the system will exit in the final callee:qemu_thread_create(), instead of
the initial callerpc_new_cpu(). In such case, I think propagating seems more reasonable.
Quoted 3 lines
I don't really agree. It is preferrable to abort immediately at the deepest place which raises the error. The stack trace will thus show the full call chain leading upto the problem.
Sorry for the above example, it is not exactly correct: for the patch3 case, the system will exit in device_set_realized(), where the first error_propagate() is called if we pass errp directly, but not in the final callee.. Sorry for the misleading.
For another example, its call trace:qemu_thread_create(, NULL)
<= iothread_complete(, NULL)
<== user_creatable_complete(, NULL)
<=== object_new_with_propv(, errp)
<==== object_new_with_props(, errp) {... error_propagate(errp, local_err); ...} <===== iothread_create(, &error_abort)
The exit occurs in object_new_with_props where the first error_propagate is called.
Either the device_set_realized() or object_new_with_props() is a middle caller, thus we can only see the top half stack trace until where error_handle_fatal() is called.
In other words, the exit() occurs neither in the final callee nor the initial caller. Sorry for the misleading example again..
Quoted 8 lines
Quoted 5 lines
How do you think passing errp straightly for the latter case, and use a local error object &
error_propagatefor the former case? This is a distinct treatment, but would shorten the code.It is inappropriate to second-guess whether the caller is a passing in NULL or
&error_abort, or another Error object. What is passed in can change at any time in the future.
ok.
Quoted 7 lines
We should only ever use a local error where the local method has a need to look at the error contents before returning to the caller. Any other case should just use the errp directly.
Regards,
Daniel
Have a nice day, thanks
Fei
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Quoted 75 lines · Fei Li
Quoted 7 lines · Daniel P. Berrangé
Quoted 6 lines · Fei Li
Thanks for the review! :)
Quoted 2 lines · Daniel P. Berrangé
On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
... snip ...
Quoted 43 lines
Quoted 35 lines
Quoted 28 lines
Quoted 15 lines
free(info); return -1; }util/compatfd.c +3
-2@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 113
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }Using a
local_erris not required - you can just pass errp stright toqemu_signalfd_compat()and then checkQuoted 1 line
if (ret < 0)For the use of a local error object &
error_propagatecall, I'd like to explain here. :)
In our code, the initial caller passes two kinds of Error to the call trace, one is
something like&error_abortand&error_fatal, the other is NULL.Quoted 8 lines
For the former, the
exit()occurs in the functions whereerror_handle_fatal()is called
(e.g. called by error_propagate/error_setg/...). The patch3:qemu_init_vcpuis the case,
that means the system will exit in the final callee:qemu_thread_create(), instead of
the initial callerpc_new_cpu(). In such case, I think propagating seems more reasonable.Quoted 3 lines
I don't really agree. It is preferrable to abort immediately at the deepest place which raises the error. The stack trace will thus show the full call chain leading upto the problem.
Sorry for the above example, it is not exactly correct: for the patch3 case, the
system will exit indevice_set_realized(), where the firsterror_propagate()is called
if we pass errp directly, but not in the final callee.. Sorry for the misleading.For another example, its call trace:
qemu_thread_create(, NULL)
<=iothread_complete(, NULL)
<==user_creatable_complete(, NULL)
<===object_new_with_propv(, errp)
<====object_new_with_props(, errp){...error_propagate(errp, local_err); ...}
<=====iothread_create(, &error_abort)
The exit occurs inobject_new_with_propswhere the firsterror_propagateis called.
Quoted 8 lines
Either the
device_set_realized()orobject_new_with_props()is a middle caller, thus
we can only see the top half stack trace until whereerror_handle_fatal()is called.In other words, the
exit()occurs neither in the final callee nor the initial caller.
Sorry for the misleading example again..
This means using error_propagate can potentially lose the final callee in the error_abort cases. That is why it's preferrable to just pass errp down the calling chain when possible. The reason why object_new_with_propv uses error_propagate is because both object_property_add_child and user_creatable_complete return void, thus cannot flag success/failure to its caller via their return values. To check whether they succeed, object_new_with_propv wants a non-NULL err parameter. But like you said, errp passed to object_new_with_propv may or may not be NULL, so a local_err local variable is defined to cope with that.
Alternatively it could do this instead:
{
...
if (errp) {
object_property_add_child(parent, id, obj, errp);
if (*errp) {
goto error;
}
} else {
Error *local_err = NULL;
object_property_add_child(parent, id, obj, &local_err);
if (local_err) {
goto error;
}
}
...}
This way if error_abort was passed and object_property_add_child failed, the abort point would be in the innermost function. But this is boilerplate code so it's not used.
On the contrary, using error_propagate when not necessary also means more lines of code but gives less info on the call trace when aborted.
So I fully agree with Dan.
Fam
Quoted 19 lines
Quoted 8 lines
Quoted 5 lines
How do you think passing errp straightly for the latter case, and use a local error object &
error_propagatefor the former case? This is a distinct treatment, but would shorten the code.It is inappropriate to second-guess whether the caller is a passing in NULL or
&error_abort, or another Error object. What is passed in can change at any time in the future.ok.
Quoted 7 lines
We should only ever use a local error where the local method has a need to look at the error contents before returning to the caller. Any other case should just use the errp directly.
Regards,
DanielHave a nice day, thanks
Fei
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
Quoted 119 lines · Fam Zheng
Quoted 74 lines · Fei Li
Quoted 7 lines · Daniel P. Berrangé
Quoted 6 lines · Fei Li
Thanks for the review! :)
Quoted 2 lines · Daniel P. Berrangé
On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
... snip ...
Quoted 43 lines
Quoted 35 lines
Quoted 28 lines
Quoted 15 lines
free(info); return -1; }util/compatfd.c +3
-2@@ -94,17 +97,21 @@ 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; +Error *local_err =NULL ; +#if defined(CONFIG_SIGNALFD ) ret =syscall (SYS_signalfd,-1 , mask, _NSIG /8 );if (ret !=-1 ) {util/compatfd.c static int qemu_signalfd_compat(const sigset_t *mask) · line 113
Quoted 10 lines
util/compatfd.c +4
-2qemu_set_cloexec (ret); return ret; }#endif - -return qemu_signalfd_compat (mask); + ret =qemu_signalfd_compat (mask, &local_err); +if (local_err) { +error_propagate (errp, local_err); + }Using a
local_erris not required - you can just pass errp stright toqemu_signalfd_compat()and then checkQuoted 1 line
if (ret < 0)For the use of a local error object &
error_propagatecall, I'd like to explain here. :)
In our code, the initial caller passes two kinds of Error to the call trace, one is
something like&error_abortand&error_fatal, the other is NULL.Quoted 8 lines
For the former, the
exit()occurs in the functions whereerror_handle_fatal()is called
(e.g. called by error_propagate/error_setg/...). The patch3:qemu_init_vcpuis the case,
that means the system will exit in the final callee:qemu_thread_create(), instead of
the initial callerpc_new_cpu(). In such case, I think propagating seems more reasonable.Quoted 3 lines
I don't really agree. It is preferrable to abort immediately at the deepest place which raises the error. The stack trace will thus show the full call chain leading upto the problem.
Sorry for the above example, it is not exactly correct: for the patch3 case, the
system will exit indevice_set_realized(), where the firsterror_propagate()is called
if we pass errp directly, but not in the final callee.. Sorry for the misleading.For another example, its call trace:
qemu_thread_create(, NULL)
<=iothread_complete(, NULL)
<==user_creatable_complete(, NULL)
<===object_new_with_propv(, errp)
<====object_new_with_props(, errp){...error_propagate(errp, local_err); ...}
<=====iothread_create(, &error_abort)
The exit occurs inobject_new_with_propswhere the firsterror_propagateis called.Quoted 8 lines
Either the
device_set_realized()orobject_new_with_props()is a middle caller, thus
we can only see the top half stack trace until whereerror_handle_fatal()is called.In other words, the
exit()occurs neither in the final callee nor the initial caller.
Sorry for the misleading example again..This means using
error_propagatecan potentially lose the final callee in theerror_abortcases. That is why it's preferrable to just pass errp down the calling chain when possible. The reason whyobject_new_with_propvuseserror_propagateis because bothobject_property_add_childanduser_creatable_completereturn void, thus cannot flag success/failure to its caller via their return values. To check whether they succeed,object_new_with_propvwants a non-NULL err parameter. But like you said, errp passed toobject_new_with_propvmay or may not be NULL, so alocal_errlocal variable is defined to cope with that.Alternatively it could do this instead:
{
... if (errp) { object_property_add_child(parent, id, obj, errp); if (*errp) { goto error; } } else { Error *local_err = NULL; object_property_add_child(parent, id, obj, &local_err); if (local_err) { goto error; } } ...}
This way if
error_abortwas passed andobject_property_add_childfailed, the abort point would be in the innermost function. But this is boilerplate code so it's not used.
Quoted 6 lines
On the contrary, using
error_propagatewhen not necessary also means more lines of code but gives less info on the call trace when aborted.So I fully agree with Dan.
Fam
Got, thanks for the review. :)
Have a nice day
Fei
Quoted 18 lines
Quoted 17 lines
Quoted 7 lines
Quoted 4 lines
How do you think passing errp straightly for the latter case, and use a local error object &
error_propagatefor the former case? This is a distinct treatment, but would shorten the code.It is inappropriate to second-guess whether the caller is a passing in NULL or
&error_abort, or another Error object. What is passed in can change at any time in the future.ok.
Quoted 6 lines
We should only ever use a local error where the local method has a need to look at the error contents before returning to the caller. Any other case should just use the errp directly.
Regards,
DanielHave a nice day, thanks
Fei
PATCH 2/5 ui/vnc.c: polish vnc_init_func
In reply to Fei LiAdd a new Error parameter to vnc_display_init() and handle the Error in its caller: vnc_init_func() just like vnc_display_open() does. And let the call trace propagate the Error.
--- include/ui/console.h | 2 +- ui/vnc-jobs.c | 2 +- ui/vnc-jobs.h | 2 +- ui/vnc.c | 15 ++++++++++++--- 4 files changed, 15 insertions(+), 6 deletions(-)
include/ui/console.h +1 -1
@@ -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);
ui/vnc-jobs.c +1 -1
@@ -331,7 +331,7 @@ static bool vnc_worker_thread_running(void)return queue;/* Check global queue */ } -void vnc_start_worker_thread (void ) +void vnc_start_worker_thread (Error **errp) {VncJobQueue *q;
ui/vnc-jobs.h +1 -1
@@ -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 ); +void vnc_start_worker_thread (Error **errp);/* Locks */ static inline int vnc_trylock_display (VncDisplay *vd)
ui/vnc.c +12 -3
@@ -3206,9 +3206,10 @@ 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; +Error *local_err =NULL ;if (vnc_display_find (id) !=NULL ) {return ;
@@ -3236,7 +3237,11 @@ void vnc_display_init(const char *id) vd->connections_limit =32 ;qemu_mutex_init (&vd->mutex); -vnc_start_worker_thread (); +vnc_start_worker_thread (&local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } vd->dcl.ops = &dcl_ops;register_displaychangelistener (&vd->dcl);
@@ -4079,7 +4084,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: " );
Signature · 1 line
2.13.7
PATCH 3/5 qemu_init_vcpu: add a new Error paramater to propagate
In reply to Fei LiThe 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 final caller check it.
--- cpus.c | 32 +++++++++++++++++++------------- include/qom/cpu.h | 2 +- target/alpha/cpu.c | 6 +++++- target/arm/cpu.c | 6 +++++- target/cris/cpu.c | 6 +++++- target/hppa/cpu.c | 6 +++++- target/i386/cpu.c | 6 +++++- target/lm32/cpu.c | 6 +++++- target/m68k/cpu.c | 6 +++++- target/microblaze/cpu.c | 6 +++++- target/mips/cpu.c | 6 +++++- target/moxie/cpu.c | 6 +++++- target/nios2/cpu.c | 6 +++++- target/openrisc/cpu.c | 6 +++++- target/ppc/translate_init.inc.c | 6 +++++- target/riscv/cpu.c | 6 +++++- target/s390x/cpu.c | 5 ++++- target/sh4/cpu.c | 6 +++++- target/sparc/cpu.c | 6 +++++- target/tilegx/cpu.c | 6 +++++- target/tricore/cpu.c | 6 +++++- target/unicore32/cpu.c | 6 +++++- target/xtensa/cpu.c | 6 +++++- 23 files changed, 124 insertions(+), 35 deletions(-)
cpus.c +19 -13
@@ -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) +void 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,17 +2047,22 @@ 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 ; }while (!cpu->created) {
include/qom/cpu.h +1 -1
@@ -1012,7 +1012,7 @@ void end_exclusive(void);* * Initializes a vCPU. */ -void qemu_init_vcpu (CPUState *cpu); +void 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 */
target/alpha/cpu.c +5 -1
@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } acc->parent_realize (dev, errp); }
target/arm/cpu.c +5 -1
@@ -1028,7 +1028,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) }#endif -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }cpu_reset (cs); acc->parent_realize (dev, errp);
target/cris/cpu.c +5 -1
@@ -140,7 +140,11 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) }cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } ccc->parent_realize (dev, errp); }
target/hppa/cpu.c +5 -1
@@ -98,7 +98,11 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } acc->parent_realize (dev, errp);#ifndef CONFIG_USER_ONLY
target/i386/cpu.c +5 -1
@@ -5112,7 +5112,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) }#endif -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }/* * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
target/lm32/cpu.c +5 -1
@@ -139,7 +139,11 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } lcc->parent_realize (dev, errp); }
target/m68k/cpu.c +5 -1
@@ -231,7 +231,11 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)m68k_cpu_init_gdb (cpu);cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } mcc->parent_realize (dev, errp); }
target/microblaze/cpu.c +5 -1
@@ -161,7 +161,11 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } env->pvr.regs[0 ] =PVR0_USE_EXC_MASK \ |PVR0_USE_ICACHE_MASK \
target/mips/cpu.c +5 -1
@@ -136,7 +136,11 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)cpu_mips_realize_env (&cpu->env);cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } mcc->parent_realize (dev, errp); }
target/moxie/cpu.c +5 -1
@@ -66,7 +66,11 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }cpu_reset (cs); mcc->parent_realize (dev, errp);
target/nios2/cpu.c +5 -1
@@ -94,7 +94,11 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }cpu_reset (cs); ncc->parent_realize (dev, errp);
target/openrisc/cpu.c +5 -1
@@ -83,7 +83,11 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }cpu_reset (cs); occ->parent_realize (dev, errp);
target/ppc/translate_init.inc.c +5 -1
@@ -9707,7 +9707,11 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp) 32, "power-vsx.xml", 0); } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } pcc->parent_realize (dev, errp);
target/riscv/cpu.c +5 -1
@@ -303,7 +303,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }cpu_reset (cs); mcc->parent_realize (dev, errp);
target/s390x/cpu.c +4 -1
@@ -217,7 +217,10 @@ 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); +qemu_init_vcpu (cs, &local_err); +if (local_err) { + goto out; + }/* * KVM requires the initial CPU reset ioctl to be executed on the target
target/sh4/cpu.c +5 -1
@@ -196,7 +196,11 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) }cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } scc->parent_realize (dev, errp); }
target/sparc/cpu.c +5 -1
@@ -773,7 +773,11 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } scc->parent_realize (dev, errp); }
target/tilegx/cpu.c +5 -1
@@ -92,7 +92,11 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp) }cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } tcc->parent_realize (dev, errp); }
target/tricore/cpu.c +5 -1
@@ -96,7 +96,11 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)set_feature (env,TRICORE_FEATURE_13 ); }cpu_reset (cs); -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } tcc->parent_realize (dev, errp); }
target/unicore32/cpu.c +5 -1
@@ -96,7 +96,11 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } ucc->parent_realize (dev, errp); }
target/xtensa/cpu.c +5 -1
@@ -131,7 +131,11 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp) cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs; -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } xcc->parent_realize (dev, errp); }
Signature · 1 line
2.13.7
Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
Quoted 120 lines · Fei Li
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 final caller check it.Signed-off-by Fei Li <[email protected]>--- cpus.c | 32 +++++++++++++++++++------------- include/qom/cpu.h | 2 +- target/alpha/cpu.c | 6 +++++- target/arm/cpu.c | 6 +++++- target/cris/cpu.c | 6 +++++- target/hppa/cpu.c | 6 +++++- target/i386/cpu.c | 6 +++++- target/lm32/cpu.c | 6 +++++- target/m68k/cpu.c | 6 +++++- target/microblaze/cpu.c | 6 +++++- target/mips/cpu.c | 6 +++++- target/moxie/cpu.c | 6 +++++- target/nios2/cpu.c | 6 +++++- target/openrisc/cpu.c | 6 +++++- target/ppc/translate_init.inc.c | 6 +++++- target/riscv/cpu.c | 6 +++++- target/s390x/cpu.c | 5 ++++- target/sh4/cpu.c | 6 +++++- target/sparc/cpu.c | 6 +++++- target/tilegx/cpu.c | 6 +++++- target/tricore/cpu.c | 6 +++++- target/unicore32/cpu.c | 6 +++++- target/xtensa/cpu.c | 6 +++++- 23 files changed, 124 insertions(+), 35 deletions(-)cpus.c +12
-12@@ -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) +void 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,17 +2047,22 @@ 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);
cpus.c void qemu_init_vcpu(CPUState *cpu) · line 2066
Quoted 10 lines
cpus.c +7
-1+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 ; }
I'd be inclined to make this method return a boolean, so....
Quoted 5 lines
target/alpha/cpu.c +0
-0
target/alpha/cpu.c static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) · line 73
Quoted 9 lines
target/alpha/cpu.c +5
-1@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }
...this can be simplified to get rid of the local error object
if (!qemu_init_vcpu(cs, errp)) {
return;
}likewise for the rest of the patch below...
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
Quoted 142 lines · Daniel P. Berrangé
Quoted 120 lines · Fei Li
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 final caller check it.Signed-off-by Fei Li <[email protected]>--- cpus.c | 32 +++++++++++++++++++------------- include/qom/cpu.h | 2 +- target/alpha/cpu.c | 6 +++++- target/arm/cpu.c | 6 +++++- target/cris/cpu.c | 6 +++++- target/hppa/cpu.c | 6 +++++- target/i386/cpu.c | 6 +++++- target/lm32/cpu.c | 6 +++++- target/m68k/cpu.c | 6 +++++- target/microblaze/cpu.c | 6 +++++- target/mips/cpu.c | 6 +++++- target/moxie/cpu.c | 6 +++++- target/nios2/cpu.c | 6 +++++- target/openrisc/cpu.c | 6 +++++- target/ppc/translate_init.inc.c | 6 +++++- target/riscv/cpu.c | 6 +++++- target/s390x/cpu.c | 5 ++++- target/sh4/cpu.c | 6 +++++- target/sparc/cpu.c | 6 +++++- target/tilegx/cpu.c | 6 +++++- target/tricore/cpu.c | 6 +++++- target/unicore32/cpu.c | 6 +++++- target/xtensa/cpu.c | 6 +++++- 23 files changed, 124 insertions(+), 35 deletions(-)cpus.c +12
-12@@ -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) +void 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,17 +2047,22 @@ 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);cpus.c void qemu_init_vcpu(CPUState *cpu) · line 2066
Quoted 10 lines
cpus.c +7
-1+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 ; }I'd be inclined to make this method return a boolean, so....
Quoted 8 lines
target/alpha/cpu.c +0
-0@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)return ; }
Quoted 7 lines
target/alpha/cpu.c static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) · line 73
Quoted 6 lines
target/alpha/cpu.c +5
-1-qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }...this can be simplified to get rid of the local error object
Emm, for these arch_cpu_realizefn() functions, I notice they already have the local_err and use error_propagate to handle the error. I guess the reason is what I explained in
patch1: considering their initial caller passes the &error_fatal, then just propagate and let the further caller handle such error instead of exit() here.
So maybe just keep their tradition here?
Have a nice day, thanks
Fei
Quoted 10 lines
if (!qemu_init_vcpu(cs, errp)) { return; }likewise for the rest of the patch below...
Regards,
Daniel
Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
In reply to Daniel P. BerrangéAdding Markus, as the maintainer of Error APIs, to make sure he sees this thread.
Quoted 1 line · Daniel P. Berrangé
On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
In the subject line: s/paramater/parameter/
Quoted 65 lines
Quoted 34 lines
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 final caller check it.Signed-off-by Fei Li <[email protected]>---
cpus.c +6
-6-void qemu_init_vcpu (CPUState *cpu) +void 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,17 +2047,22 @@ 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);cpus.c void qemu_init_vcpu(CPUState *cpu) · line 2066
Quoted 10 lines
cpus.c +7
-1+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 ; }I'd be inclined to make this method return a boolean, so....
Quoted 5 lines
target/alpha/cpu.c +0
-0target/alpha/cpu.c static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) · line 73
Quoted 9 lines
target/alpha/cpu.c +5
-1@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)return ; } -qemu_init_vcpu (cs); +qemu_init_vcpu (cs, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }...this can be simplified to get rid of the local error object
Quoted 3 lines
if (!qemu_init_vcpu(cs, errp)) {
return;
}Returning a bool introduces an interesting semantic question on whether 0 is success or failure (-1/0 vs. false/true means you have to pay attention to return type to decide between !func() or func()<0 when using the return value to learn if errp was set). But Markus has expressed some thoughts on the matter before:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00124.html
and favored a bool return if only for consistency with glib and to avoid abuse of trying to overload the returned int when using errp is better.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
Quoted 1 line · Eric Blake
Adding Markus, as the maintainer of Error APIs, to make sure he sees this thread.
Thanks. :)
Quoted 5 lines · Daniel P. Berrangé
Quoted 1 line · Fei Li
In the subject line: s/paramater/parameter/
Thanks for pointing this out! :)
Quoted 68 lines
Quoted 64 lines
Quoted 33 lines
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 final caller check it.Signed-off-by Fei Li <[email protected]>---
cpus.c +1
-1-void qemu_init_vcpu (CPUState *cpu) +void 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,cpus.c +4
-5@@ -2046,17 +2047,22 @@ 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);cpus.c void qemu_init_vcpu(CPUState *cpu) · line 2065
Quoted 10 lines
cpus.c +7
-1+ 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; }I'd be inclined to make this method return a boolean, so....
Quoted 5 lines
target/alpha/cpu.c +0
-0target/alpha/cpu.c static void alpha_cpu_realizefn(DeviceState *dev, · line 74
Quoted 9 lines
target/alpha/cpu.c +5
-0@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) return; } - qemu_init_vcpu(cs); + qemu_init_vcpu(cs, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + }...this can be simplified to get rid of the local error object
Quoted 3 lines
if (!qemu_init_vcpu(cs, errp)) {
return;
}
Quoted 6 lines
Returning a bool introduces an interesting semantic question on whether 0 is success or failure (-1/0 vs. false/true means you have to pay attention to return type to decide between !func() or func()<0 when using the return value to learn if errp was set). But Markus has expressed some thoughts on the matter before:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00124.html
and favored a bool return if only for consistency with glib and to avoid abuse of trying to overload the returned int when using errp is better.
Thanks for providing these instructive discussions, I read the link, and yes, I agree with "return a bool: false/true inside the callee". Just as the following:
if (!foo(arg, errp)) { // assuming foo() returns a bool: false/true handle the error...
}
As this is "Nicely concise." :)
I will prepare the next version according to all the comments.
Have a nice day, thanks all
Fei
PATCH 4/5 qemu_thread_create: propagate the error to callers to check
In reply to Fei LiAdd a new Error paramater for qemu_thread_create() to indicate if it succeeds rather than failing with an error. And propagate the error to let the callers check it.
Besides, directly return if thread->data is NULL to avoid the segmentation fault in qemu_thread_join in qemu-thread-win32.c.
--- include/qemu/thread.h | 2 +- util/qemu-thread-posix.c | 15 +++++++++++---- util/qemu-thread-win32.c | 12 +++++++++--- 3 files changed, 21 insertions(+), 8 deletions(-)
include/qemu/thread.h +1 -1
@@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev);void 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);
util/qemu-thread-posix.c +11 -4
@@ -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;
@@ -506,16 +507,17 @@ static void *qemu_thread_start(void *args)void qemu_thread_create (QemuThread *thread,const char *name,void *(*start_routine)(void *), -void *arg,int mode) +void *arg,int mode,Error **errp) {sigset_t set, oldset;int err;pthread_attr_t attr;QemuThreadArgs *qemu_thread_args; +Error *local_err =NULL ; err =pthread_attr_init (&attr);if (err) { -error_exit (err, __func__); + goto fail; }if (mode ==QEMU_THREAD_DETACHED ) {
@@ -534,12 +536,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 ; +fail: +error_setg (&local_err,"%s" ,strerror (err)); +error_propagate (errp, local_err); }void qemu_thread_get_self (QemuThread *thread)
util/qemu-thread-win32.c +9 -3
@@ -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 ; }
@@ -390,10 +391,11 @@ void *qemu_thread_join(QemuThread *thread)void qemu_thread_create (QemuThread *thread,const char *name,void *(*start_routine)(void *), -void *arg,int mode) +void *arg,int mode,Error **errp) {HANDLE hThread;struct QemuThreadData *data; +Error *local_err =NULL ; data =g_malloc (sizeof *data); data->start_routine = start_routine;
@@ -409,7 +411,11 @@ 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__); +error_setg_win32 (&local_err,GetLastError (), +"failed to creat win32_start_routine" ); +g_free (data); +error_propagate (errp, local_err); +return ; }CloseHandle (hThread); thread->data = data;
Signature · 1 line
2.13.7
Re: [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check
Quoted 19 lines · Fei Li
Add a new Error paramater for
qemu_thread_create()to indicate if it succeeds rather than failing with an error. And propagate the error to let the callers check it.Besides, directly return if
thread->datais NULL to avoid the segmentation fault inqemu_thread_joininqemu-thread-win32.c.Signed-off-by Fei Li <[email protected]>--- include/qemu/thread.h | 2 +- util/qemu-thread-posix.c | 15 +++++++++++---- util/qemu-thread-win32.c | 12 +++++++++--- 3 files changed, 21 insertions(+), 8 deletions(-)include/qemu/thread.h +0
-0
include/qemu/thread.h void qemu_event_destroy(QemuEvent *ev); · line 140
Quoted 5 lines
include/qemu/thread.h +1
-1@@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev);void qemu_thread_create (QemuThread *thread,const char *name,void *(*start_routine)(void *), -void *arg,int mode); +void *arg,int mode,Error **errp);
You've changed the API signature in this patch, but don't update any of the callers until the next patch. This means that the build will fail on this patch.
In order to ensure that "git bisect" can be usable we require that the code is able to build sucessfully on every patch in a series.
So I think you'll have to merge patch 5 into this patch to ensure the build succeeds.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check
Quoted 29 lines · Daniel P. Berrangé
Quoted 19 lines · Fei Li
Add a new Error paramater for
qemu_thread_create()to indicate if it succeeds rather than failing with an error. And propagate the error to let the callers check it.Besides, directly return if
thread->datais NULL to avoid the segmentation fault inqemu_thread_joininqemu-thread-win32.c.Signed-off-by Fei Li <[email protected]>--- include/qemu/thread.h | 2 +- util/qemu-thread-posix.c | 15 +++++++++++---- util/qemu-thread-win32.c | 12 +++++++++--- 3 files changed, 21 insertions(+), 8 deletions(-)include/qemu/thread.h +0
-0include/qemu/thread.h void qemu_event_destroy(QemuEvent *ev); · line 140
Quoted 5 lines
include/qemu/thread.h +1
-1@@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev);void qemu_thread_create (QemuThread *thread,const char *name,void *(*start_routine)(void *), -void *arg,int mode); +void *arg,int mode,Error **errp);You've changed the API signature in this patch, but don't update any of the callers until the next patch. This means that the build will fail on this patch.
Quoted 8 lines
In order to ensure that "git bisect" can be usable we require that the code is able to build sucessfully on every patch in a series.
So I think you'll have to merge patch 5 into this patch to ensure the build succeeds.
Regards,
Daniel
Oops, thanks for the reminder! Will merge them into one in the next version.
Have a nice day
Fei
PATCH 5/5 Propagate qemu_thread_create's error to all callers to handle
In reply to Fei LiLet's propagate qemu_thread_create's error to make all callers check it. For those critical callers, just pass the &error_abort.
--- cpus.c | 48 ++++++++++++++++++++++++++++++++++++++------- dump.c | 6 +++++- hw/misc/edu.c | 8 +++++++- hw/ppc/spapr_hcall.c | 9 ++++++++- hw/rdma/rdma_backend.c | 3 ++- hw/usb/ccid-card-emulated.c | 13 ++++++++++-- io/task.c | 3 ++- iothread.c | 15 +++++++++----- migration/migration.c | 47 ++++++++++++++++++++++++++++++++------------ migration/postcopy-ram.c | 11 ++++++++++- migration/ram.c | 32 ++++++++++++++++++++++++++---- migration/savevm.c | 8 +++++++- 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 | 11 +++++++++-- util/compatfd.c | 8 +++++++- util/oslib-posix.c | 10 +++++++++- util/rcu.c | 3 ++- util/thread-pool.c | 4 +++- 23 files changed, 206 insertions(+), 49 deletions(-)
cpus.c +41 -7
@@ -1904,6 +1904,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)static QemuCond *single_tcg_halt_cond;static QemuThread *single_tcg_cpu_thread;static int tcg_region_inited; +Error *local_err =NULL ;assert (tcg_enabled ());/*
@@ -1929,14 +1930,22 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) cpu->cpu_index);qemu_thread_create (cpu->thread, thread_name, qemu_tcg_cpu_thread_fn, - cpu,QEMU_THREAD_JOINABLE ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +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 ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } single_tcg_halt_cond = cpu->halt_cond; single_tcg_cpu_thread = cpu->thread;
@@ -1957,6 +1966,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)static void qemu_hax_start_vcpu (CPUState *cpu,Error **errp) {char thread_name[VCPU_THREAD_NAME_SIZE ]; +Error *local_err =NULL ; cpu->thread =g_malloc0 (sizeof (QemuThread)); cpu->halt_cond =g_malloc0 (sizeof (QemuCond));
@@ -1965,7 +1975,11 @@ 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 ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }#ifdef _WIN32 cpu->hThread =qemu_thread_get_handle (cpu->thread);#endif
@@ -1974,6 +1988,7 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)static void qemu_kvm_start_vcpu (CPUState *cpu,Error **errp) {char thread_name[VCPU_THREAD_NAME_SIZE ]; +Error *local_err =NULL ; cpu->thread =g_malloc0 (sizeof (QemuThread)); cpu->halt_cond =g_malloc0 (sizeof (QemuCond));
@@ -1981,12 +1996,17 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)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 ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } }static void qemu_hvf_start_vcpu (CPUState *cpu,Error **errp) {char thread_name[VCPU_THREAD_NAME_SIZE ]; +Error *local_err =NULL ;/* HVF currently does not support TCG, and only runs in * unrestricted-guest mode. */
@@ -1999,12 +2019,17 @@ 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 ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } }static void qemu_whpx_start_vcpu (CPUState *cpu,Error **errp) {char thread_name[VCPU_THREAD_NAME_SIZE ]; +Error *local_err =NULL ; cpu->thread =g_malloc0 (sizeof (QemuThread)); cpu->halt_cond =g_malloc0 (sizeof (QemuCond));
@@ -2012,7 +2037,11 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)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 ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }#ifdef _WIN32 cpu->hThread =qemu_thread_get_handle (cpu->thread);#endif
@@ -2021,6 +2050,7 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)static void qemu_dummy_start_vcpu (CPUState *cpu,Error **errp) {char thread_name[VCPU_THREAD_NAME_SIZE ]; +Error *local_err =NULL ; cpu->thread =g_malloc0 (sizeof (QemuThread)); cpu->halt_cond =g_malloc0 (sizeof (QemuCond));
@@ -2028,7 +2058,11 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)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 ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } }void qemu_init_vcpu (CPUState *cpu,Error **errp)
dump.c +5 -1
@@ -2022,7 +2022,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,/* detached dump */ s->detached = true;qemu_thread_create (&s->dump_thread,"dump_thread" , dump_thread, - s,QEMU_THREAD_DETACHED ); + s,QEMU_THREAD_DETACHED , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } }else {/* sync dump */ dump_process (s, errp);
hw/misc/edu.c +7 -1
@@ -29,6 +29,7 @@#include "qemu/timer.h" #include "qemu/main-loop.h" /* iothread mutex */ #include "qapi/visitor.h" +#include "qapi/error.h" #define EDU (obj) OBJECT_CHECK(EduState, obj, "edu")
@@ -343,6 +344,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp) {EduState *edu =DO_UPCAST (EduState, pdev, pdev);uint8_t *pci_conf = pdev->config; +Error *local_err =NULL ;pci_config_set_interrupt_pin (pci_conf,1 );
@@ -355,7 +357,11 @@ 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 ); + edu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }memory_region_init_io (&edu->mmio,OBJECT (edu), &edu_mmio_ops, edu,"edu-mmio" ,1 * MiB);
hw/ppc/spapr_hcall.c +8 -1
@@ -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 ;
@@ -539,7 +540,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, pending->ret =H_HARDWARE ;qemu_thread_create (&pending->thread,"sPAPR HPT prepare" , - hpt_prepare_thread, pending,QEMU_THREAD_DETACHED ); + hpt_prepare_thread, pending, +QEMU_THREAD_DETACHED , &local_err); +if (local_err) { +error_reportf_err (error_in,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); +return H_RESOURCE ; + } spapr->pending_hpt = pending;
hw/rdma/rdma_backend.c +2 -1
@@ -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,
hw/usb/ccid-card-emulated.c +11 -2
@@ -483,6 +483,7 @@ static void emulated_realize(CCIDCardState *base, Error **errp)EmulatedState *card =EMULATED_CCID_CARD (base);VCardEmulError ret;const EnumTable *ptable; +Error *local_err =NULL ;QSIMPLEQ_INIT (&card->event_list);QSIMPLEQ_INIT (&card->guest_apdu_list);
@@ -539,9 +540,17 @@ static void emulated_realize(CCIDCardState *base, Error **errp)return ; }qemu_thread_create (&card->event_thread_id,"ccid/event" , event_thread, - card,QEMU_THREAD_JOINABLE ); + card,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }qemu_thread_create (&card->apdu_thread_id,"ccid/apdu" , handle_apdu_thread, - card,QEMU_THREAD_JOINABLE ); + card,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } }static void emulated_unrealize (CCIDCardState *base,Error **errp)
io/task.c +2 -1
@@ -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); }
iothread.c +10 -5
@@ -160,10 +160,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) iothread->poll_shrink, &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);
@@ -176,9 +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 ); + iothread,QEMU_THREAD_JOINABLE , &local_error);g_free (thread_name);g_free (name); +if (local_error) { + goto fail; + }/* Wait for initialization to complete */ qemu_mutex_lock (&iothread->init_done_lock);
@@ -187,6 +187,11 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &iothread->init_done_lock); }qemu_mutex_unlock (&iothread->init_done_lock); +return ; +fail: +error_propagate (errp, local_error); +aio_context_unref (iothread->ctx); + iothread->ctx =NULL ; } typedef struct {
migration/migration.c +35 -12
88 lines not on this page · raw message
migration/postcopy-ram.c +10 -1
@@ -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 ) {
@@ -1109,7 +1111,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 ); + postcopy_ram_fault_thread, mis, +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); +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;
migration/ram.c +28 -4
@@ -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 ;
@@ -501,7 +502,12 @@ static int compress_threads_save_setup(void)qemu_cond_init (&comp_param[i].cond);qemu_thread_create (compress_threads + i,"compress" , do_data_compress, comp_param + i, -QEMU_THREAD_JOINABLE ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); + goto exit; + } }return 0 ;
@@ -1076,7 +1082,13 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)qio_channel_set_delay (p->c, false); p->running = true;qemu_thread_create (&p->thread, p->name, multifd_send_thread, p, -QEMU_THREAD_JOINABLE ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); +migrate_set_error (migrate_get_current (), local_err); +return ; + }atomic_inc (&multifd_send_state->count); }
@@ -1346,7 +1358,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc) p->running = true;qemu_thread_create (&p->thread, p->name, multifd_recv_thread, p, -QEMU_THREAD_JOINABLE ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); +multifd_recv_terminate_threads (local_err); +return false; + }atomic_inc (&multifd_recv_state->count);return multifd_recv_state->count ==migrate_multifd_channels (); }
@@ -3542,6 +3560,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 ;
@@ -3565,7 +3584,12 @@ static int compress_threads_load_setup(QEMUFile *f) decomp_param[i].quit = false;qemu_thread_create (decompress_threads + i,"decompress" , do_data_decompress, decomp_param + i, -QEMU_THREAD_JOINABLE ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); + goto exit; + } }return 0 ; exit:
migration/savevm.c +7 -1
@@ -1729,7 +1729,13 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)qemu_sem_init (&mis->listen_thread_sem,0 );qemu_thread_create (&mis->listen_thread,"postcopy/listen" , postcopy_ram_listen_thread,NULL , -QEMU_THREAD_DETACHED ); +QEMU_THREAD_DETACHED , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); +qemu_sem_destroy (&mis->listen_thread_sem); +return -1 ; + }qemu_sem_wait (&mis->listen_thread_sem);qemu_sem_destroy (&mis->listen_thread_sem);
tests/atomic_add-bench.c +2 -1
@@ -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); } }
tests/iothread.c +1 -1
@@ -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);
tests/qht-bench.c +2 -1
@@ -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); } }
tests/rcutorture.c +2 -1
@@ -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++; }
tests/test-aio.c +1 -1
@@ -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);
tests/test-rcu-list.c +2 -1
@@ -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++; }
ui/vnc-jobs.c +9 -2
@@ -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" /*
@@ -333,13 +334,19 @@ static bool vnc_worker_thread_running(void)void vnc_start_worker_thread (Error **errp) { +Error *local_err =NULL ; +VncJobQueue *q;if (vnc_worker_thread_running ()) -return ; +return ; q =vnc_queue_init ();qemu_thread_create (&q->thread,"vnc_worker" , vnc_worker_thread, q, -QEMU_THREAD_DETACHED ); +QEMU_THREAD_DETACHED , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } queue = q;/* Set global queue */ }
util/compatfd.c +7 -1
@@ -71,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)struct sigfd_compat_info *info;QemuThread thread;int fds[2 ]; +Error *local_err =NULL ; info =malloc (sizeof (*info));if (info ==NULL ) {
@@ -92,7 +93,12 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) info->fd = fds[1 ];qemu_thread_create (&thread,"signalfd_compat" , sigwait_compat, info, -QEMU_THREAD_DETACHED ); +QEMU_THREAD_DETACHED , &local_err); +if (local_err) { +error_propagate (errp, local_err); +free (info); +return -1 ; + }return fds[0 ]; }
util/oslib-posix.c +9 -1
@@ -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);
@@ -377,13 +378,20 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, memset_thread[i].hpagesize = hpagesize;qemu_thread_create (&memset_thread[i].pgthread,"touch_pages" , do_touch_pages, &memset_thread[i], -QEMU_THREAD_JOINABLE ); +QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_reportf_err (local_err,"Failed in %s() when calls " +"qemu_thread_create(): \n" , __func__); + 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 ;
util/rcu.c +2 -1
@@ -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 (); }
util/thread-pool.c +3 -1
@@ -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)
Signature · 1 line
2.13.7
Re: [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle
Quoted 43 lines · Fei Li
Let's propagate qemu_thread_create's error to make all callers check it. For those critical callers, just pass the
&error_abort.Signed-off-by Fei Li <[email protected]>--- cpus.c | 48 ++++++++++++++++++++++++++++++++++++++------- dump.c | 6 +++++- hw/misc/edu.c | 8 +++++++- hw/ppc/spapr_hcall.c | 9 ++++++++- hw/rdma/rdma_backend.c | 3 ++- hw/usb/ccid-card-emulated.c | 13 ++++++++++-- io/task.c | 3 ++- iothread.c | 15 +++++++++----- migration/migration.c | 47 ++++++++++++++++++++++++++++++++------------ migration/postcopy-ram.c | 11 ++++++++++- migration/ram.c | 32 ++++++++++++++++++++++++++---- migration/savevm.c | 8 +++++++- 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 | 11 +++++++++-- util/compatfd.c | 8 +++++++- util/oslib-posix.c | 10 +++++++++- util/rcu.c | 3 ++- util/thread-pool.c | 4 +++- 23 files changed, 206 insertions(+), 49 deletions(-)cpus.c +1
-0@@ -1904,6 +1904,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)static QemuCond *single_tcg_halt_cond;static QemuThread *single_tcg_cpu_thread;static int tcg_region_inited; +Error *local_err =NULL ;assert (tcg_enabled ());/*
cpus.c static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) · line 1937
Quoted 9 lines
cpus.c +5
-1@@ -1929,14 +1930,22 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) cpu->cpu_index);qemu_thread_create (cpu->thread, thread_name, qemu_tcg_cpu_thread_fn, - cpu,QEMU_THREAD_JOINABLE ); + cpu,QEMU_THREAD_JOINABLE , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + }
Having to use a local error object & error_propagate calls is making this patch larger than it needs.
I'd suggest changing qemu_thread_create() so that it returns a boolean, so it can then do
if (!qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
cpu, QEMU_THREAD_JOINABLE, errp) {
return;
}avoiding the local error object & shortening the code.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|