* [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check
@ 2018-09-04 11:08 Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails Fei Li
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 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(-)
--
2.13.7
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
@ 2018-09-04 11:08 ` Fei Li
2018-09-04 11:26 ` Daniel P. Berrangé
2018-09-04 11:08 ` [Qemu-devel] [PATCH 2/5] ui/vnc.c: polish vnc_init_func Fei Li
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: 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.
Signed-off-by: Fei Li <fli@suse.com>
---
include/qemu/osdep.h | 2 +-
util/compatfd.c | 17 ++++++++++++-----
util/main-loop.c | 11 +++++++----
3 files changed, 20 insertions(+), 10 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..65501de622 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 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;
}
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..20f6ad3849 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -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;
}
--
2.13.7
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/5] ui/vnc.c: polish vnc_init_func
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails Fei Li
@ 2018-09-04 11:08 ` Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate Fei Li
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: fli
Add 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.
Signed-off-by: Fei Li <fli@suse.com>
---
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(-)
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..7c05a1e6df 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -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;
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index 59f66bcc35..31eb482582 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);
+void 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..ff22bbc055 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -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: ");
--
2.13.7
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 2/5] ui/vnc.c: polish vnc_init_func Fei Li
@ 2018-09-04 11:08 ` Fei Li
2018-09-04 11:22 ` Daniel P. Berrangé
2018-09-04 11:08 ` [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle Fei Li
4 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 final caller check it.
Signed-off-by: Fei Li <fli@suse.com>
---
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(-)
diff --git a/cpus.c b/cpus.c
index 8ee6e5db93..41efddc218 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)
+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) {
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index dc130cd307..0766e694df 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);
+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 */
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index b08078e7fc..5b0b4892f2 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -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);
}
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 258ba6dcaa..a06a5629cd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -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);
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index a23aba2688..707ef63293 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -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);
}
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 00bf444620..45249a505a 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -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
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f24295e6e4..768039c65b 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -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
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index b7499cb627..7c4e4c4d88 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -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);
}
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 582e3a73b3..ed5c340242 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -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);
}
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 9b546a2c18..2d82a5885a 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -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 \
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 497706b669..3e21067f7a 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -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);
}
diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
index 8d67eb6727..c9e91d7e53 100644
--- a/target/moxie/cpu.c
+++ b/target/moxie/cpu.c
@@ -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);
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index fbfaa2ce26..be6601fc92 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -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);
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index fb7cb5c507..ee4c931280 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -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);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index d920d3e538..50980dec9a 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -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);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d630e8fd6c..5416cf86c2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -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);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8ed4823d6e..bd362e3775 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -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
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index b9f393b7c7..2ad3a8f09e 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -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);
}
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 0f090ece54..b3616f8d59 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -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);
}
diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
index bfe9be59b5..59c0850a7c 100644
--- a/target/tilegx/cpu.c
+++ b/target/tilegx/cpu.c
@@ -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);
}
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 2edaef1aef..c95d8e9856 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -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);
}
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index 68f978d80b..0102f4ea79 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -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);
}
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 590813d4f7..b6740c0d66 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -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);
}
--
2.13.7
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
` (2 preceding siblings ...)
2018-09-04 11:08 ` [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate Fei Li
@ 2018-09-04 11:08 ` Fei Li
2018-09-04 11:18 ` Daniel P. Berrangé
2018-09-04 11:08 ` [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle Fei Li
4 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: fli
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->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>
---
include/qemu/thread.h | 2 +-
util/qemu-thread-posix.c | 15 +++++++++++----
util/qemu-thread-win32.c | 12 +++++++++---
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index dacebcfff0..71d8be5851 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -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);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index dfa66ff2fb..a31c00abfd 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;
@@ -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)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 4a363ca675..725200abc9 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;
}
@@ -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;
--
2.13.7
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
` (3 preceding siblings ...)
2018-09-04 11:08 ` [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check Fei Li
@ 2018-09-04 11:08 ` Fei Li
2018-09-04 11:20 ` Daniel P. Berrangé
4 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-04 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: fli
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 <fli@suse.com>
---
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(-)
diff --git a/cpus.c b/cpus.c
index 41efddc218..24159af1e6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -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)
diff --git a/dump.c b/dump.c
index 500b554523..82d343f0e7 100644
--- a/dump.c
+++ b/dump.c
@@ -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);
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index df26a4d046..d884ee99ce 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -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);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ae913d070f..07d857410f 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;
@@ -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;
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..8f73f73ca4 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -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)
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..b19e548e5b 100644
--- a/iothread.c
+++ b/iothread.c
@@ -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 {
diff --git a/migration/migration.c b/migration/migration.c
index 4b316ec343..34af7b82b9 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();
@@ -421,7 +422,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);
+ colo_process_incoming_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__);
+ goto fail;
+ }
mis->have_colo_incoming_thread = true;
qemu_coroutine_yield();
@@ -430,20 +437,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 +2297,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) {
@@ -2302,7 +2312,13 @@ static int open_return_path_on_source(MigrationState *ms,
}
qemu_thread_create(&ms->rp_state.rp_thread, "return path",
- source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ source_return_path_thread, ms,
+ QEMU_THREAD_JOINABLE, &local_err);
+ if (local_err) {
+ error_reportf_err(local_err, "Failed in %s() when calls "
+ "qemu_thread_create(): \n", __func__);
+ return -1;
+ }
trace_open_return_path_on_source_continue();
@@ -3128,7 +3144,14 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
return;
}
qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_in);
+ if (error_in) {
+ error_reportf_err(error_in, "Failed in %s() when calls "
+ "qemu_thread_create(): \n", __func__);
+ 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..85d9114bd1 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) {
@@ -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;
diff --git a/migration/ram.c b/migration/ram.c
index 79c89425a3..ebbd21f8c2 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;
@@ -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:
diff --git a/migration/savevm.c b/migration/savevm.c
index 13e51f0e34..31973312cf 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -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);
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 7c05a1e6df..60d5ceb6a8 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"
/*
@@ -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 */
}
diff --git a/util/compatfd.c b/util/compatfd.c
index 65501de622..b3d3c82266 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -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];
}
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 13b6f8d776..91a7921a57 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);
@@ -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;
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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check
2018-09-04 11:08 ` [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check Fei Li
@ 2018-09-04 11:18 ` Daniel P. Berrangé
2018-09-05 4:20 ` Fei Li
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-09-04 11:18 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
On Tue, Sep 04, 2018 at 07:08:21PM +0800, Fei Li wrote:
> 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->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>
> ---
> include/qemu/thread.h | 2 +-
> util/qemu-thread-posix.c | 15 +++++++++++----
> util/qemu-thread-win32.c | 12 +++++++++---
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index dacebcfff0..71d8be5851 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -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
--
|: 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle
2018-09-04 11:08 ` [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle Fei Li
@ 2018-09-04 11:20 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-09-04 11:20 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
On Tue, Sep 04, 2018 at 07:08:22PM +0800, Fei Li wrote:
> 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 <fli@suse.com>
> ---
> 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(-)
>
> diff --git a/cpus.c b/cpus.c
> index 41efddc218..24159af1e6 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -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;
> + }
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
--
|: 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
2018-09-04 11:08 ` [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate Fei Li
@ 2018-09-04 11:22 ` Daniel P. Berrangé
2018-09-05 4:36 ` Fei Li
2018-09-05 17:15 ` Eric Blake
0 siblings, 2 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-09-04 11:22 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
> 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 <fli@suse.com>
> ---
> 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(-)
>
> diff --git a/cpus.c b/cpus.c
> index 8ee6e5db93..41efddc218 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)
> +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;
> }
I'd be inclined to make this method return a boolean, so....
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index b08078e7fc..5b0b4892f2 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -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
--
|: 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-04 11:08 ` [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails Fei Li
@ 2018-09-04 11:26 ` Daniel P. Berrangé
2018-09-05 4:17 ` Fei Li
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-09-04 11:26 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
On Tue, Sep 04, 2018 at 07:08:18PM +0800, 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.
>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
> include/qemu/osdep.h | 2 +-
> util/compatfd.c | 17 ++++++++++++-----
> util/main-loop.c | 11 +++++++----
> 3 files changed, 20 insertions(+), 10 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..65501de622 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 malloc in %s", __func__);
I'd avoid using __func__ in error messages. Instead
"Failed to allocate signalfd memory"
> errno = ENOMEM;
> return -1;
> }
>
> if (pipe(fds) == -1) {
> + error_setg(errp, "Failed to create a pipe in %s", __func__);
"Failed to create signalfd pipe"
> 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);
> + }
Using a local_err is not required - you can just pass errp stright
to qemu_signalfd_compat() and then check
if (ret < 0)
> + return ret;
> }
> diff --git a/util/main-loop.c b/util/main-loop.c
> index affe0403c5..20f6ad3849 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -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;
> }
Again, no need for local_err - just pass errp stright in
>
> @@ -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;
> }
Again, no need for local_error.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-04 11:26 ` Daniel P. Berrangé
@ 2018-09-05 4:17 ` Fei Li
2018-09-05 8:36 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-05 4:17 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
Thanks for the review! :)
On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
> On Tue, Sep 04, 2018 at 07:08:18PM +0800, 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.
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> include/qemu/osdep.h | 2 +-
>> util/compatfd.c | 17 ++++++++++++-----
>> util/main-loop.c | 11 +++++++----
>> 3 files changed, 20 insertions(+), 10 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..65501de622 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 malloc in %s", __func__);
> I'd avoid using __func__ in error messages. Instead
>
> "Failed to allocate signalfd memory"
Ok, thanks.
>
>> errno = ENOMEM;
>> return -1;
>> }
>>
>> if (pipe(fds) == -1) {
>> + error_setg(errp, "Failed to create a pipe in %s", __func__);
> "Failed to create signalfd pipe"
OK.
>
>> 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);
>> + }
> Using a local_err is not required - you can just pass errp stright
> to qemu_signalfd_compat() and then check
>
> 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
>> + return ret;
>> }
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index affe0403c5..20f6ad3849 100644
>> --- a/util/main-loop.c
>> +++ b/util/main-loop.c
>> @@ -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;
>> }
> Again, no need for local_err - just pass errp stright in
>
>>
>> @@ -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;
>> }
> Again, no need for local_error.
>
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check
2018-09-04 11:18 ` Daniel P. Berrangé
@ 2018-09-05 4:20 ` Fei Li
0 siblings, 0 replies; 19+ messages in thread
From: Fei Li @ 2018-09-05 4:20 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On 09/04/2018 07:18 PM, Daniel P. Berrangé wrote:
> On Tue, Sep 04, 2018 at 07:08:21PM +0800, Fei Li wrote:
>> 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->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>
>> ---
>> include/qemu/thread.h | 2 +-
>> util/qemu-thread-posix.c | 15 +++++++++++----
>> util/qemu-thread-win32.c | 12 +++++++++---
>> 3 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
>> index dacebcfff0..71d8be5851 100644
>> --- a/include/qemu/thread.h
>> +++ b/include/qemu/thread.h
>> @@ -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
Oops, thanks for the reminder! Will merge them into one in the next version.
Have a nice day
Fei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
2018-09-04 11:22 ` Daniel P. Berrangé
@ 2018-09-05 4:36 ` Fei Li
2018-09-05 17:15 ` Eric Blake
1 sibling, 0 replies; 19+ messages in thread
From: Fei Li @ 2018-09-05 4:36 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On 09/04/2018 07:22 PM, Daniel P. Berrangé wrote:
> On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
>> 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 <fli@suse.com>
>> ---
>> 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(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 8ee6e5db93..41efddc218 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)
>> +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;
>> }
> I'd be inclined to make this method return a boolean, so....
>
>
>> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
>> index b08078e7fc..5b0b4892f2 100644
>> --- a/target/alpha/cpu.c
>> +++ b/target/alpha/cpu.c
>> @@ -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
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
>
> if (!qemu_init_vcpu(cs, errp)) {
> return;
> }
>
> likewise for the rest of the patch below...
>
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-05 4:17 ` Fei Li
@ 2018-09-05 8:36 ` Daniel P. Berrangé
2018-09-05 11:20 ` Fei Li
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-09-05 8:36 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel
On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
> Thanks for the review! :)
>
>
> On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
> > On Tue, Sep 04, 2018 at 07:08:18PM +0800, 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.
> > >
> > > Signed-off-by: Fei Li <fli@suse.com>
> > > ---
> > > include/qemu/osdep.h | 2 +-
> > > util/compatfd.c | 17 ++++++++++++-----
> > > util/main-loop.c | 11 +++++++----
> > > 3 files changed, 20 insertions(+), 10 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..65501de622 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 malloc in %s", __func__);
> > I'd avoid using __func__ in error messages. Instead
> >
> > "Failed to allocate signalfd memory"
> Ok, thanks.
> >
> > > errno = ENOMEM;
> > > return -1;
> > > }
> > > if (pipe(fds) == -1) {
> > > + error_setg(errp, "Failed to create a pipe in %s", __func__);
> > "Failed to create signalfd pipe"
> OK.
> >
> > > 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);
> > > + }
> > Using a local_err is not required - you can just pass errp stright
> > to qemu_signalfd_compat() and then check
> >
> > 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.
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.
> 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.
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
--
|: 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-05 8:36 ` Daniel P. Berrangé
@ 2018-09-05 11:20 ` Fei Li
2018-09-05 14:38 ` Fam Zheng
0 siblings, 1 reply; 19+ messages in thread
From: Fei Li @ 2018-09-05 11:20 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On 09/05/2018 04:36 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
>> Thanks for the review! :)
>>
>>
>> On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
>>> On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
>>>
... snip ...
>>>> 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);
>>>> + }
>>> Using a local_err is not required - you can just pass errp stright
>>> to qemu_signalfd_compat() and then check
>>>
>>> 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.
> 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..
>
>> 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.
> 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.
>
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-05 11:20 ` Fei Li
@ 2018-09-05 14:38 ` Fam Zheng
2018-09-06 6:31 ` Fei Li
0 siblings, 1 reply; 19+ messages in thread
From: Fam Zheng @ 2018-09-05 14:38 UTC (permalink / raw)
To: Fei Li; +Cc: Daniel P. Berrangé, qemu-devel
On Wed, 09/05 19:20, Fei Li wrote:
>
>
> On 09/05/2018 04:36 PM, Daniel P. Berrangé wrote:
> > On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
> > > Thanks for the review! :)
> > >
> > >
> > > On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
> > > > On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
> > > >
> ... snip ...
> > > > > 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);
> > > > > + }
> > > > Using a local_err is not required - you can just pass errp stright
> > > > to qemu_signalfd_compat() and then check
> > > >
> > > > 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.
> > 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..
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
> >
> > > 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.
> > 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.
> >
> > 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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
2018-09-04 11:22 ` Daniel P. Berrangé
2018-09-05 4:36 ` Fei Li
@ 2018-09-05 17:15 ` Eric Blake
2018-09-06 6:52 ` Fei Li
1 sibling, 1 reply; 19+ messages in thread
From: Eric Blake @ 2018-09-05 17:15 UTC (permalink / raw)
To: Daniel P. Berrangé, Fei Li; +Cc: qemu-devel, Markus Armbruster
Adding Markus, as the maintainer of Error APIs, to make sure he sees
this thread.
On 09/04/2018 06:22 AM, Daniel P. Berrangé wrote:
> On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
In the subject line: s/paramater/parameter/
>> 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 <fli@suse.com>
>> ---
>> -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;
>> }
>
> I'd be inclined to make this method return a boolean, so....
>
>
>> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
>> index b08078e7fc..5b0b4892f2 100644
>> --- a/target/alpha/cpu.c
>> +++ b/target/alpha/cpu.c
>> @@ -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;
> }
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails
2018-09-05 14:38 ` Fam Zheng
@ 2018-09-06 6:31 ` Fei Li
0 siblings, 0 replies; 19+ messages in thread
From: Fei Li @ 2018-09-06 6:31 UTC (permalink / raw)
To: Fam Zheng; +Cc: Daniel P. Berrangé, qemu-devel
On 09/05/2018 10:38 PM, Fam Zheng wrote:
> On Wed, 09/05 19:20, Fei Li wrote:
>>
>> On 09/05/2018 04:36 PM, Daniel P. Berrangé wrote:
>>> On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
>>>> Thanks for the review! :)
>>>>
>>>>
>>>> On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
>>>>> On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
>>>>>
>> ... snip ...
>>>>>> 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);
>>>>>> + }
>>>>> Using a local_err is not required - you can just pass errp stright
>>>>> to qemu_signalfd_compat() and then check
>>>>>
>>>>> 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.
>>> 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..
> 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
Got, thanks for the review. :)
Have a nice day
Fei
>>>> 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.
>>> 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.
>>> 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
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
2018-09-05 17:15 ` Eric Blake
@ 2018-09-06 6:52 ` Fei Li
0 siblings, 0 replies; 19+ messages in thread
From: Fei Li @ 2018-09-06 6:52 UTC (permalink / raw)
To: Eric Blake, Daniel P. Berrangé; +Cc: qemu-devel, Markus Armbruster
On 09/06/2018 01:15 AM, Eric Blake wrote:
> Adding Markus, as the maintainer of Error APIs, to make sure he sees
> this thread.
Thanks. :)
>
> On 09/04/2018 06:22 AM, Daniel P. Berrangé wrote:
>> On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:
>
> In the subject line: s/paramater/parameter/
Thanks for pointing this out! :)
>
>>> 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 <fli@suse.com>
>>> ---
>
>>> -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;
>>> }
>>
>> I'd be inclined to make this method return a boolean, so....
>>
>>
>>> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
>>> index b08078e7fc..5b0b4892f2 100644
>>> --- a/target/alpha/cpu.c
>>> +++ b/target/alpha/cpu.c
>>> @@ -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;
>> }
>
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-09-06 6:52 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 11:08 [Qemu-devel] [PATCH 0/5] qemu_thread_create: propagate errors to callers to check Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails Fei Li
2018-09-04 11:26 ` Daniel P. Berrangé
2018-09-05 4:17 ` Fei Li
2018-09-05 8:36 ` Daniel P. Berrangé
2018-09-05 11:20 ` Fei Li
2018-09-05 14:38 ` Fam Zheng
2018-09-06 6:31 ` Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 2/5] ui/vnc.c: polish vnc_init_func Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate Fei Li
2018-09-04 11:22 ` Daniel P. Berrangé
2018-09-05 4:36 ` Fei Li
2018-09-05 17:15 ` Eric Blake
2018-09-06 6:52 ` Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 4/5] qemu_thread_create: propagate the error to callers to check Fei Li
2018-09-04 11:18 ` Daniel P. Berrangé
2018-09-05 4:20 ` Fei Li
2018-09-04 11:08 ` [Qemu-devel] [PATCH 5/5] Propagate qemu_thread_create's error to all callers to handle Fei Li
2018-09-04 11:20 ` Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.