* [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check
@ 2018-10-10 12:08 Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails Fei Li
` (6 more replies)
0 siblings, 7 replies; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
Hi,
This idea comes from BiteSizedTasks, and this patch series implement
the error checking of qemu_thread_create: make qemu_thread_create
return a flag to indicate if it succeeded rather than failing with an
error; make all callers check it.
The first three patches apply to those call traces whose further
callers already have the *errp to pass, thus add a new Error paramater
in the call trace to propagate the error and let the further caller
check it. The last patch modifies the qemu_thread_create() and makes
it return a bool to all direct callers to indicate if it succeeds.
The middle three fix some segmentation fault when using GDB to debug.
###### Here I have a concern about using multifd migration. ######
It is about "[PATCH RFC v4 5/7] migration: fix the multifd code".
In our current code, when multifd is used during migration, if there
is an error before the destination receives all new channels (I mean
multifd_recv_new_channel(ioc)), the destination does not exit but
keeps waiting (Hang in recvmsg() in qio_channel_socket_readv) until
the source exits. I know hang is not right, but so sorry that I did
not come up with a good idea on handling this. Not so familiar with
that part of code.. Hope @Juan could give a hand. Thanks!
Please help to review, thanks. :)
v5:
- Remove `errno = err` in qemu_thread_create() for Linux, and change
`return errno` to `return -1` in qemu_signal_init() to indicate
the error in case qemu_thread_create() fails.
- Delete the v4-added qemu_cond/mutex_destroy() in iothread_complete()
as the destroy() will be done by its callers' object_unref().
v4:
- Separate the migration compression patch from this series
- Add one more error handling patch related with migration
- Add more cleaning up code for touched functions
v3:
- Add two migration related patches to fix the segmentaion fault
- Extract the segmentation fault fix from v2's last patch to be a
separate patch
- Add cleaning up code for touched functions
- Update some error messages
v2:
- Pass errp straightly instead of using a local_err & error_propagate
- Return a bool: false/true to indicate if one function succeeds
- Merge v1's last two patches into one to avoid the compile error
- Fix one omitted error in patch1 and update some error messages
Fei Li (7):
Fix segmentation fault when qemu_signal_init fails
ui/vnc.c: polish vnc_init_func
qemu_init_vcpu: add a new Error parameter to propagate
qemu_thread_join: fix segmentation fault
migration: fix the multifd code
migration: fix some error handling
qemu_thread_create: propagate the error to callers to handle
accel/tcg/user-exec-stub.c | 2 +-
cpus.c | 79 ++++++++++++++++++++++++++---------------
dump.c | 6 ++--
hw/misc/edu.c | 6 ++--
hw/ppc/spapr_hcall.c | 10 ++++--
hw/rdma/rdma_backend.c | 4 ++-
hw/usb/ccid-card-emulated.c | 13 ++++---
include/qemu/osdep.h | 2 +-
include/qemu/thread.h | 4 +--
include/qom/cpu.h | 2 +-
include/ui/console.h | 2 +-
io/task.c | 3 +-
iothread.c | 16 ++++++---
migration/migration.c | 54 ++++++++++++++++++----------
migration/postcopy-ram.c | 17 +++++++--
migration/ram.c | 68 ++++++++++++++++++++++++-----------
migration/ram.h | 2 +-
migration/savevm.c | 11 ++++--
target/alpha/cpu.c | 4 ++-
target/arm/cpu.c | 4 ++-
target/cris/cpu.c | 4 ++-
target/hppa/cpu.c | 4 ++-
target/i386/cpu.c | 4 ++-
target/lm32/cpu.c | 4 ++-
target/m68k/cpu.c | 4 ++-
target/microblaze/cpu.c | 4 ++-
target/mips/cpu.c | 4 ++-
target/moxie/cpu.c | 4 ++-
target/nios2/cpu.c | 4 ++-
target/openrisc/cpu.c | 4 ++-
target/ppc/translate_init.inc.c | 4 ++-
target/riscv/cpu.c | 4 ++-
target/s390x/cpu.c | 4 ++-
target/sh4/cpu.c | 4 ++-
target/sparc/cpu.c | 4 ++-
target/tilegx/cpu.c | 4 ++-
target/tricore/cpu.c | 4 ++-
target/unicore32/cpu.c | 4 ++-
target/xtensa/cpu.c | 4 ++-
tests/atomic_add-bench.c | 3 +-
tests/iothread.c | 2 +-
tests/qht-bench.c | 3 +-
tests/rcutorture.c | 3 +-
tests/test-aio.c | 2 +-
tests/test-rcu-list.c | 3 +-
ui/vnc-jobs.c | 17 ++++++---
ui/vnc-jobs.h | 2 +-
ui/vnc.c | 12 +++++--
util/compatfd.c | 18 +++++++---
util/main-loop.c | 11 +++---
util/oslib-posix.c | 17 ++++++---
util/qemu-thread-posix.c | 25 ++++++++-----
util/qemu-thread-win32.c | 18 +++++++---
util/rcu.c | 3 +-
util/thread-pool.c | 4 ++-
55 files changed, 366 insertions(+), 162 deletions(-)
--
2.13.7
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-11 10:02 ` Markus Armbruster
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func Fei Li
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
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>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
include/qemu/osdep.h | 2 +-
util/compatfd.c | 9 ++++++---
util/main-loop.c | 9 ++++-----
3 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4f8559e550..f1f56763a0 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
additional fields in the future) */
};
-int qemu_signalfd(const sigset_t *mask);
+int qemu_signalfd(const sigset_t *mask, Error **errp);
void sigaction_invoke(struct sigaction *action,
struct qemu_signalfd_siginfo *info);
#endif
diff --git a/util/compatfd.c b/util/compatfd.c
index 980bd33e52..d3ed890405 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -16,6 +16,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
#include "qemu/thread.h"
+#include "qapi/error.h"
#include <sys/syscall.h>
@@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
}
}
-static int qemu_signalfd_compat(const sigset_t *mask)
+static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
{
struct sigfd_compat_info *info;
QemuThread thread;
@@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
info = malloc(sizeof(*info));
if (info == NULL) {
+ error_setg(errp, "Failed to allocate signalfd memory");
errno = ENOMEM;
return -1;
}
if (pipe(fds) == -1) {
+ error_setg(errp, "Failed to create signalfd pipe");
free(info);
return -1;
}
@@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
return fds[0];
}
-int qemu_signalfd(const sigset_t *mask)
+int qemu_signalfd(const sigset_t *mask, Error **errp)
{
#if defined(CONFIG_SIGNALFD)
int ret;
@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
}
#endif
- return qemu_signalfd_compat(mask);
+ return qemu_signalfd_compat(mask, errp);
}
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..9671b6d226 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
}
}
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
{
int sigfd;
sigset_t set;
@@ -94,9 +94,8 @@ static int qemu_signal_init(void)
pthread_sigmask(SIG_BLOCK, &set, NULL);
sigdelset(&set, SIG_IPI);
- sigfd = qemu_signalfd(&set);
+ sigfd = qemu_signalfd(&set, errp);
if (sigfd == -1) {
- fprintf(stderr, "failed to create signalfd\n");
return -errno;
}
@@ -109,7 +108,7 @@ static int qemu_signal_init(void)
#else /* _WIN32 */
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
{
return 0;
}
@@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
init_clocks(qemu_timer_notify_cb);
- ret = qemu_signal_init();
+ ret = qemu_signal_init(errp);
if (ret) {
return ret;
}
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-11 13:13 ` Markus Armbruster
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate Fei Li
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
Add a new Error parameter for vnc_display_init() to handle errors
in its caller: vnc_init_func(), just like vnc_display_open() does.
And let the call trace propagate the Error.
Besides, make vnc_start_worker_thread() return a bool to indicate
whether it succeeds instead of returning nothing.
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
include/ui/console.h | 2 +-
ui/vnc-jobs.c | 9 ++++++---
ui/vnc-jobs.h | 2 +-
ui/vnc.c | 12 +++++++++---
4 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index fb969caf70..c17803c530 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
/* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
void vnc_display_open(const char *id, Error **errp);
void vnc_display_add_client(const char *id, int csock, bool skipauth);
int vnc_display_password(const char *id, const char *password);
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 929391f85d..8807d7217c 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
return queue; /* Check global queue */
}
-void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
{
VncJobQueue *q;
- if (vnc_worker_thread_running())
- return ;
+ if (vnc_worker_thread_running()) {
+ goto out;
+ }
q = vnc_queue_init();
qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
QEMU_THREAD_DETACHED);
queue = q; /* Set global queue */
+out:
+ return true;
}
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index 59f66bcc35..14640593db 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
void vnc_jobs_join(VncState *vs);
void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
/* Locks */
static inline int vnc_trylock_display(VncDisplay *vd)
diff --git a/ui/vnc.c b/ui/vnc.c
index cf221c83cc..f3806e76db 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_cursor_define = vnc_dpy_cursor_define,
};
-void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
{
VncDisplay *vd;
@@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
vd->connections_limit = 32;
qemu_mutex_init(&vd->mutex);
- vnc_start_worker_thread();
+ if (!vnc_start_worker_thread(errp)) {
+ return;
+ }
vd->dcl.ops = &dcl_ops;
register_displaychangelistener(&vd->dcl);
@@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
char *id = (char *)qemu_opts_id(opts);
assert(id);
- vnc_display_init(id);
+ vnc_display_init(id, &local_err);
+ if (local_err) {
+ error_reportf_err(local_err, "Failed to init VNC server: ");
+ exit(1);
+ }
vnc_display_open(id, &local_err);
if (local_err != NULL) {
error_reportf_err(local_err, "Failed to start VNC server: ");
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-11 13:19 ` Markus Armbruster
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 4/7] qemu_thread_join: fix segmentation fault Fei Li
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
The caller of qemu_init_vcpu() already passed the **errp to handle
errors. In view of this, add a new Error parameter to the following
call trace to propagate the error and let the further caller check it.
Besides, make qemu_init_vcpu() return a Boolean value to let its
callers know whether it succeeds.
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
accel/tcg/user-exec-stub.c | 2 +-
cpus.c | 34 +++++++++++++++++++++-------------
include/qom/cpu.h | 2 +-
target/alpha/cpu.c | 4 +++-
target/arm/cpu.c | 4 +++-
target/cris/cpu.c | 4 +++-
target/hppa/cpu.c | 4 +++-
target/i386/cpu.c | 4 +++-
target/lm32/cpu.c | 4 +++-
target/m68k/cpu.c | 4 +++-
target/microblaze/cpu.c | 4 +++-
target/mips/cpu.c | 4 +++-
target/moxie/cpu.c | 4 +++-
target/nios2/cpu.c | 4 +++-
target/openrisc/cpu.c | 4 +++-
target/ppc/translate_init.inc.c | 4 +++-
target/riscv/cpu.c | 4 +++-
target/s390x/cpu.c | 4 +++-
target/sh4/cpu.c | 4 +++-
target/sparc/cpu.c | 4 +++-
target/tilegx/cpu.c | 4 +++-
target/tricore/cpu.c | 4 +++-
target/unicore32/cpu.c | 4 +++-
target/xtensa/cpu.c | 4 +++-
24 files changed, 86 insertions(+), 36 deletions(-)
diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index a32b4496af..38f6b928d4 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
{
}
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
{
}
diff --git a/cpus.c b/cpus.c
index 361678e459..c4db70607e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1918,7 +1918,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;
@@ -1974,7 +1974,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];
@@ -1991,7 +1991,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];
@@ -2004,7 +2004,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];
@@ -2022,7 +2022,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];
@@ -2038,7 +2038,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];
@@ -2051,11 +2051,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
QEMU_THREAD_JOINABLE);
}
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
{
cpu->nr_cores = smp_cores;
cpu->nr_threads = smp_threads;
cpu->stopped = true;
+ Error *local_err = NULL;
if (!cpu->as) {
/* If the target cpu hasn't set up any address spaces itself,
@@ -2066,22 +2067,29 @@ void qemu_init_vcpu(CPUState *cpu)
}
if (kvm_enabled()) {
- qemu_kvm_start_vcpu(cpu);
+ qemu_kvm_start_vcpu(cpu, &local_err);
} else if (hax_enabled()) {
- qemu_hax_start_vcpu(cpu);
+ qemu_hax_start_vcpu(cpu, &local_err);
} else if (hvf_enabled()) {
- qemu_hvf_start_vcpu(cpu);
+ qemu_hvf_start_vcpu(cpu, &local_err);
} else if (tcg_enabled()) {
- qemu_tcg_init_vcpu(cpu);
+ qemu_tcg_init_vcpu(cpu, &local_err);
} else if (whpx_enabled()) {
- qemu_whpx_start_vcpu(cpu);
+ qemu_whpx_start_vcpu(cpu, &local_err);
} else {
- qemu_dummy_start_vcpu(cpu);
+ qemu_dummy_start_vcpu(cpu, &local_err);
+ }
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return false;
}
while (!cpu->created) {
qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
}
+
+ return true;
}
void cpu_stop_current(void)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index dc130cd307..4d85dda175 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -1012,7 +1012,7 @@ void end_exclusive(void);
*
* Initializes a vCPU.
*/
-void qemu_init_vcpu(CPUState *cpu);
+bool qemu_init_vcpu(CPUState *cpu, Error **errp);
#define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
#define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index b08078e7fc..d531bd4f7e 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
acc->parent_realize(dev, errp);
}
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b5e61cc177..40f300174d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1038,7 +1038,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
}
#endif
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
cpu_reset(cs);
acc->parent_realize(dev, errp);
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index a23aba2688..ec92d69781 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
}
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
ccc->parent_realize(dev, errp);
}
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 00bf444620..08f600ced9 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
acc->parent_realize(dev, errp);
#ifndef CONFIG_USER_ONLY
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c88876dfe3..d199f91258 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
}
#endif
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
/*
* Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index b7499cb627..d50b1e4a43 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
lcc->parent_realize(dev, errp);
}
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 582e3a73b3..4ab53f2d58 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
m68k_cpu_init_gdb(cpu);
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
mcc->parent_realize(dev, errp);
}
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 9b546a2c18..3906c864a3 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
env->pvr.regs[0] = PVR0_USE_EXC_MASK \
| PVR0_USE_ICACHE_MASK \
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 497706b669..62be84af76 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
cpu_mips_realize_env(&cpu->env);
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
mcc->parent_realize(dev, errp);
}
diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
index 8d67eb6727..8581a6d922 100644
--- a/target/moxie/cpu.c
+++ b/target/moxie/cpu.c
@@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
cpu_reset(cs);
mcc->parent_realize(dev, errp);
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index fbfaa2ce26..5c7b4b486e 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
cpu_reset(cs);
ncc->parent_realize(dev, errp);
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index fb7cb5c507..a6dcdb9df9 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
cpu_reset(cs);
occ->parent_realize(dev, errp);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 263e63cb03..a6dd2318a6 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
32, "power-vsx.xml", 0);
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ goto unrealize;
+ }
pcc->parent_realize(dev, errp);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d630e8fd6c..ef56215e92 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
cpu_reset(cs);
mcc->parent_realize(dev, errp);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 18ba7f85a5..2a3eac9761 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
#endif
s390_cpu_gdb_init(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
/*
* KVM requires the initial CPU reset ioctl to be executed on the target
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index b9f393b7c7..d32ef2e1cb 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
}
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
scc->parent_realize(dev, errp);
}
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 0f090ece54..9c22f6a7df 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
scc->parent_realize(dev, errp);
}
diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
index bfe9be59b5..234148fabd 100644
--- a/target/tilegx/cpu.c
+++ b/target/tilegx/cpu.c
@@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
}
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
tcc->parent_realize(dev, errp);
}
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 2edaef1aef..5482d6ea3f 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
set_feature(env, TRICORE_FEATURE_13);
}
cpu_reset(cs);
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
tcc->parent_realize(dev, errp);
}
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index 68f978d80b..1f6a33b6f3 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
return;
}
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
ucc->parent_realize(dev, errp);
}
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index a54dbe4260..d2351c9b20 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
- qemu_init_vcpu(cs);
+ if (!qemu_init_vcpu(cs, errp)) {
+ return;
+ }
xcc->parent_realize(dev, errp);
}
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 4/7] qemu_thread_join: fix segmentation fault
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
` (2 preceding siblings ...)
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code Fei Li
` (2 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
To avoid the segmentation fault in qemu_thread_join(), just directly
return when the QemuThread *thread failed to be created in either
qemu-thread-posix.c or qemu-thread-win32.c.
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
util/qemu-thread-posix.c | 3 +++
util/qemu-thread-win32.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index dfa66ff2fb..289af4fab5 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -562,6 +562,9 @@ void *qemu_thread_join(QemuThread *thread)
int err;
void *ret;
+ if (!thread->thread) {
+ return NULL;
+ }
err = pthread_join(thread->thread, &ret);
if (err) {
error_exit(err, __func__);
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 4a363ca675..1a27e1cf6f 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -366,7 +366,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;
}
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
` (3 preceding siblings ...)
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 4/7] qemu_thread_join: fix segmentation fault Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-11 13:28 ` Markus Armbruster
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 6/7] migration: fix some error handling Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle Fei Li
6 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
When multifd is used during migration, if there is an error before
the destination receives all new channels, the destination does not
exit but keeps waiting in our current code. However, a segmentaion
fault will occur in the source when multifd_save_cleanup() is called
again as the multifd_send_state has been freed earlier in the first
error handling. This can happen when migrate_fd_connect() fails and
multifd_fd_cleanup() is called, and then multifd_new_send_channel_
async() fails and multifd_save_cleanup() is called again.
If the QIOChannel *c of multifd_recv_state->params[i] (p->c) is not
initialized, there is no need to close the channel. Or else a
segmentation fault will occur in multifd_recv_terminate_threads()
when multifd_recv_initial_packet() fails.
Signed-off-by: Fei Li <fli@suse.com>
---
migration/ram.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index bc38d98cc3..41dc94c059 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -907,6 +907,11 @@ static void multifd_send_terminate_threads(Error *err)
}
}
+ /* in case multifd_send_state has been freed earlier */
+ if (!multifd_send_state) {
+ return;
+ }
+
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDSendParams *p = &multifd_send_state->params[i];
@@ -922,7 +927,7 @@ int multifd_save_cleanup(Error **errp)
int i;
int ret = 0;
- if (!migrate_use_multifd()) {
+ if (!migrate_use_multifd() || !multifd_send_state) {
return 0;
}
multifd_send_terminate_threads(NULL);
@@ -1131,7 +1136,7 @@ struct {
uint64_t packet_num;
} *multifd_recv_state;
-static void multifd_recv_terminate_threads(Error *err)
+static void multifd_recv_terminate_threads(Error *err, bool channel)
{
int i;
@@ -1145,6 +1150,11 @@ static void multifd_recv_terminate_threads(Error *err)
}
}
+ /* in case p->c is not initialized */
+ if (!channel) {
+ return;
+ }
+
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDRecvParams *p = &multifd_recv_state->params[i];
@@ -1166,7 +1176,7 @@ int multifd_load_cleanup(Error **errp)
if (!migrate_use_multifd()) {
return 0;
}
- multifd_recv_terminate_threads(NULL);
+ multifd_recv_terminate_threads(NULL, true);
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDRecvParams *p = &multifd_recv_state->params[i];
@@ -1269,7 +1279,7 @@ static void *multifd_recv_thread(void *opaque)
}
if (local_err) {
- multifd_recv_terminate_threads(local_err);
+ multifd_recv_terminate_threads(local_err, true);
}
qemu_mutex_lock(&p->mutex);
p->running = false;
@@ -1331,7 +1341,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
id = multifd_recv_initial_packet(ioc, &local_err);
if (id < 0) {
- multifd_recv_terminate_threads(local_err);
+ multifd_recv_terminate_threads(local_err, false);
return false;
}
@@ -1339,7 +1349,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
if (p->c != NULL) {
error_setg(&local_err, "multifd: received id '%d' already setup'",
id);
- multifd_recv_terminate_threads(local_err);
+ multifd_recv_terminate_threads(local_err, true);
return false;
}
p->c = ioc;
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 6/7] migration: fix some error handling
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
` (4 preceding siblings ...)
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle Fei Li
6 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
Add error handling for qemu_ram_foreach_migratable_block() when
it fails.
Always call migrate_set_error() to set the error state without relying
on whether multifd_save_cleanup() succeeds. As the passed &local_err
is never used in multifd_save_cleanup(), remove it.
Signed-off-by: Fei Li <fli@suse.com>
---
migration/migration.c | 5 +----
migration/postcopy-ram.c | 3 +++
migration/ram.c | 7 +++----
migration/ram.h | 2 +-
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index d6ae879dc8..c344bab8f1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1341,7 +1341,6 @@ static void migrate_fd_cleanup(void *opaque)
qemu_savevm_state_cleanup();
if (s->to_dst_file) {
- Error *local_err = NULL;
QEMUFile *tmp;
trace_migrate_fd_cleanup();
@@ -1352,9 +1351,7 @@ static void migrate_fd_cleanup(void *opaque)
}
qemu_mutex_lock_iothread();
- if (multifd_save_cleanup(&local_err) != 0) {
- error_report_err(local_err);
- }
+ multifd_save_cleanup();
qemu_mutex_lock(&s->qemu_file_lock);
tmp = s->to_dst_file;
s->to_dst_file = NULL;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 853d8b32ca..f96e0ae1f8 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1116,6 +1116,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
/* Mark so that we get notified of accesses to unwritten areas */
if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+ error_report("ram_block_enable_notify failed");
+ close(mis->userfault_event_fd);
+ close(mis->userfault_fd);
return -1;
}
diff --git a/migration/ram.c b/migration/ram.c
index 41dc94c059..25ec8f1b54 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
}
}
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
{
int i;
int ret = 0;
@@ -1076,9 +1076,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
Error *local_err = NULL;
if (qio_task_propagate_error(task, &local_err)) {
- if (multifd_save_cleanup(&local_err) != 0) {
- migrate_set_error(migrate_get_current(), local_err);
- }
+ multifd_save_cleanup();
+ migrate_set_error(migrate_get_current(), local_err);
} else {
p->c = QIO_CHANNEL(sioc);
qio_channel_set_delay(p->c, false);
diff --git a/migration/ram.h b/migration/ram.h
index a139066846..0b0e4d8717 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
uint64_t ram_bytes_total(void);
int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
int multifd_load_setup(void);
int multifd_load_cleanup(Error **errp);
bool multifd_recv_all_channels_created(void);
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
` (5 preceding siblings ...)
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 6/7] migration: fix some error handling Fei Li
@ 2018-10-10 12:08 ` Fei Li
2018-10-11 13:45 ` Markus Armbruster
6 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-10 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, peterx, armbru, berrange, quintela, dgilbert, eblake
Make qemu_thread_create() return a Boolean to indicate if it succeeds
rather than failing with an error. And add an Error parameter to hold
the error message and let the callers handle it.
Signed-off-by: Fei Li <fli@suse.com>
---
cpus.c | 45 +++++++++++++++++++++++++++--------------
dump.c | 6 ++++--
hw/misc/edu.c | 6 ++++--
hw/ppc/spapr_hcall.c | 10 +++++++--
hw/rdma/rdma_backend.c | 4 +++-
hw/usb/ccid-card-emulated.c | 13 ++++++++----
include/qemu/thread.h | 4 ++--
io/task.c | 3 ++-
iothread.c | 16 ++++++++++-----
migration/migration.c | 49 +++++++++++++++++++++++++++++++--------------
migration/postcopy-ram.c | 14 +++++++++++--
migration/ram.c | 39 +++++++++++++++++++++++++++---------
migration/savevm.c | 11 +++++++---
tests/atomic_add-bench.c | 3 ++-
tests/iothread.c | 2 +-
tests/qht-bench.c | 3 ++-
tests/rcutorture.c | 3 ++-
tests/test-aio.c | 2 +-
tests/test-rcu-list.c | 3 ++-
ui/vnc-jobs.c | 8 ++++++--
util/compatfd.c | 9 +++++++--
util/main-loop.c | 2 +-
util/oslib-posix.c | 17 ++++++++++++----
util/qemu-thread-posix.c | 22 ++++++++++++--------
util/qemu-thread-win32.c | 16 +++++++++++----
util/rcu.c | 3 ++-
util/thread-pool.c | 4 +++-
27 files changed, 224 insertions(+), 93 deletions(-)
diff --git a/cpus.c b/cpus.c
index c4db70607e..6ccc0e4ae4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1948,15 +1948,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name,
+ qemu_tcg_cpu_thread_fn, cpu,
+ QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
} else {
/* share a single thread for all cpus with TCG */
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
- qemu_thread_create(cpu->thread, thread_name,
- qemu_tcg_rr_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name,
+ qemu_tcg_rr_cpu_thread_fn, cpu,
+ QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
single_tcg_halt_cond = cpu->halt_cond;
single_tcg_cpu_thread = cpu->thread;
@@ -1984,8 +1989,10 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#endif
@@ -2000,8 +2007,10 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp)) {
+ /* keep 'if' here in case there is further error handling logic */
+ }
}
static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
@@ -2018,8 +2027,10 @@ static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp)) {
+ /* keep 'if' here in case there is further error handling logic */
+ }
}
static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
@@ -2031,8 +2042,10 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#endif
@@ -2047,8 +2060,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
cpu->cpu_index);
- qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp)) {
+ /* keep 'if' here in case there is further error handling logic */
+ }
}
bool qemu_init_vcpu(CPUState *cpu, Error **errp)
diff --git a/dump.c b/dump.c
index 4ec94c5e25..1f003aff9a 100644
--- a/dump.c
+++ b/dump.c
@@ -2020,8 +2020,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
if (detach_p) {
/* detached dump */
s->detached = true;
- qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
- s, QEMU_THREAD_DETACHED);
+ if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+ s, QEMU_THREAD_DETACHED, errp)) {
+ /* keep 'if' here in case there is further error handling logic */
+ }
} else {
/* sync dump */
dump_process(s, errp);
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 0687ffd343..53bcd7122d 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -355,8 +355,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
qemu_mutex_init(&edu->thr_mutex);
qemu_cond_init(&edu->thr_cond);
- qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
- edu, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+ edu, QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
"edu-mmio", 1 * MiB);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ae913d070f..7c16ade04a 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
sPAPRPendingHPT *pending = spapr->pending_hpt;
uint64_t current_ram_size;
int rc;
+ Error *local_err = NULL;
if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
return H_AUTHORITY;
@@ -538,8 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
pending->shift = shift;
pending->ret = H_HARDWARE;
- qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
- hpt_prepare_thread, pending, QEMU_THREAD_DETACHED);
+ if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+ hpt_prepare_thread, pending,
+ QEMU_THREAD_DETACHED, &local_err)) {
+ error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+ g_free(pending);
+ return H_RESOURCE;
+ }
spapr->pending_hpt = pending;
diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index d7a4bbd91f..53a2bd0d85 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -164,8 +164,10 @@ static void start_comp_thread(RdmaBackendDev *backend_dev)
snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
ibv_get_device_name(backend_dev->ib_dev));
backend_dev->comp_thread.run = true;
+ /* FIXME: let the further caller handle the error instead of abort() here */
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..09682b5d30 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -538,10 +538,15 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
return;
}
- qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
- card, QEMU_THREAD_JOINABLE);
- qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
- card, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
+ card, QEMU_THREAD_JOINABLE, errp)) {
+ return;
+ }
+ if (!qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
+ handle_apdu_thread, card,
+ QEMU_THREAD_JOINABLE, errp)) {
+ /* keep 'if' here in case there is further error handling logic */
+ }
}
static void emulated_unrealize(CCIDCardState *base, Error **errp)
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index b2661b6672..c230eb5d5b 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
void qemu_event_wait(QemuEvent *ev);
void qemu_event_destroy(QemuEvent *ev);
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
void *(*start_routine)(void *),
- void *arg, int mode);
+ void *arg, int mode, Error **errp);
void *qemu_thread_join(QemuThread *thread);
void qemu_thread_get_self(QemuThread *thread);
bool qemu_thread_is_self(QemuThread *thread);
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..6d3a18ab80 100644
--- a/io/task.c
+++ b/io/task.c
@@ -149,7 +149,8 @@ void qio_task_run_in_thread(QIOTask *task,
"io-task-worker",
qio_task_thread_worker,
data,
- QEMU_THREAD_DETACHED);
+ QEMU_THREAD_DETACHED,
+ &error_abort);
}
diff --git a/iothread.c b/iothread.c
index aff1281257..5b2a1df36d 100644
--- a/iothread.c
+++ b/iothread.c
@@ -161,9 +161,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
&local_error);
if (local_error) {
error_propagate(errp, local_error);
- aio_context_unref(iothread->ctx);
- iothread->ctx = NULL;
- return;
+ goto fail;
}
qemu_mutex_init(&iothread->init_done_lock);
@@ -175,8 +173,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
*/
name = object_get_canonical_path_component(OBJECT(obj));
thread_name = g_strdup_printf("IO %s", name);
- qemu_thread_create(&iothread->thread, thread_name, iothread_run,
- iothread, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run,
+ iothread, QEMU_THREAD_JOINABLE, errp)) {
+ g_free(thread_name);
+ g_free(name);
+ goto fail;
+ }
g_free(thread_name);
g_free(name);
@@ -187,6 +189,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
&iothread->init_done_lock);
}
qemu_mutex_unlock(&iothread->init_done_lock);
+ return;
+fail:
+ aio_context_unref(iothread->ctx);
+ iothread->ctx = NULL;
}
typedef struct {
diff --git a/migration/migration.c b/migration/migration.c
index c344bab8f1..6d0c7a2ab2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -388,6 +388,7 @@ static void process_incoming_migration_co(void *opaque)
MigrationIncomingState *mis = migration_incoming_get_current();
PostcopyState ps;
int ret;
+ Error *local_err = NULL;
assert(mis->from_src_file);
mis->migration_incoming_co = qemu_coroutine_self();
@@ -420,8 +421,13 @@ static void process_incoming_migration_co(void *opaque)
/* we get COLO info, and know if we are in COLO mode */
if (!ret && migration_incoming_enable_colo()) {
- qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
- colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
+ colo_process_incoming_thread, mis,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err, "failed to create "
+ "colo_process_incoming_thread: ");
+ goto fail;
+ }
mis->have_colo_incoming_thread = true;
qemu_coroutine_yield();
@@ -430,20 +436,22 @@ static void process_incoming_migration_co(void *opaque)
}
if (ret < 0) {
- Error *local_err = NULL;
-
- migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
- MIGRATION_STATUS_FAILED);
error_report("load of migration failed: %s", strerror(-ret));
- qemu_fclose(mis->from_src_file);
- if (multifd_load_cleanup(&local_err) != 0) {
- error_report_err(local_err);
- }
- exit(EXIT_FAILURE);
+ goto fail;
}
mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
qemu_bh_schedule(mis->bh);
mis->migration_incoming_co = NULL;
+ return;
+fail:
+ local_err = NULL;
+ migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ qemu_fclose(mis->from_src_file);
+ if (multifd_load_cleanup(&local_err) != 0) {
+ error_report_err(local_err);
+ }
+ exit(EXIT_FAILURE);
}
static void migration_incoming_setup(QEMUFile *f)
@@ -2300,6 +2308,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) {
@@ -2313,8 +2322,13 @@ static int open_return_path_on_source(MigrationState *ms,
return 0;
}
- qemu_thread_create(&ms->rp_state.rp_thread, "return path",
- source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&ms->rp_state.rp_thread, "return path",
+ source_return_path_thread, ms,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err,
+ "failed to create source_return_path_thread: ");
+ return -1;
+ }
trace_open_return_path_on_source_continue();
@@ -3139,8 +3153,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
migrate_fd_cleanup(s);
return;
}
- qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&s->thread, "live_migration", migration_thread,
+ s, QEMU_THREAD_JOINABLE, &error_in)) {
+ error_reportf_err(error_in, "failed to create migration_thread: ");
+ migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+ migrate_fd_cleanup(s);
+ return;
+ }
s->migration_thread_running = true;
}
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index f96e0ae1f8..c6d2ca0df9 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1082,6 +1082,8 @@ retry:
int postcopy_ram_enable_notify(MigrationIncomingState *mis)
{
+ Error *local_err = NULL;
+
/* Open the fd for the kernel to give us userfaults */
mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
if (mis->userfault_fd == -1) {
@@ -1108,8 +1110,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
}
qemu_sem_init(&mis->fault_thread_sem, 0);
- qemu_thread_create(&mis->fault_thread, "postcopy/fault",
- postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&mis->fault_thread, "postcopy/fault",
+ postcopy_ram_fault_thread, mis,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err,
+ "failed to create postcopy_ram_fault_thread: ");
+ close(mis->userfault_event_fd);
+ close(mis->userfault_fd);
+ 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 25ec8f1b54..b67e0255e3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -473,6 +473,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;
@@ -502,9 +503,12 @@ static int compress_threads_save_setup(void)
comp_param[i].quit = false;
qemu_mutex_init(&comp_param[i].mutex);
qemu_cond_init(&comp_param[i].cond);
- qemu_thread_create(compress_threads + i, "compress",
- do_data_compress, comp_param + i,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(compress_threads + i, "compress",
+ do_data_compress, comp_param + i,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err, "failed to create do_data_compress: ");
+ goto exit;
+ }
}
return 0;
@@ -1082,8 +1086,14 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
p->c = QIO_CHANNEL(sioc);
qio_channel_set_delay(p->c, false);
p->running = true;
- qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err,
+ "failed to create multifd_send_thread: ");
+ multifd_save_cleanup();
+ migrate_set_error(migrate_get_current(), local_err);
+ return;
+ }
atomic_inc(&multifd_send_state->count);
}
@@ -1357,8 +1367,12 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
p->num_packets = 1;
p->running = true;
- qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&p->thread, p->name, multifd_recv_thread,
+ p, QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err, "failed to create multifd_recv_thread: ");
+ multifd_recv_terminate_threads(local_err, true);
+ return false;
+ }
atomic_inc(&multifd_recv_state->count);
return multifd_recv_state->count == migrate_multifd_channels();
}
@@ -3600,6 +3614,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;
@@ -3621,9 +3636,13 @@ static int compress_threads_load_setup(QEMUFile *f)
qemu_cond_init(&decomp_param[i].cond);
decomp_param[i].done = true;
decomp_param[i].quit = false;
- qemu_thread_create(decompress_threads + i, "decompress",
- do_data_decompress, decomp_param + i,
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(decompress_threads + i, "decompress",
+ do_data_decompress, decomp_param + i,
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err,
+ "failed to create do_data_decompress: ");
+ goto exit;
+ }
}
return 0;
exit:
diff --git a/migration/savevm.c b/migration/savevm.c
index 2d10e45582..7f5eefd5d4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1728,9 +1728,14 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
mis->have_listen_thread = true;
/* Start up the listening thread and wait for it to signal ready */
qemu_sem_init(&mis->listen_thread_sem, 0);
- qemu_thread_create(&mis->listen_thread, "postcopy/listen",
- postcopy_ram_listen_thread, NULL,
- QEMU_THREAD_DETACHED);
+ if (!qemu_thread_create(&mis->listen_thread, "postcopy/listen",
+ postcopy_ram_listen_thread, NULL,
+ QEMU_THREAD_DETACHED, &local_err)) {
+ error_reportf_err(local_err,
+ "failed to create postcopy_ram_listen_thread: ");
+ qemu_sem_destroy(&mis->listen_thread_sem);
+ return -1;
+ }
qemu_sem_wait(&mis->listen_thread_sem);
qemu_sem_destroy(&mis->listen_thread_sem);
diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c
index 2f6c72f63a..338b9563e3 100644
--- a/tests/atomic_add-bench.c
+++ b/tests/atomic_add-bench.c
@@ -2,6 +2,7 @@
#include "qemu/thread.h"
#include "qemu/host-utils.h"
#include "qemu/processor.h"
+#include "qapi/error.h"
struct thread_info {
uint64_t r;
@@ -110,7 +111,7 @@ static void create_threads(void)
info->r = (i + 1) ^ time(NULL);
qemu_thread_create(&threads[i], NULL, thread_func, info,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
}
}
diff --git a/tests/iothread.c b/tests/iothread.c
index 777d9eea46..f4ad992e61 100644
--- a/tests/iothread.c
+++ b/tests/iothread.c
@@ -73,7 +73,7 @@ IOThread *iothread_new(void)
qemu_mutex_init(&iothread->init_done_lock);
qemu_cond_init(&iothread->init_done_cond);
qemu_thread_create(&iothread->thread, NULL, iothread_run,
- iothread, QEMU_THREAD_JOINABLE);
+ iothread, QEMU_THREAD_JOINABLE, &error_abort);
/* Wait for initialization to complete */
qemu_mutex_lock(&iothread->init_done_lock);
diff --git a/tests/qht-bench.c b/tests/qht-bench.c
index 2089e2bed1..71df567ea2 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 {
@@ -247,7 +248,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 2e6f70bd59..0f7da81291 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -25,6 +25,7 @@
#include "qemu/rcu.h"
#include "qemu/thread.h"
#include "qemu/rcu_queue.h"
+#include "qapi/error.h"
/*
* Test variables.
@@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *))
exit(-1);
}
qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads],
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
n_threads++;
}
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 8807d7217c..35a652d1fd 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -31,6 +31,7 @@
#include "vnc-jobs.h"
#include "qemu/sockets.h"
#include "qemu/main-loop.h"
+#include "qapi/error.h"
#include "block/aio.h"
/*
@@ -340,8 +341,11 @@ bool vnc_start_worker_thread(Error **errp)
}
q = vnc_queue_init();
- qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
- QEMU_THREAD_DETACHED);
+ if (!qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread,
+ q, QEMU_THREAD_DETACHED, errp)) {
+ vnc_queue_clear(q);
+ return false;
+ }
queue = q; /* Set global queue */
out:
return true;
diff --git a/util/compatfd.c b/util/compatfd.c
index d3ed890405..1ab94d423b 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -91,8 +91,13 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
memcpy(&info->mask, mask, sizeof(*mask));
info->fd = fds[1];
- qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
- QEMU_THREAD_DETACHED);
+ if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat,
+ info, QEMU_THREAD_DETACHED, errp)) {
+ close(fds[0]);
+ close(fds[1]);
+ free(info);
+ return -1;
+ }
return fds[0];
}
diff --git a/util/main-loop.c b/util/main-loop.c
index 9671b6d226..b783f3c806 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -96,7 +96,7 @@ static int qemu_signal_init(Error **errp)
sigdelset(&set, SIG_IPI);
sigfd = qemu_signalfd(&set, errp);
if (sigfd == -1) {
- return -errno;
+ return -1;
}
fcntl_setfl(sigfd, O_NONBLOCK);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index fbd0dc8c57..c05ed9d020 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -437,9 +437,12 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
size_t size_per_thread;
char *addr = area;
int i = 0;
+ int started_thread = 0;
+ Error *local_err = NULL;
memset_thread_failed = false;
memset_num_threads = get_memset_num_threads(smp_cpus);
+ started_thread = memset_num_threads;
memset_thread = g_new0(MemsetThread, memset_num_threads);
numpages_per_thread = (numpages / memset_num_threads);
size_per_thread = (hpagesize * numpages_per_thread);
@@ -448,13 +451,19 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
memset_thread[i].numpages = (i == (memset_num_threads - 1)) ?
numpages : numpages_per_thread;
memset_thread[i].hpagesize = hpagesize;
- qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
- do_touch_pages, &memset_thread[i],
- QEMU_THREAD_JOINABLE);
+ if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
+ do_touch_pages, &memset_thread[i],
+ QEMU_THREAD_JOINABLE, &local_err)) {
+ error_reportf_err(local_err, "failed to create do_touch_pages: ");
+ memset_thread_failed = true;
+ started_thread = i;
+ goto out;
+ }
addr += size_per_thread;
numpages -= numpages_per_thread;
}
- for (i = 0; i < memset_num_threads; i++) {
+out:
+ for (i = 0; i < started_thread; i++) {
qemu_thread_join(&memset_thread[i].pgthread);
}
g_free(memset_thread);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 289af4fab5..3a2a0cb3c1 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -15,6 +15,7 @@
#include "qemu/atomic.h"
#include "qemu/notify.h"
#include "qemu-thread-common.h"
+#include "qapi/error.h"
static bool name_threads;
@@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)
return start_routine(arg);
}
-void qemu_thread_create(QemuThread *thread, const char *name,
- void *(*start_routine)(void*),
- void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+ void *(*start_routine)(void *),
+ void *arg, int mode, Error **errp)
{
sigset_t set, oldset;
int err;
@@ -515,7 +516,8 @@ void qemu_thread_create(QemuThread *thread, const char *name,
err = pthread_attr_init(&attr);
if (err) {
- error_exit(err, __func__);
+ error_setg(errp, "pthread_attr_init failed: %s", strerror(err));
+ return false;
}
if (mode == QEMU_THREAD_DETACHED) {
@@ -530,16 +532,20 @@ void qemu_thread_create(QemuThread *thread, const char *name,
qemu_thread_args->name = g_strdup(name);
qemu_thread_args->start_routine = start_routine;
qemu_thread_args->arg = arg;
-
err = pthread_create(&thread->thread, &attr,
qemu_thread_start, qemu_thread_args);
-
- if (err)
- error_exit(err, __func__);
+ if (err) {
+ error_setg(errp, "pthread_create failed: %s", strerror(err));
+ pthread_attr_destroy(&attr);
+ g_free(qemu_thread_args->name);
+ g_free(qemu_thread_args);
+ return false;
+ }
pthread_sigmask(SIG_SETMASK, &oldset, NULL);
pthread_attr_destroy(&attr);
+ return true;
}
void qemu_thread_get_self(QemuThread *thread)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 1a27e1cf6f..96e5d19ca3 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;
@@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
return ret;
}
-void qemu_thread_create(QemuThread *thread, const char *name,
- void *(*start_routine)(void *),
- void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+ void *(*start_routine)(void *),
+ void *arg, int mode, Error **errp)
{
HANDLE hThread;
struct QemuThreadData *data;
@@ -409,10 +410,17 @@ 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__);
+ if (data->mode != QEMU_THREAD_DETACHED) {
+ DeleteCriticalSection(&data->cs);
+ }
+ error_setg_win32(errp, GetLastError(),
+ "failed to create win32_start_routine");
+ g_free(data);
+ return false;
}
CloseHandle(hThread);
thread->data = data;
+ return true;
}
void qemu_thread_get_self(QemuThread *thread)
diff --git a/util/rcu.c b/util/rcu.c
index 5676c22bd1..145dcdb0c6 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -32,6 +32,7 @@
#include "qemu/atomic.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "qapi/error.h"
#if defined(CONFIG_MALLOC_TRIM)
#include <malloc.h>
#endif
@@ -325,7 +326,7 @@ static void rcu_init_complete(void)
* must have been quiescent even after forking, just recreate it.
*/
qemu_thread_create(&thread, "call_rcu", call_rcu_thread,
- NULL, QEMU_THREAD_DETACHED);
+ NULL, QEMU_THREAD_DETACHED, &error_abort);
rcu_register_thread();
}
diff --git a/util/thread-pool.c b/util/thread-pool.c
index 610646d131..ad0f980783 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -22,6 +22,7 @@
#include "trace.h"
#include "block/thread-pool.h"
#include "qemu/main-loop.h"
+#include "qapi/error.h"
static void do_spawn_thread(ThreadPool *pool);
@@ -132,7 +133,8 @@ static void do_spawn_thread(ThreadPool *pool)
pool->new_threads--;
pool->pending_threads++;
- qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
+ qemu_thread_create(&t, "worker", worker_thread, pool,
+ QEMU_THREAD_DETACHED, &error_abort);
}
static void spawn_thread_bh_fn(void *opaque)
--
2.13.7
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails Fei Li
@ 2018-10-11 10:02 ` Markus Armbruster
2018-10-12 4:24 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-11 10:02 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
Fei Li <fli@suse.com> writes:
> 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.
The bug is in qemu_init_main_loop():
ret = qemu_signal_init();
if (ret) {
return ret;
}
Fails without setting an error, unlike the other failures. Its callers
crash then.
> 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>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/osdep.h | 2 +-
> util/compatfd.c | 9 ++++++---
> util/main-loop.c | 9 ++++-----
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4f8559e550..f1f56763a0 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
> additional fields in the future) */
> };
>
> -int qemu_signalfd(const sigset_t *mask);
> +int qemu_signalfd(const sigset_t *mask, Error **errp);
> void sigaction_invoke(struct sigaction *action,
> struct qemu_signalfd_siginfo *info);
> #endif
> diff --git a/util/compatfd.c b/util/compatfd.c
> index 980bd33e52..d3ed890405 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -16,6 +16,7 @@
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> #include "qemu/thread.h"
> +#include "qapi/error.h"
>
> #include <sys/syscall.h>
>
> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
> }
> }
>
> -static int qemu_signalfd_compat(const sigset_t *mask)
> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
> {
> struct sigfd_compat_info *info;
> QemuThread thread;
> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>
> info = malloc(sizeof(*info));
> if (info == NULL) {
> + error_setg(errp, "Failed to allocate signalfd memory");
> errno = ENOMEM;
> return -1;
> }
>
> if (pipe(fds) == -1) {
> + error_setg(errp, "Failed to create signalfd pipe");
> free(info);
> return -1;
> }
> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> return fds[0];
> }
>
> -int qemu_signalfd(const sigset_t *mask)
> +int qemu_signalfd(const sigset_t *mask, Error **errp)
> {
> #if defined(CONFIG_SIGNALFD)
> int ret;
> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
> }
> #endif
>
> - return qemu_signalfd_compat(mask);
> + return qemu_signalfd_compat(mask, errp);
> }
I think this takes the Error conversion too far.
qemu_signalfd() is like the signalfd() system call, only portable, and
setting FD_CLOEXEC. In particular, it reports failure just like a
system call: it sets errno and returns -1. I'd prefer to keep it that
way. Instead...
> diff --git a/util/main-loop.c b/util/main-loop.c
> index affe0403c5..9671b6d226 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
> }
> }
>
> -static int qemu_signal_init(void)
> +static int qemu_signal_init(Error **errp)
> {
> int sigfd;
> sigset_t set;
> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
> pthread_sigmask(SIG_BLOCK, &set, NULL);
>
> sigdelset(&set, SIG_IPI);
> - sigfd = qemu_signalfd(&set);
> + sigfd = qemu_signalfd(&set, errp);
> if (sigfd == -1) {
> - fprintf(stderr, "failed to create signalfd\n");
> return -errno;
> }
>
... change this function so:
pthread_sigmask(SIG_BLOCK, &set, NULL);
sigdelset(&set, SIG_IPI);
sigfd = qemu_signalfd(&set);
if (sigfd == -1) {
- fprintf(stderr, "failed to create signalfd\n");
+ error_setg_errno(errp, errno, "failed to create signalfd");
return -errno;
}
Does this make sense?
> @@ -109,7 +108,7 @@ static int qemu_signal_init(void)
>
> #else /* _WIN32 */
>
> -static int qemu_signal_init(void)
> +static int qemu_signal_init(Error **errp)
> {
> return 0;
> }
> @@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
>
> init_clocks(qemu_timer_notify_cb);
>
> - ret = qemu_signal_init();
> + ret = qemu_signal_init(errp);
> if (ret) {
> return ret;
> }
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func Fei Li
@ 2018-10-11 13:13 ` Markus Armbruster
2018-10-12 5:40 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-11 13:13 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, quintela, dgilbert, peterx, armbru, famz
Fei Li <fli@suse.com> writes:
> Add a new Error parameter for vnc_display_init() to handle errors
> in its caller: vnc_init_func(), just like vnc_display_open() does.
> And let the call trace propagate the Error.
>
> Besides, make vnc_start_worker_thread() return a bool to indicate
> whether it succeeds instead of returning nothing.
>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> include/ui/console.h | 2 +-
> ui/vnc-jobs.c | 9 ++++++---
> ui/vnc-jobs.h | 2 +-
> ui/vnc.c | 12 +++++++++---
> 4 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>
> /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
> void vnc_display_open(const char *id, Error **errp);
> void vnc_display_add_client(const char *id, int csock, bool skipauth);
> int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index 929391f85d..8807d7217c 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
> return queue; /* Check global queue */
> }
>
> -void vnc_start_worker_thread(void)
> +bool vnc_start_worker_thread(Error **errp)
> {
> VncJobQueue *q;
>
> - if (vnc_worker_thread_running())
> - return ;
> + if (vnc_worker_thread_running()) {
> + goto out;
> + }
>
> q = vnc_queue_init();
> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> QEMU_THREAD_DETACHED);
> queue = q; /* Set global queue */
> +out:
> + return true;
> }
This function cannot fail. Why convert it to Error, and complicate its
caller?
> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
> index 59f66bcc35..14640593db 100644
> --- a/ui/vnc-jobs.h
> +++ b/ui/vnc-jobs.h
> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
> void vnc_jobs_join(VncState *vs);
>
> void vnc_jobs_consume_buffer(VncState *vs);
> -void vnc_start_worker_thread(void);
> +bool vnc_start_worker_thread(Error **errp);
>
> /* Locks */
> static inline int vnc_trylock_display(VncDisplay *vd)
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..f3806e76db 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
> .dpy_cursor_define = vnc_dpy_cursor_define,
> };
>
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
> {
> VncDisplay *vd;
>
if (vnc_display_find(id) != NULL) {
return;
}
vd = g_malloc0(sizeof(*vd));
vd->id = strdup(id);
QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
QTAILQ_INIT(&vd->clients);
vd->expires = TIME_MAX;
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
} else {
vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
}
if (!vd->kbd_layout) {
exit(1);
Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
here makes them fatal. Okay before this patch, but inappropriate
afterwards. I'm afraid you have to convert init_keyboard_layout() to
Error first.
}
vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
> vd->connections_limit = 32;
>
> qemu_mutex_init(&vd->mutex);
> - vnc_start_worker_thread();
> + if (!vnc_start_worker_thread(errp)) {
> + return;
> + }
>
> vd->dcl.ops = &dcl_ops;
> register_displaychangelistener(&vd->dcl);
> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> char *id = (char *)qemu_opts_id(opts);
>
> assert(id);
> - vnc_display_init(id);
> + vnc_display_init(id, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "Failed to init VNC server: ");
> + exit(1);
> + }
> vnc_display_open(id, &local_err);
> if (local_err != NULL) {
> error_reportf_err(local_err, "Failed to start VNC server: ");
Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
vnc_init_func()". Your patch shows that mine is incomplete.
Without my patch, there's no real reason for yours, however. The two
should be squashed together.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate Fei Li
@ 2018-10-11 13:19 ` Markus Armbruster
2018-10-12 5:55 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-11 13:19 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, quintela, dgilbert, peterx, armbru, famz
Fei Li <fli@suse.com> writes:
> The caller of qemu_init_vcpu() already passed the **errp to handle
Which caller? There are many. Or do you mean "The callers"?
> errors. In view of this, add a new Error parameter to the following
> call trace to propagate the error and let the further caller check it.
Which "call trace"?
> Besides, make qemu_init_vcpu() return a Boolean value to let its
> callers know whether it succeeds.
>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> accel/tcg/user-exec-stub.c | 2 +-
> cpus.c | 34 +++++++++++++++++++++-------------
> include/qom/cpu.h | 2 +-
> target/alpha/cpu.c | 4 +++-
> target/arm/cpu.c | 4 +++-
> target/cris/cpu.c | 4 +++-
> target/hppa/cpu.c | 4 +++-
> target/i386/cpu.c | 4 +++-
> target/lm32/cpu.c | 4 +++-
> target/m68k/cpu.c | 4 +++-
> target/microblaze/cpu.c | 4 +++-
> target/mips/cpu.c | 4 +++-
> target/moxie/cpu.c | 4 +++-
> target/nios2/cpu.c | 4 +++-
> target/openrisc/cpu.c | 4 +++-
> target/ppc/translate_init.inc.c | 4 +++-
> target/riscv/cpu.c | 4 +++-
> target/s390x/cpu.c | 4 +++-
> target/sh4/cpu.c | 4 +++-
> target/sparc/cpu.c | 4 +++-
> target/tilegx/cpu.c | 4 +++-
> target/tricore/cpu.c | 4 +++-
> target/unicore32/cpu.c | 4 +++-
> target/xtensa/cpu.c | 4 +++-
> 24 files changed, 86 insertions(+), 36 deletions(-)
>
> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
> index a32b4496af..38f6b928d4 100644
> --- a/accel/tcg/user-exec-stub.c
> +++ b/accel/tcg/user-exec-stub.c
> @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
> {
> }
>
> -void qemu_init_vcpu(CPUState *cpu)
> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
> {
You need to return a value here. Sure you compile-tested this?
> }
>
> diff --git a/cpus.c b/cpus.c
> index 361678e459..c4db70607e 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1918,7 +1918,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;
> @@ -1974,7 +1974,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];
>
> @@ -1991,7 +1991,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];
>
> @@ -2004,7 +2004,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];
>
> @@ -2022,7 +2022,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];
>
> @@ -2038,7 +2038,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];
>
> @@ -2051,11 +2051,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
> QEMU_THREAD_JOINABLE);
> }
>
> -void qemu_init_vcpu(CPUState *cpu)
> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
> {
> cpu->nr_cores = smp_cores;
> cpu->nr_threads = smp_threads;
> cpu->stopped = true;
> + Error *local_err = NULL;
>
> if (!cpu->as) {
> /* If the target cpu hasn't set up any address spaces itself,
> @@ -2066,22 +2067,29 @@ void qemu_init_vcpu(CPUState *cpu)
> }
>
> if (kvm_enabled()) {
> - qemu_kvm_start_vcpu(cpu);
> + qemu_kvm_start_vcpu(cpu, &local_err);
> } else if (hax_enabled()) {
> - qemu_hax_start_vcpu(cpu);
> + qemu_hax_start_vcpu(cpu, &local_err);
> } else if (hvf_enabled()) {
> - qemu_hvf_start_vcpu(cpu);
> + qemu_hvf_start_vcpu(cpu, &local_err);
> } else if (tcg_enabled()) {
> - qemu_tcg_init_vcpu(cpu);
> + qemu_tcg_init_vcpu(cpu, &local_err);
> } else if (whpx_enabled()) {
> - qemu_whpx_start_vcpu(cpu);
> + qemu_whpx_start_vcpu(cpu, &local_err);
> } else {
> - qemu_dummy_start_vcpu(cpu);
> + qemu_dummy_start_vcpu(cpu, &local_err);
> + }
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return false;
> }
>
> while (!cpu->created) {
> qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> }
> +
> + return true;
> }
>
> void cpu_stop_current(void)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index dc130cd307..4d85dda175 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -1012,7 +1012,7 @@ void end_exclusive(void);
> *
> * Initializes a vCPU.
> */
> -void qemu_init_vcpu(CPUState *cpu);
> +bool qemu_init_vcpu(CPUState *cpu, Error **errp);
>
> #define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
> #define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index b08078e7fc..d531bd4f7e 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> acc->parent_realize(dev, errp);
> }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b5e61cc177..40f300174d 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1038,7 +1038,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> }
> #endif
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> cpu_reset(cs);
>
> acc->parent_realize(dev, errp);
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index a23aba2688..ec92d69781 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
> }
>
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> ccc->parent_realize(dev, errp);
> }
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 00bf444620..08f600ced9 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> acc->parent_realize(dev, errp);
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c88876dfe3..d199f91258 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> }
> #endif
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> /*
> * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
> diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
> index b7499cb627..d50b1e4a43 100644
> --- a/target/lm32/cpu.c
> +++ b/target/lm32/cpu.c
> @@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>
> cpu_reset(cs);
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> lcc->parent_realize(dev, errp);
> }
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 582e3a73b3..4ab53f2d58 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
> m68k_cpu_init_gdb(cpu);
>
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> mcc->parent_realize(dev, errp);
> }
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 9b546a2c18..3906c864a3 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> env->pvr.regs[0] = PVR0_USE_EXC_MASK \
> | PVR0_USE_ICACHE_MASK \
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 497706b669..62be84af76 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
> cpu_mips_realize_env(&cpu->env);
>
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> mcc->parent_realize(dev, errp);
> }
> diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
> index 8d67eb6727..8581a6d922 100644
> --- a/target/moxie/cpu.c
> +++ b/target/moxie/cpu.c
> @@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> cpu_reset(cs);
>
> mcc->parent_realize(dev, errp);
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index fbfaa2ce26..5c7b4b486e 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> cpu_reset(cs);
>
> ncc->parent_realize(dev, errp);
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index fb7cb5c507..a6dcdb9df9 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> cpu_reset(cs);
>
> occ->parent_realize(dev, errp);
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 263e63cb03..a6dd2318a6 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
> 32, "power-vsx.xml", 0);
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + goto unrealize;
> + }
>
> pcc->parent_realize(dev, errp);
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d630e8fd6c..ef56215e92 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
> cpu_reset(cs);
>
> mcc->parent_realize(dev, errp);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 18ba7f85a5..2a3eac9761 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
> qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> #endif
> s390_cpu_gdb_init(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> /*
> * KVM requires the initial CPU reset ioctl to be executed on the target
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index b9f393b7c7..d32ef2e1cb 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
> }
>
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> scc->parent_realize(dev, errp);
> }
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index 0f090ece54..9c22f6a7df 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> scc->parent_realize(dev, errp);
> }
> diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
> index bfe9be59b5..234148fabd 100644
> --- a/target/tilegx/cpu.c
> +++ b/target/tilegx/cpu.c
> @@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
> }
>
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> tcc->parent_realize(dev, errp);
> }
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index 2edaef1aef..5482d6ea3f 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
> set_feature(env, TRICORE_FEATURE_13);
> }
> cpu_reset(cs);
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> tcc->parent_realize(dev, errp);
> }
> diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
> index 68f978d80b..1f6a33b6f3 100644
> --- a/target/unicore32/cpu.c
> +++ b/target/unicore32/cpu.c
> @@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> ucc->parent_realize(dev, errp);
> }
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index a54dbe4260..d2351c9b20 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>
> cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>
> - qemu_init_vcpu(cs);
> + if (!qemu_init_vcpu(cs, errp)) {
> + return;
> + }
>
> xcc->parent_realize(dev, errp);
> }
I see how you changed the code to pass an Error from the
qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't
see how such errors can be created. Without a way to create any, the
patch is pointless. What am I missing?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code Fei Li
@ 2018-10-11 13:28 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2018-10-11 13:28 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
Fei Li <fli@suse.com> writes:
> When multifd is used during migration, if there is an error before
> the destination receives all new channels, the destination does not
> exit but keeps waiting in our current code. However, a segmentaion
segmentation
> fault will occur in the source when multifd_save_cleanup() is called
> again as the multifd_send_state has been freed earlier in the first
> error handling. This can happen when migrate_fd_connect() fails and
> multifd_fd_cleanup() is called, and then multifd_new_send_channel_
> async() fails and multifd_save_cleanup() is called again.
>
> If the QIOChannel *c of multifd_recv_state->params[i] (p->c) is not
> initialized, there is no need to close the channel. Or else a
> segmentation fault will occur in multifd_recv_terminate_threads()
> when multifd_recv_initial_packet() fails.
>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
> migration/ram.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index bc38d98cc3..41dc94c059 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -907,6 +907,11 @@ static void multifd_send_terminate_threads(Error *err)
> }
> }
>
> + /* in case multifd_send_state has been freed earlier */
> + if (!multifd_send_state) {
> + return;
> + }
> +
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDSendParams *p = &multifd_send_state->params[i];
>
> @@ -922,7 +927,7 @@ int multifd_save_cleanup(Error **errp)
> int i;
> int ret = 0;
>
> - if (!migrate_use_multifd()) {
> + if (!migrate_use_multifd() || !multifd_send_state) {
> return 0;
> }
> multifd_send_terminate_threads(NULL);
> @@ -1131,7 +1136,7 @@ struct {
> uint64_t packet_num;
> } *multifd_recv_state;
>
> -static void multifd_recv_terminate_threads(Error *err)
> +static void multifd_recv_terminate_threads(Error *err, bool channel)
> {
> int i;
>
> @@ -1145,6 +1150,11 @@ static void multifd_recv_terminate_threads(Error *err)
> }
> }
>
> + /* in case p->c is not initialized */
> + if (!channel) {
> + return;
> + }
> +
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = &multifd_recv_state->params[i];
>
The function does one thing always, another thing only when its
argument @channel is true, and ...
> @@ -1166,7 +1176,7 @@ int multifd_load_cleanup(Error **errp)
> if (!migrate_use_multifd()) {
> return 0;
> }
> - multifd_recv_terminate_threads(NULL);
> + multifd_recv_terminate_threads(NULL, true);
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = &multifd_recv_state->params[i];
>
> @@ -1269,7 +1279,7 @@ static void *multifd_recv_thread(void *opaque)
> }
>
> if (local_err) {
> - multifd_recv_terminate_threads(local_err);
> + multifd_recv_terminate_threads(local_err, true);
> }
> qemu_mutex_lock(&p->mutex);
> p->running = false;
> @@ -1331,7 +1341,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
>
> id = multifd_recv_initial_packet(ioc, &local_err);
> if (id < 0) {
> - multifd_recv_terminate_threads(local_err);
> + multifd_recv_terminate_threads(local_err, false);
> return false;
> }
>
> @@ -1339,7 +1349,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
> if (p->c != NULL) {
> error_setg(&local_err, "multifd: received id '%d' already setup'",
> id);
> - multifd_recv_terminate_threads(local_err);
> + multifd_recv_terminate_threads(local_err, true);
> return false;
> }
> p->c = ioc;
... all callers pass literal true or false. Should we split the
function into its two parts, and call the second half only in the places
where its needed? Matter of taste, I guess. The maintainers may have
an opinion.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle Fei Li
@ 2018-10-11 13:45 ` Markus Armbruster
2018-10-12 6:00 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-11 13:45 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, quintela, dgilbert, peterx, armbru, famz
Fei Li <fli@suse.com> writes:
> Make qemu_thread_create() return a Boolean to indicate if it succeeds
> rather than failing with an error. And add an Error parameter to hold
> the error message and let the callers handle it.
>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
[...]
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 289af4fab5..3a2a0cb3c1 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -15,6 +15,7 @@
> #include "qemu/atomic.h"
> #include "qemu/notify.h"
> #include "qemu-thread-common.h"
> +#include "qapi/error.h"
>
> static bool name_threads;
>
> @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)
> return start_routine(arg);
> }
>
> -void qemu_thread_create(QemuThread *thread, const char *name,
> - void *(*start_routine)(void*),
> - void *arg, int mode)
> +bool qemu_thread_create(QemuThread *thread, const char *name,
> + void *(*start_routine)(void *),
> + void *arg, int mode, Error **errp)
> {
> sigset_t set, oldset;
> int err;
> @@ -515,7 +516,8 @@ void qemu_thread_create(QemuThread *thread, const char *name,
>
> err = pthread_attr_init(&attr);
> if (err) {
> - error_exit(err, __func__);
> + error_setg(errp, "pthread_attr_init failed: %s", strerror(err));
> + return false;
The commit message claims this function was "failing with an error".
Not true; error_exit() abort()s. It exit()ed until commit 53380ac37f2,
v1.0. The name error_exit() became misleading then. The bad name is
not this patch's problem, but its commit message needs to be corrected.
> }
>
> if (mode == QEMU_THREAD_DETACHED) {
[...]
I think David Gilbert added the bite-sized task you took on. David,
please review.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-11 10:02 ` Markus Armbruster
@ 2018-10-12 4:24 ` Fei Li
2018-10-12 7:56 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-12 4:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
On 10/11/2018 06:02 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> 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.
> The bug is in qemu_init_main_loop():
>
> ret = qemu_signal_init();
> if (ret) {
> return ret;
> }
>
> Fails without setting an error, unlike the other failures. Its callers
> crash then.
Thanks!
>> 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>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> include/qemu/osdep.h | 2 +-
>> util/compatfd.c | 9 ++++++---
>> util/main-loop.c | 9 ++++-----
>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 4f8559e550..f1f56763a0 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>> additional fields in the future) */
>> };
>>
>> -int qemu_signalfd(const sigset_t *mask);
>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>> void sigaction_invoke(struct sigaction *action,
>> struct qemu_signalfd_siginfo *info);
>> #endif
>> diff --git a/util/compatfd.c b/util/compatfd.c
>> index 980bd33e52..d3ed890405 100644
>> --- a/util/compatfd.c
>> +++ b/util/compatfd.c
>> @@ -16,6 +16,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu-common.h"
>> #include "qemu/thread.h"
>> +#include "qapi/error.h"
>>
>> #include <sys/syscall.h>
>>
>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>> }
>> }
>>
>> -static int qemu_signalfd_compat(const sigset_t *mask)
>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>> {
>> struct sigfd_compat_info *info;
>> QemuThread thread;
>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>
>> info = malloc(sizeof(*info));
>> if (info == NULL) {
>> + error_setg(errp, "Failed to allocate signalfd memory");
>> errno = ENOMEM;
>> return -1;
>> }
>>
>> if (pipe(fds) == -1) {
>> + error_setg(errp, "Failed to create signalfd pipe");
>> free(info);
>> return -1;
>> }
>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>> return fds[0];
>> }
>>
>> -int qemu_signalfd(const sigset_t *mask)
>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>> {
>> #if defined(CONFIG_SIGNALFD)
>> int ret;
>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>> }
>> #endif
>>
>> - return qemu_signalfd_compat(mask);
>> + return qemu_signalfd_compat(mask, errp);
>> }
> I think this takes the Error conversion too far.
>
> qemu_signalfd() is like the signalfd() system call, only portable, and
> setting FD_CLOEXEC. In particular, it reports failure just like a
> system call: it sets errno and returns -1. I'd prefer to keep it that
> way. Instead...
>
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index affe0403c5..9671b6d226 100644
>> --- a/util/main-loop.c
>> +++ b/util/main-loop.c
>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>> }
>> }
>>
>> -static int qemu_signal_init(void)
>> +static int qemu_signal_init(Error **errp)
>> {
>> int sigfd;
>> sigset_t set;
>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>
>> sigdelset(&set, SIG_IPI);
>> - sigfd = qemu_signalfd(&set);
>> + sigfd = qemu_signalfd(&set, errp);
>> if (sigfd == -1) {
>> - fprintf(stderr, "failed to create signalfd\n");
>> return -errno;
>> }
>>
> ... change this function so:
>
> pthread_sigmask(SIG_BLOCK, &set, NULL);
>
> sigdelset(&set, SIG_IPI);
> sigfd = qemu_signalfd(&set);
> if (sigfd == -1) {
> - fprintf(stderr, "failed to create signalfd\n");
> + error_setg_errno(errp, errno, "failed to create signalfd");
> return -errno;
> }
>
> Does this make sense?
Yes, it looks more concise if we only have this patch, but triggers one
errno-set
issue after applying patch 7/7, which adds a Error **errp parameter for
qemu_thread_create() to let its callers handle the error instead of
exit() directly
to add the robustness.
For the patch series' current implementation, the modified
qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds and
set the
error reason into the passed errp, and did not set the errno. Actually
another
similar errno-set issue has been talked in last patch. :)
If we set the errno in future qemu_thread_create(), we need to
distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno() to
set errno.
But for Windows, I am not sure if we can use
"errno = GetLastError()"
to set errno, as this seems a little weird. Do you have any idea about this?
BTW, if we have a decent errno-set way for Windows, I will adopt your above
proposal for this patch.
Have a nice day, thanks for the review :)
Fei
>> @@ -109,7 +108,7 @@ static int qemu_signal_init(void)
>>
>> #else /* _WIN32 */
>>
>> -static int qemu_signal_init(void)
>> +static int qemu_signal_init(Error **errp)
>> {
>> return 0;
>> }
>> @@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
>>
>> init_clocks(qemu_timer_notify_cb);
>>
>> - ret = qemu_signal_init();
>> + ret = qemu_signal_init(errp);
>> if (ret) {
>> return ret;
>> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-11 13:13 ` Markus Armbruster
@ 2018-10-12 5:40 ` Fei Li
2018-10-12 8:18 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-12 5:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
On 10/11/2018 09:13 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> Add a new Error parameter for vnc_display_init() to handle errors
>> in its caller: vnc_init_func(), just like vnc_display_open() does.
>> And let the call trace propagate the Error.
>>
>> Besides, make vnc_start_worker_thread() return a bool to indicate
>> whether it succeeds instead of returning nothing.
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> include/ui/console.h | 2 +-
>> ui/vnc-jobs.c | 9 ++++++---
>> ui/vnc-jobs.h | 2 +-
>> ui/vnc.c | 12 +++++++++---
>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index fb969caf70..c17803c530 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>>
>> /* vnc.c */
>> -void vnc_display_init(const char *id);
>> +void vnc_display_init(const char *id, Error **errp);
>> void vnc_display_open(const char *id, Error **errp);
>> void vnc_display_add_client(const char *id, int csock, bool skipauth);
>> int vnc_display_password(const char *id, const char *password);
>> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
>> index 929391f85d..8807d7217c 100644
>> --- a/ui/vnc-jobs.c
>> +++ b/ui/vnc-jobs.c
>> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
>> return queue; /* Check global queue */
>> }
>>
>> -void vnc_start_worker_thread(void)
>> +bool vnc_start_worker_thread(Error **errp)
>> {
>> VncJobQueue *q;
>>
>> - if (vnc_worker_thread_running())
>> - return ;
>> + if (vnc_worker_thread_running()) {
>> + goto out;
>> + }
>>
>> q = vnc_queue_init();
>> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
>> QEMU_THREAD_DETACHED);
>> queue = q; /* Set global queue */
>> +out:
>> + return true;
>> }
> This function cannot fail. Why convert it to Error, and complicate its
> caller?
[Issue1]
Actually, this is to pave the way for patch 7/7, in case
qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
connects the broken errp for callers who initially have the errp.
[I am back... If the other codes in this patches be squashed, maybe
merge [Issue1]
into patch 7/7 looks more intuitive.]
>> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
>> index 59f66bcc35..14640593db 100644
>> --- a/ui/vnc-jobs.h
>> +++ b/ui/vnc-jobs.h
>> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
>> void vnc_jobs_join(VncState *vs);
>>
>> void vnc_jobs_consume_buffer(VncState *vs);
>> -void vnc_start_worker_thread(void);
>> +bool vnc_start_worker_thread(Error **errp);
>>
>> /* Locks */
>> static inline int vnc_trylock_display(VncDisplay *vd)
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index cf221c83cc..f3806e76db 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>> .dpy_cursor_define = vnc_dpy_cursor_define,
>> };
>>
>> -void vnc_display_init(const char *id)
>> +void vnc_display_init(const char *id, Error **errp)
>> {
>> VncDisplay *vd;
>>
> if (vnc_display_find(id) != NULL) {
> return;
> }
> vd = g_malloc0(sizeof(*vd));
>
> vd->id = strdup(id);
> QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
>
> QTAILQ_INIT(&vd->clients);
> vd->expires = TIME_MAX;
>
> if (keyboard_layout) {
> trace_vnc_key_map_init(keyboard_layout);
> vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> } else {
> vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> }
>
> if (!vd->kbd_layout) {
> exit(1);
>
> Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
> here makes them fatal. Okay before this patch, but inappropriate
> afterwards. I'm afraid you have to convert init_keyboard_layout() to
> Error first.
[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static
value and also
will be used by others, how about passing the errp parameter to
init_keyboard_layout()
as follows? And for its other callers, just pass NULL.
@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
- vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout);
+ vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout, errp);
} else {
- vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+ vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
}
if (!vd->kbd_layout) {
- exit(1);
+ return;
}
>
> }
>
> vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
>> vd->connections_limit = 32;
>>
>> qemu_mutex_init(&vd->mutex);
>> - vnc_start_worker_thread();
>> + if (!vnc_start_worker_thread(errp)) {
>> + return;
>> + }
>>
>> vd->dcl.ops = &dcl_ops;
>> register_displaychangelistener(&vd->dcl);
>> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>> char *id = (char *)qemu_opts_id(opts);
>>
>> assert(id);
>> - vnc_display_init(id);
>> + vnc_display_init(id, &local_err);
>> + if (local_err) {
>> + error_reportf_err(local_err, "Failed to init VNC server: ");
>> + exit(1);
>> + }
>> vnc_display_open(id, &local_err);
>> if (local_err != NULL) {
>> error_reportf_err(local_err, "Failed to start VNC server: ");
> Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
> vnc_init_func()". Your patch shows that mine is incomplete.
>
> Without my patch, there's no real reason for yours, however. The two
> should be squashed together.
Ah, I noticed your patch 24/31. OK, let's squash.
Should I write a new patch by
- updating to error_propagate(...) if vnc_display_init() fails
&&
- modifying [Issue2] ?
Or you do this in your original patch?
BTW, for your patch 24/31, the updated passed errp for vnc_init_func is
&error_fatal,
then the system will exit(1) when running error_propagate(...) which calls
error_handle_fatal(...). This means the following two lines will not be
touched.
But if we want the following prepended error message, could we move it
earlier than
the error_propagare? I mean:
if (local_err != NULL) {
- error_reportf_err(local_err, "Failed to start VNC server: ");
- exit(1);
+ error_prepend(&local_err, "Failed to start VNC server: ");
+ error_propagate(errp, local_err);
+ return -1;
}
Have a nice day
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate
2018-10-11 13:19 ` Markus Armbruster
@ 2018-10-12 5:55 ` Fei Li
2018-10-12 8:24 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-12 5:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
On 10/11/2018 09:19 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> The caller of qemu_init_vcpu() already passed the **errp to handle
> Which caller? There are many. Or do you mean "The callers"?
Oh, sorry, I mean "The callers" :)
>
>> errors. In view of this, add a new Error parameter to the following
>> call trace to propagate the error and let the further caller check it.
> Which "call trace"?
Actually, I want to say all functions called by qemu_init_vcpu()..
>
>> Besides, make qemu_init_vcpu() return a Boolean value to let its
>> callers know whether it succeeds.
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> accel/tcg/user-exec-stub.c | 2 +-
>> cpus.c | 34 +++++++++++++++++++++-------------
>> include/qom/cpu.h | 2 +-
>> target/alpha/cpu.c | 4 +++-
>> target/arm/cpu.c | 4 +++-
>> target/cris/cpu.c | 4 +++-
>> target/hppa/cpu.c | 4 +++-
>> target/i386/cpu.c | 4 +++-
>> target/lm32/cpu.c | 4 +++-
>> target/m68k/cpu.c | 4 +++-
>> target/microblaze/cpu.c | 4 +++-
>> target/mips/cpu.c | 4 +++-
>> target/moxie/cpu.c | 4 +++-
>> target/nios2/cpu.c | 4 +++-
>> target/openrisc/cpu.c | 4 +++-
>> target/ppc/translate_init.inc.c | 4 +++-
>> target/riscv/cpu.c | 4 +++-
>> target/s390x/cpu.c | 4 +++-
>> target/sh4/cpu.c | 4 +++-
>> target/sparc/cpu.c | 4 +++-
>> target/tilegx/cpu.c | 4 +++-
>> target/tricore/cpu.c | 4 +++-
>> target/unicore32/cpu.c | 4 +++-
>> target/xtensa/cpu.c | 4 +++-
>> 24 files changed, 86 insertions(+), 36 deletions(-)
>>
>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>> index a32b4496af..38f6b928d4 100644
>> --- a/accel/tcg/user-exec-stub.c
>> +++ b/accel/tcg/user-exec-stub.c
>> @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
>> {
>> }
>>
>> -void qemu_init_vcpu(CPUState *cpu)
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>> {
> You need to return a value here. Sure you compile-tested this?
Oops! I forget the TCG case.. The `return true` should be added.
Thanks for pointing this out!
>
>> }
>>
>> diff --git a/cpus.c b/cpus.c
>> index 361678e459..c4db70607e 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1918,7 +1918,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;
>> @@ -1974,7 +1974,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];
>>
>> @@ -1991,7 +1991,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];
>>
>> @@ -2004,7 +2004,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];
>>
>> @@ -2022,7 +2022,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];
>>
>> @@ -2038,7 +2038,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];
>>
>> @@ -2051,11 +2051,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>> QEMU_THREAD_JOINABLE);
>> }
>>
>> -void qemu_init_vcpu(CPUState *cpu)
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>> {
>> cpu->nr_cores = smp_cores;
>> cpu->nr_threads = smp_threads;
>> cpu->stopped = true;
>> + Error *local_err = NULL;
>>
>> if (!cpu->as) {
>> /* If the target cpu hasn't set up any address spaces itself,
>> @@ -2066,22 +2067,29 @@ void qemu_init_vcpu(CPUState *cpu)
>> }
>>
>> if (kvm_enabled()) {
>> - qemu_kvm_start_vcpu(cpu);
>> + qemu_kvm_start_vcpu(cpu, &local_err);
>> } else if (hax_enabled()) {
>> - qemu_hax_start_vcpu(cpu);
>> + qemu_hax_start_vcpu(cpu, &local_err);
>> } else if (hvf_enabled()) {
>> - qemu_hvf_start_vcpu(cpu);
>> + qemu_hvf_start_vcpu(cpu, &local_err);
>> } else if (tcg_enabled()) {
>> - qemu_tcg_init_vcpu(cpu);
>> + qemu_tcg_init_vcpu(cpu, &local_err);
>> } else if (whpx_enabled()) {
>> - qemu_whpx_start_vcpu(cpu);
>> + qemu_whpx_start_vcpu(cpu, &local_err);
>> } else {
>> - qemu_dummy_start_vcpu(cpu);
>> + qemu_dummy_start_vcpu(cpu, &local_err);
>> + }
>> +
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return false;
>> }
>>
>> while (!cpu->created) {
>> qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
>> }
>> +
>> + return true;
>> }
>>
>> void cpu_stop_current(void)
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index dc130cd307..4d85dda175 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -1012,7 +1012,7 @@ void end_exclusive(void);
>> *
>> * Initializes a vCPU.
>> */
>> -void qemu_init_vcpu(CPUState *cpu);
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp);
>>
>> #define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
>> #define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
>> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
>> index b08078e7fc..d531bd4f7e 100644
>> --- a/target/alpha/cpu.c
>> +++ b/target/alpha/cpu.c
>> @@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> acc->parent_realize(dev, errp);
>> }
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index b5e61cc177..40f300174d 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1038,7 +1038,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>> }
>> #endif
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> cpu_reset(cs);
>>
>> acc->parent_realize(dev, errp);
>> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
>> index a23aba2688..ec92d69781 100644
>> --- a/target/cris/cpu.c
>> +++ b/target/cris/cpu.c
>> @@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>> }
>>
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> ccc->parent_realize(dev, errp);
>> }
>> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
>> index 00bf444620..08f600ced9 100644
>> --- a/target/hppa/cpu.c
>> +++ b/target/hppa/cpu.c
>> @@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> acc->parent_realize(dev, errp);
>>
>> #ifndef CONFIG_USER_ONLY
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index c88876dfe3..d199f91258 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>> }
>> #endif
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> /*
>> * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
>> diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
>> index b7499cb627..d50b1e4a43 100644
>> --- a/target/lm32/cpu.c
>> +++ b/target/lm32/cpu.c
>> @@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>>
>> cpu_reset(cs);
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> lcc->parent_realize(dev, errp);
>> }
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index 582e3a73b3..4ab53f2d58 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>> m68k_cpu_init_gdb(cpu);
>>
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> mcc->parent_realize(dev, errp);
>> }
>> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
>> index 9b546a2c18..3906c864a3 100644
>> --- a/target/microblaze/cpu.c
>> +++ b/target/microblaze/cpu.c
>> @@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> env->pvr.regs[0] = PVR0_USE_EXC_MASK \
>> | PVR0_USE_ICACHE_MASK \
>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>> index 497706b669..62be84af76 100644
>> --- a/target/mips/cpu.c
>> +++ b/target/mips/cpu.c
>> @@ -136,7 +136,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>> cpu_mips_realize_env(&cpu->env);
>>
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> mcc->parent_realize(dev, errp);
>> }
>> diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
>> index 8d67eb6727..8581a6d922 100644
>> --- a/target/moxie/cpu.c
>> +++ b/target/moxie/cpu.c
>> @@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> cpu_reset(cs);
>>
>> mcc->parent_realize(dev, errp);
>> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
>> index fbfaa2ce26..5c7b4b486e 100644
>> --- a/target/nios2/cpu.c
>> +++ b/target/nios2/cpu.c
>> @@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> cpu_reset(cs);
>>
>> ncc->parent_realize(dev, errp);
>> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
>> index fb7cb5c507..a6dcdb9df9 100644
>> --- a/target/openrisc/cpu.c
>> +++ b/target/openrisc/cpu.c
>> @@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> cpu_reset(cs);
>>
>> occ->parent_realize(dev, errp);
>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>> index 263e63cb03..a6dd2318a6 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
>> 32, "power-vsx.xml", 0);
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + goto unrealize;
>> + }
>>
>> pcc->parent_realize(dev, errp);
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index d630e8fd6c..ef56215e92 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -303,7 +303,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>> cpu_reset(cs);
>>
>> mcc->parent_realize(dev, errp);
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 18ba7f85a5..2a3eac9761 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>> qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>> #endif
>> s390_cpu_gdb_init(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> /*
>> * KVM requires the initial CPU reset ioctl to be executed on the target
>> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
>> index b9f393b7c7..d32ef2e1cb 100644
>> --- a/target/sh4/cpu.c
>> +++ b/target/sh4/cpu.c
>> @@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>> }
>>
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> scc->parent_realize(dev, errp);
>> }
>> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
>> index 0f090ece54..9c22f6a7df 100644
>> --- a/target/sparc/cpu.c
>> +++ b/target/sparc/cpu.c
>> @@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> scc->parent_realize(dev, errp);
>> }
>> diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
>> index bfe9be59b5..234148fabd 100644
>> --- a/target/tilegx/cpu.c
>> +++ b/target/tilegx/cpu.c
>> @@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>> }
>>
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> tcc->parent_realize(dev, errp);
>> }
>> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
>> index 2edaef1aef..5482d6ea3f 100644
>> --- a/target/tricore/cpu.c
>> +++ b/target/tricore/cpu.c
>> @@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>> set_feature(env, TRICORE_FEATURE_13);
>> }
>> cpu_reset(cs);
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> tcc->parent_realize(dev, errp);
>> }
>> diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
>> index 68f978d80b..1f6a33b6f3 100644
>> --- a/target/unicore32/cpu.c
>> +++ b/target/unicore32/cpu.c
>> @@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> ucc->parent_realize(dev, errp);
>> }
>> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
>> index a54dbe4260..d2351c9b20 100644
>> --- a/target/xtensa/cpu.c
>> +++ b/target/xtensa/cpu.c
>> @@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>>
>> cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>>
>> - qemu_init_vcpu(cs);
>> + if (!qemu_init_vcpu(cs, errp)) {
>> + return;
>> + }
>>
>> xcc->parent_realize(dev, errp);
>> }
> I see how you changed the code to pass an Error from the
> qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't
> see how such errors can be created. Without a way to create any, the
> patch is pointless. What am I missing?
This patch is also the pre-patch for the updated qemu_thread_create() in
patch 7/7
just as I explained in patch 2/7 [Issue1].
Have a nice day, thanks
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle
2018-10-11 13:45 ` Markus Armbruster
@ 2018-10-12 6:00 ` Fei Li
0 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-12 6:00 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, quintela, dgilbert, peterx, famz
On 10/11/2018 09:45 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> Make qemu_thread_create() return a Boolean to indicate if it succeeds
>> rather than failing with an error. And add an Error parameter to hold
>> the error message and let the callers handle it.
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
> [...]
>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>> index 289af4fab5..3a2a0cb3c1 100644
>> --- a/util/qemu-thread-posix.c
>> +++ b/util/qemu-thread-posix.c
>> @@ -15,6 +15,7 @@
>> #include "qemu/atomic.h"
>> #include "qemu/notify.h"
>> #include "qemu-thread-common.h"
>> +#include "qapi/error.h"
>>
>> static bool name_threads;
>>
>> @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)
>> return start_routine(arg);
>> }
>>
>> -void qemu_thread_create(QemuThread *thread, const char *name,
>> - void *(*start_routine)(void*),
>> - void *arg, int mode)
>> +bool qemu_thread_create(QemuThread *thread, const char *name,
>> + void *(*start_routine)(void *),
>> + void *arg, int mode, Error **errp)
>> {
>> sigset_t set, oldset;
>> int err;
>> @@ -515,7 +516,8 @@ void qemu_thread_create(QemuThread *thread, const char *name,
>>
>> err = pthread_attr_init(&attr);
>> if (err) {
>> - error_exit(err, __func__);
>> + error_setg(errp, "pthread_attr_init failed: %s", strerror(err));
>> + return false;
> The commit message claims this function was "failing with an error".
> Not true; error_exit() abort()s. It exit()ed until commit 53380ac37f2,
> v1.0. The name error_exit() became misleading then. The bad name is
> not this patch's problem, but its commit message needs to be corrected.
Ok, thanks. I will amend the message to "instead of abort() directly"
>
>> }
>>
>> if (mode == QEMU_THREAD_DETACHED) {
> [...]
>
> I think David Gilbert added the bite-sized task you took on. David,
> please review.
>
Thanks!
Have a nice day
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-12 4:24 ` Fei Li
@ 2018-10-12 7:56 ` Markus Armbruster
2018-10-12 9:42 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-12 7:56 UTC (permalink / raw)
To: Fei Li; +Cc: Markus Armbruster, peterx, famz, qemu-devel, dgilbert, quintela
Fei Li <fli@suse.com> writes:
> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> 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.
>> The bug is in qemu_init_main_loop():
>>
>> ret = qemu_signal_init();
>> if (ret) {
>> return ret;
>> }
>>
>> Fails without setting an error, unlike the other failures. Its callers
>> crash then.
> Thanks!
Consider working that into your commit message.
>>> 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>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> include/qemu/osdep.h | 2 +-
>>> util/compatfd.c | 9 ++++++---
>>> util/main-loop.c | 9 ++++-----
>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>> index 4f8559e550..f1f56763a0 100644
>>> --- a/include/qemu/osdep.h
>>> +++ b/include/qemu/osdep.h
>>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>>> additional fields in the future) */
>>> };
>>> -int qemu_signalfd(const sigset_t *mask);
>>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>>> void sigaction_invoke(struct sigaction *action,
>>> struct qemu_signalfd_siginfo *info);
>>> #endif
>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>> index 980bd33e52..d3ed890405 100644
>>> --- a/util/compatfd.c
>>> +++ b/util/compatfd.c
>>> @@ -16,6 +16,7 @@
>>> #include "qemu/osdep.h"
>>> #include "qemu-common.h"
>>> #include "qemu/thread.h"
>>> +#include "qapi/error.h"
>>> #include <sys/syscall.h>
>>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>>> }
>>> }
>>> -static int qemu_signalfd_compat(const sigset_t *mask)
>>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>>> {
>>> struct sigfd_compat_info *info;
>>> QemuThread thread;
>>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>> info = malloc(sizeof(*info));
>>> if (info == NULL) {
>>> + error_setg(errp, "Failed to allocate signalfd memory");
>>> errno = ENOMEM;
>>> return -1;
>>> }
>>> if (pipe(fds) == -1) {
>>> + error_setg(errp, "Failed to create signalfd pipe");
>>> free(info);
>>> return -1;
>>> }
>>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>> return fds[0];
>>> }
>>> -int qemu_signalfd(const sigset_t *mask)
>>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>>> {
>>> #if defined(CONFIG_SIGNALFD)
>>> int ret;
>>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>>> }
>>> #endif
>>> - return qemu_signalfd_compat(mask);
>>> + return qemu_signalfd_compat(mask, errp);
>>> }
>> I think this takes the Error conversion too far.
>>
>> qemu_signalfd() is like the signalfd() system call, only portable, and
>> setting FD_CLOEXEC. In particular, it reports failure just like a
>> system call: it sets errno and returns -1. I'd prefer to keep it that
>> way. Instead...
>>
>>> diff --git a/util/main-loop.c b/util/main-loop.c
>>> index affe0403c5..9671b6d226 100644
>>> --- a/util/main-loop.c
>>> +++ b/util/main-loop.c
>>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>>> }
>>> }
>>> -static int qemu_signal_init(void)
>>> +static int qemu_signal_init(Error **errp)
>>> {
>>> int sigfd;
>>> sigset_t set;
>>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>> sigdelset(&set, SIG_IPI);
>>> - sigfd = qemu_signalfd(&set);
>>> + sigfd = qemu_signalfd(&set, errp);
>>> if (sigfd == -1) {
>>> - fprintf(stderr, "failed to create signalfd\n");
>>> return -errno;
>>> }
>>>
>> ... change this function so:
>>
>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>> sigdelset(&set, SIG_IPI);
>> sigfd = qemu_signalfd(&set);
>> if (sigfd == -1) {
>> - fprintf(stderr, "failed to create signalfd\n");
>> + error_setg_errno(errp, errno, "failed to create signalfd");
>> return -errno;
>> }
>>
>> Does this make sense?
> Yes, it looks more concise if we only have this patch, but triggers
> one errno-set
> issue after applying patch 7/7, which adds a Error **errp parameter for
> qemu_thread_create() to let its callers handle the error instead of
> exit() directly
> to add the robustness.
I guess you're talking about the qemu_thread_create() in
qemu_signalfd_compat(). Correct?
> For the patch series' current implementation, the modified
> qemu_thread_create()
> in 7/7 patch returns a Boolean value to indicate whether it succeeds
> and set the
> error reason into the passed errp, and did not set the errno. Actually
> another
> similar errno-set issue has been talked in last patch. :)
> If we set the errno in future qemu_thread_create(), we need to
> distinguish the Linux
> and Windows implementation. For Linux, we can use error_setg_errno()
> to set errno.
> But for Windows, I am not sure if we can use
>
> "errno = GetLastError()"
No, that won't work.
> to set errno, as this seems a little weird. Do you have any idea about this?
>
> BTW, if we have a decent errno-set way for Windows, I will adopt your above
> proposal for this patch.
According to MS docs, _beginthreadex() sets errno on failure:
If successful, each of these functions returns a handle to the newly
created thread; however, if the newly created thread exits too
quickly, _beginthread might not return a valid handle. (See the
discussion in the Remarks section.) On an error, _beginthread
returns -1L, and errno is set to EAGAIN if there are too many
threads, to EINVAL if the argument is invalid or the stack size is
incorrect, or to EACCES if there are insufficient resources (such as
memory). On an error, _beginthreadex returns 0, and errno and
_doserrno are set.
https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
Looks like the current code's use of GetLastError() after
_beginthreadex() failure is wrong.
Fix that, and both qemu_thread_create() implementations set errno on
failure, which in turn lets you make qemu_signalfd_compat() and thus
qemu_signalfd() behave sanely regardless of which qemu_thread_create()
implementation they use below the hood.
> Have a nice day, thanks for the review :)
> Fei
>>> @@ -109,7 +108,7 @@ static int qemu_signal_init(void)
>>> #else /* _WIN32 */
>>> -static int qemu_signal_init(void)
>>> +static int qemu_signal_init(Error **errp)
>>> {
>>> return 0;
>>> }
>>> @@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
>>> init_clocks(qemu_timer_notify_cb);
>>> - ret = qemu_signal_init();
>>> + ret = qemu_signal_init(errp);
>>> if (ret) {
>>> return ret;
>>> }
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-12 5:40 ` Fei Li
@ 2018-10-12 8:18 ` Markus Armbruster
2018-10-12 10:23 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-12 8:18 UTC (permalink / raw)
To: Fei Li; +Cc: Markus Armbruster, peterx, famz, qemu-devel, dgilbert, quintela
Fei Li <fli@suse.com> writes:
> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> Add a new Error parameter for vnc_display_init() to handle errors
>>> in its caller: vnc_init_func(), just like vnc_display_open() does.
>>> And let the call trace propagate the Error.
>>>
>>> Besides, make vnc_start_worker_thread() return a bool to indicate
>>> whether it succeeds instead of returning nothing.
>>>
>>> Signed-off-by: Fei Li <fli@suse.com>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> include/ui/console.h | 2 +-
>>> ui/vnc-jobs.c | 9 ++++++---
>>> ui/vnc-jobs.h | 2 +-
>>> ui/vnc.c | 12 +++++++++---
>>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/ui/console.h b/include/ui/console.h
>>> index fb969caf70..c17803c530 100644
>>> --- a/include/ui/console.h
>>> +++ b/include/ui/console.h
>>> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>>> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>>> /* vnc.c */
>>> -void vnc_display_init(const char *id);
>>> +void vnc_display_init(const char *id, Error **errp);
>>> void vnc_display_open(const char *id, Error **errp);
>>> void vnc_display_add_client(const char *id, int csock, bool skipauth);
>>> int vnc_display_password(const char *id, const char *password);
>>> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
>>> index 929391f85d..8807d7217c 100644
>>> --- a/ui/vnc-jobs.c
>>> +++ b/ui/vnc-jobs.c
>>> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
>>> return queue; /* Check global queue */
>>> }
>>> -void vnc_start_worker_thread(void)
>>> +bool vnc_start_worker_thread(Error **errp)
>>> {
>>> VncJobQueue *q;
>>> - if (vnc_worker_thread_running())
>>> - return ;
>>> + if (vnc_worker_thread_running()) {
>>> + goto out;
>>> + }
>>> q = vnc_queue_init();
>>> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
>>> QEMU_THREAD_DETACHED);
>>> queue = q; /* Set global queue */
>>> +out:
>>> + return true;
>>> }
>> This function cannot fail. Why convert it to Error, and complicate its
>> caller?
> [Issue1]
> Actually, this is to pave the way for patch 7/7, in case
> qemu_thread_create() fails.
> Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
> connects the broken errp for callers who initially have the errp.
>
> [I am back... If the other codes in this patches be squashed, maybe
> merge [Issue1]
> into patch 7/7 looks more intuitive.]
>>> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
>>> index 59f66bcc35..14640593db 100644
>>> --- a/ui/vnc-jobs.h
>>> +++ b/ui/vnc-jobs.h
>>> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
>>> void vnc_jobs_join(VncState *vs);
>>> void vnc_jobs_consume_buffer(VncState *vs);
>>> -void vnc_start_worker_thread(void);
>>> +bool vnc_start_worker_thread(Error **errp);
>>> /* Locks */
>>> static inline int vnc_trylock_display(VncDisplay *vd)
>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>> index cf221c83cc..f3806e76db 100644
>>> --- a/ui/vnc.c
>>> +++ b/ui/vnc.c
>>> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>>> .dpy_cursor_define = vnc_dpy_cursor_define,
>>> };
>>> -void vnc_display_init(const char *id)
>>> +void vnc_display_init(const char *id, Error **errp)
>>> {
>>> VncDisplay *vd;
>>>
>> if (vnc_display_find(id) != NULL) {
>> return;
>> }
>> vd = g_malloc0(sizeof(*vd));
>>
>> vd->id = strdup(id);
>> QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
>>
>> QTAILQ_INIT(&vd->clients);
>> vd->expires = TIME_MAX;
>>
>> if (keyboard_layout) {
>> trace_vnc_key_map_init(keyboard_layout);
>> vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>> } else {
>> vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
>> }
>>
>> if (!vd->kbd_layout) {
>> exit(1);
>>
>> Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
>> here makes them fatal. Okay before this patch, but inappropriate
>> afterwards. I'm afraid you have to convert init_keyboard_layout() to
>> Error first.
> [Issue2]
> Right. But I notice the returned kbd_layout_t *kbd_layout is a static
> value and also
> will be used by others, how about passing the errp parameter to
> init_keyboard_layout()
> as follows? And for its other callers, just pass NULL.
>
> @@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
>
> if (keyboard_layout) {
> trace_vnc_key_map_init(keyboard_layout);
> - vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> + vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout, errp);
> } else {
> - vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> + vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
> }
>
> if (!vd->kbd_layout) {
> - exit(1);
> + return;
> }
Sounds good to me, except you should pass &error_fatal instead of NULL
to preserve "report error and exit" semantics.
>>
>> }
>>
>> vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>>> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
>>> vd->connections_limit = 32;
>>> qemu_mutex_init(&vd->mutex);
>>> - vnc_start_worker_thread();
>>> + if (!vnc_start_worker_thread(errp)) {
>>> + return;
>>> + }
>>> vd->dcl.ops = &dcl_ops;
>>> register_displaychangelistener(&vd->dcl);
>>> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>> char *id = (char *)qemu_opts_id(opts);
>>> assert(id);
>>> - vnc_display_init(id);
>>> + vnc_display_init(id, &local_err);
>>> + if (local_err) {
>>> + error_reportf_err(local_err, "Failed to init VNC server: ");
>>> + exit(1);
>>> + }
>>> vnc_display_open(id, &local_err);
>>> if (local_err != NULL) {
>>> error_reportf_err(local_err, "Failed to start VNC server: ");
>> Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
>> vnc_init_func()". Your patch shows that mine is incomplete.
>>
>> Without my patch, there's no real reason for yours, however. The two
>> should be squashed together.
> Ah, I noticed your patch 24/31. OK, let's squash.
> Should I write a new patch by
> - updating to error_propagate(...) if vnc_display_init() fails
> &&
> - modifying [Issue2] ?
> Or you do this in your original patch?
If you send a v2 along that lines, I'll probably pick your patch into my
series. Should work even in case your series gets merged first.
> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
> is &error_fatal,
> then the system will exit(1) when running error_propagate(...) which calls
> error_handle_fatal(...). This means the following two lines will not
> be touched.
> But if we want the following prepended error message, could we move it
> earlier than
> the error_propagare? I mean:
>
> if (local_err != NULL) {
> - error_reportf_err(local_err, "Failed to start VNC server: ");
> - exit(1);
> + error_prepend(&local_err, "Failed to start VNC server: ");
> + error_propagate(errp, local_err);
> + return -1;
> }
Both
error_propagate(errp, local_err);
error_prepend(errp, "Failed to start VNC server: ");
and
error_prepend(&local_err, "Failed to start VNC server: ");
error_propagate(errp, local_err);
work. The former is slightly more efficient when errp is null. But
you're right, it fails to include the "Failed to start VNC server: "
prefix with &error_fatal. Thus, the latter is preferrable.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate
2018-10-12 5:55 ` Fei Li
@ 2018-10-12 8:24 ` Markus Armbruster
2018-10-12 10:25 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-12 8:24 UTC (permalink / raw)
To: Fei Li; +Cc: peterx, famz, qemu-devel, dgilbert, quintela
Fei Li <fli@suse.com> writes:
> On 10/11/2018 09:19 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> The caller of qemu_init_vcpu() already passed the **errp to handle
>> Which caller? There are many. Or do you mean "The callers"?
> Oh, sorry, I mean "The callers" :)
>>
>>> errors. In view of this, add a new Error parameter to the following
>>> call trace to propagate the error and let the further caller check it.
>> Which "call trace"?
> Actually, I want to say all functions called by qemu_init_vcpu()..
>>
>>> Besides, make qemu_init_vcpu() return a Boolean value to let its
>>> callers know whether it succeeds.
>>>
>>> Signed-off-by: Fei Li <fli@suse.com>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> accel/tcg/user-exec-stub.c | 2 +-
>>> cpus.c | 34 +++++++++++++++++++++-------------
>>> include/qom/cpu.h | 2 +-
>>> target/alpha/cpu.c | 4 +++-
>>> target/arm/cpu.c | 4 +++-
>>> target/cris/cpu.c | 4 +++-
>>> target/hppa/cpu.c | 4 +++-
>>> target/i386/cpu.c | 4 +++-
>>> target/lm32/cpu.c | 4 +++-
>>> target/m68k/cpu.c | 4 +++-
>>> target/microblaze/cpu.c | 4 +++-
>>> target/mips/cpu.c | 4 +++-
>>> target/moxie/cpu.c | 4 +++-
>>> target/nios2/cpu.c | 4 +++-
>>> target/openrisc/cpu.c | 4 +++-
>>> target/ppc/translate_init.inc.c | 4 +++-
>>> target/riscv/cpu.c | 4 +++-
>>> target/s390x/cpu.c | 4 +++-
>>> target/sh4/cpu.c | 4 +++-
>>> target/sparc/cpu.c | 4 +++-
>>> target/tilegx/cpu.c | 4 +++-
>>> target/tricore/cpu.c | 4 +++-
>>> target/unicore32/cpu.c | 4 +++-
>>> target/xtensa/cpu.c | 4 +++-
>>> 24 files changed, 86 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>>> index a32b4496af..38f6b928d4 100644
>>> --- a/accel/tcg/user-exec-stub.c
>>> +++ b/accel/tcg/user-exec-stub.c
>>> @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
>>> {
>>> }
>>> -void qemu_init_vcpu(CPUState *cpu)
>>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>>> {
>> You need to return a value here. Sure you compile-tested this?
> Oops! I forget the TCG case.. The `return true` should be added.
> Thanks for pointing this out!
You're welcome.
>>> }
>>> diff --git a/cpus.c b/cpus.c
[...]
>> I see how you changed the code to pass an Error from the
>> qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't
>> see how such errors can be created. Without a way to create any, the
>> patch is pointless. What am I missing?
> This patch is also the pre-patch for the updated qemu_thread_create()
> in patch 7/7
> just as I explained in patch 2/7 [Issue1].
Patches that make little sense on their own, but pave the way for a
later patch are okay. But they should explain that purpose in their
commit message.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-12 7:56 ` Markus Armbruster
@ 2018-10-12 9:42 ` Fei Li
2018-10-12 13:26 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-12 9:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: peterx, famz, qemu-devel, dgilbert, quintela
On 10/12/2018 03:56 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>>> Fei Li <fli@suse.com> writes:
>>>
>>>> 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.
>>> The bug is in qemu_init_main_loop():
>>>
>>> ret = qemu_signal_init();
>>> if (ret) {
>>> return ret;
>>> }
>>>
>>> Fails without setting an error, unlike the other failures. Its callers
>>> crash then.
>> Thanks!
> Consider working that into your commit message.
Ok. I'd like to reword as follows:
Currently in qemu_init_main_loop(), qemu_signal_init() fails without
setting an error
like the other failures. Its callers crash then when runs
error_report_err(err).
>
>>>> 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>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>> include/qemu/osdep.h | 2 +-
>>>> util/compatfd.c | 9 ++++++---
>>>> util/main-loop.c | 9 ++++-----
>>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>>> index 4f8559e550..f1f56763a0 100644
>>>> --- a/include/qemu/osdep.h
>>>> +++ b/include/qemu/osdep.h
>>>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>>>> additional fields in the future) */
>>>> };
>>>> -int qemu_signalfd(const sigset_t *mask);
>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>>>> void sigaction_invoke(struct sigaction *action,
>>>> struct qemu_signalfd_siginfo *info);
>>>> #endif
>>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>>> index 980bd33e52..d3ed890405 100644
>>>> --- a/util/compatfd.c
>>>> +++ b/util/compatfd.c
>>>> @@ -16,6 +16,7 @@
>>>> #include "qemu/osdep.h"
>>>> #include "qemu-common.h"
>>>> #include "qemu/thread.h"
>>>> +#include "qapi/error.h"
>>>> #include <sys/syscall.h>
>>>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>>>> }
>>>> }
>>>> -static int qemu_signalfd_compat(const sigset_t *mask)
>>>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>>>> {
>>>> struct sigfd_compat_info *info;
>>>> QemuThread thread;
>>>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>> info = malloc(sizeof(*info));
>>>> if (info == NULL) {
>>>> + error_setg(errp, "Failed to allocate signalfd memory");
>>>> errno = ENOMEM;
>>>> return -1;
>>>> }
>>>> if (pipe(fds) == -1) {
>>>> + error_setg(errp, "Failed to create signalfd pipe");
>>>> free(info);
>>>> return -1;
>>>> }
>>>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>> return fds[0];
>>>> }
>>>> -int qemu_signalfd(const sigset_t *mask)
>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>>>> {
>>>> #if defined(CONFIG_SIGNALFD)
>>>> int ret;
>>>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>>>> }
>>>> #endif
>>>> - return qemu_signalfd_compat(mask);
>>>> + return qemu_signalfd_compat(mask, errp);
>>>> }
>>> I think this takes the Error conversion too far.
>>>
>>> qemu_signalfd() is like the signalfd() system call, only portable, and
>>> setting FD_CLOEXEC. In particular, it reports failure just like a
>>> system call: it sets errno and returns -1. I'd prefer to keep it that
>>> way. Instead...
>>>
>>>> diff --git a/util/main-loop.c b/util/main-loop.c
>>>> index affe0403c5..9671b6d226 100644
>>>> --- a/util/main-loop.c
>>>> +++ b/util/main-loop.c
>>>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>>>> }
>>>> }
>>>> -static int qemu_signal_init(void)
>>>> +static int qemu_signal_init(Error **errp)
>>>> {
>>>> int sigfd;
>>>> sigset_t set;
>>>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>>> sigdelset(&set, SIG_IPI);
>>>> - sigfd = qemu_signalfd(&set);
>>>> + sigfd = qemu_signalfd(&set, errp);
>>>> if (sigfd == -1) {
>>>> - fprintf(stderr, "failed to create signalfd\n");
>>>> return -errno;
>>>> }
>>>>
>>> ... change this function so:
>>>
>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>> sigdelset(&set, SIG_IPI);
>>> sigfd = qemu_signalfd(&set);
>>> if (sigfd == -1) {
>>> - fprintf(stderr, "failed to create signalfd\n");
>>> + error_setg_errno(errp, errno, "failed to create signalfd");
>>> return -errno;
>>> }
>>>
>>> Does this make sense?
>> Yes, it looks more concise if we only have this patch, but triggers
>> one errno-set
>> issue after applying patch 7/7, which adds a Error **errp parameter for
>> qemu_thread_create() to let its callers handle the error instead of
>> exit() directly
>> to add the robustness.
> I guess you're talking about the qemu_thread_create() in
> qemu_signalfd_compat(). Correct?
Yes.
>
>> For the patch series' current implementation, the modified
>> qemu_thread_create()
>> in 7/7 patch returns a Boolean value to indicate whether it succeeds
>> and set the
>> error reason into the passed errp, and did not set the errno. Actually
>> another
>> similar errno-set issue has been talked in last patch. :)
>> If we set the errno in future qemu_thread_create(), we need to
>> distinguish the Linux
>> and Windows implementation. For Linux, we can use error_setg_errno()
>> to set errno.
>> But for Windows, I am not sure if we can use
>>
>> "errno = GetLastError()"
> No, that won't work.
>
>> to set errno, as this seems a little weird. Do you have any idea about this?
>>
>> BTW, if we have a decent errno-set way for Windows, I will adopt your above
>> proposal for this patch.
> According to MS docs, _beginthreadex() sets errno on failure:
>
> If successful, each of these functions returns a handle to the newly
> created thread; however, if the newly created thread exits too
> quickly, _beginthread might not return a valid handle. (See the
> discussion in the Remarks section.) On an error, _beginthread
> returns -1L, and errno is set to EAGAIN if there are too many
> threads, to EINVAL if the argument is invalid or the stack size is
> incorrect, or to EACCES if there are insufficient resources (such as
> memory). On an error, _beginthreadex returns 0, and errno and
> _doserrno are set.
>
> https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
>
> Looks like the current code's use of GetLastError() after
> _beginthreadex() failure is wrong.
>
> Fix that, and both qemu_thread_create() implementations set errno on
> failure, which in turn lets you make qemu_signalfd_compat() and thus
> qemu_signalfd() behave sanely regardless of which qemu_thread_create()
> implementation they use below the hood.
Thanks for the detail explanation. :)
To fix that, how about replacing the "GetLastError()" with the returned
value "hThread" (actually returns 0)? I mean
...
hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
data, 0, &thread->tid);
if (!hThread) {
if (data->mode != QEMU_THREAD_DETACHED) {
DeleteCriticalSection(&data->cs);
}
error_setg_win32(errp, hThread,
"failed to create win32_start_routine");
g_free(data);
return false;
}
Have a nice day, thanks
Fei
>
>> Have a nice day, thanks for the review :)
>> Fei
>>>> @@ -109,7 +108,7 @@ static int qemu_signal_init(void)
>>>> #else /* _WIN32 */
>>>> -static int qemu_signal_init(void)
>>>> +static int qemu_signal_init(Error **errp)
>>>> {
>>>> return 0;
>>>> }
>>>> @@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
>>>> init_clocks(qemu_timer_notify_cb);
>>>> - ret = qemu_signal_init();
>>>> + ret = qemu_signal_init(errp);
>>>> if (ret) {
>>>> return ret;
>>>> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-12 8:18 ` Markus Armbruster
@ 2018-10-12 10:23 ` Fei Li
2018-10-12 10:50 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-12 10:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: peterx, famz, qemu-devel, dgilbert, quintela
On 10/12/2018 04:18 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>>> Fei Li <fli@suse.com> writes:
>>>
>>>> Add a new Error parameter for vnc_display_init() to handle errors
>>>> in its caller: vnc_init_func(), just like vnc_display_open() does.
>>>> And let the call trace propagate the Error.
>>>>
>>>> Besides, make vnc_start_worker_thread() return a bool to indicate
>>>> whether it succeeds instead of returning nothing.
>>>>
>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>> include/ui/console.h | 2 +-
>>>> ui/vnc-jobs.c | 9 ++++++---
>>>> ui/vnc-jobs.h | 2 +-
>>>> ui/vnc.c | 12 +++++++++---
>>>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/ui/console.h b/include/ui/console.h
>>>> index fb969caf70..c17803c530 100644
>>>> --- a/include/ui/console.h
>>>> +++ b/include/ui/console.h
>>>> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
>>>> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>>>> /* vnc.c */
>>>> -void vnc_display_init(const char *id);
>>>> +void vnc_display_init(const char *id, Error **errp);
>>>> void vnc_display_open(const char *id, Error **errp);
>>>> void vnc_display_add_client(const char *id, int csock, bool skipauth);
>>>> int vnc_display_password(const char *id, const char *password);
>>>> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
>>>> index 929391f85d..8807d7217c 100644
>>>> --- a/ui/vnc-jobs.c
>>>> +++ b/ui/vnc-jobs.c
>>>> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
>>>> return queue; /* Check global queue */
>>>> }
>>>> -void vnc_start_worker_thread(void)
>>>> +bool vnc_start_worker_thread(Error **errp)
>>>> {
>>>> VncJobQueue *q;
>>>> - if (vnc_worker_thread_running())
>>>> - return ;
>>>> + if (vnc_worker_thread_running()) {
>>>> + goto out;
>>>> + }
>>>> q = vnc_queue_init();
>>>> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
>>>> QEMU_THREAD_DETACHED);
>>>> queue = q; /* Set global queue */
>>>> +out:
>>>> + return true;
>>>> }
>>> This function cannot fail. Why convert it to Error, and complicate its
>>> caller?
>> [Issue1]
>> Actually, this is to pave the way for patch 7/7, in case
>> qemu_thread_create() fails.
>> Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
>> connects the broken errp for callers who initially have the errp.
>>
>> [I am back... If the other codes in this patches be squashed, maybe
>> merge [Issue1]
>> into patch 7/7 looks more intuitive.]
>>>> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
>>>> index 59f66bcc35..14640593db 100644
>>>> --- a/ui/vnc-jobs.h
>>>> +++ b/ui/vnc-jobs.h
>>>> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
>>>> void vnc_jobs_join(VncState *vs);
>>>> void vnc_jobs_consume_buffer(VncState *vs);
>>>> -void vnc_start_worker_thread(void);
>>>> +bool vnc_start_worker_thread(Error **errp);
>>>> /* Locks */
>>>> static inline int vnc_trylock_display(VncDisplay *vd)
>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>> index cf221c83cc..f3806e76db 100644
>>>> --- a/ui/vnc.c
>>>> +++ b/ui/vnc.c
>>>> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>>>> .dpy_cursor_define = vnc_dpy_cursor_define,
>>>> };
>>>> -void vnc_display_init(const char *id)
>>>> +void vnc_display_init(const char *id, Error **errp)
>>>> {
>>>> VncDisplay *vd;
>>>>
>>> if (vnc_display_find(id) != NULL) {
>>> return;
>>> }
>>> vd = g_malloc0(sizeof(*vd));
>>>
>>> vd->id = strdup(id);
>>> QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
>>>
>>> QTAILQ_INIT(&vd->clients);
>>> vd->expires = TIME_MAX;
>>>
>>> if (keyboard_layout) {
>>> trace_vnc_key_map_init(keyboard_layout);
>>> vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>>> } else {
>>> vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
>>> }
>>>
>>> if (!vd->kbd_layout) {
>>> exit(1);
>>>
>>> Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
>>> here makes them fatal. Okay before this patch, but inappropriate
>>> afterwards. I'm afraid you have to convert init_keyboard_layout() to
>>> Error first.
>> [Issue2]
>> Right. But I notice the returned kbd_layout_t *kbd_layout is a static
>> value and also
>> will be used by others, how about passing the errp parameter to
>> init_keyboard_layout()
>> as follows? And for its other callers, just pass NULL.
>>
>> @@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
>>
>> if (keyboard_layout) {
>> trace_vnc_key_map_init(keyboard_layout);
>> - vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
>> + vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout, errp);
>> } else {
>> - vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
>> + vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
>> }
>>
>> if (!vd->kbd_layout) {
>> - exit(1);
>> + return;
>> }
> Sounds good to me, except you should pass &error_fatal instead of NULL
> to preserve "report error and exit" semantics.
Yes, you are right. I should pass &error_fatal and let the following
error_setg() handle this.
@@ -94,7 +94,7 @@ static kbd_layout_t *parse_keyboard_layout(const
name2keysym_t *table,
f = filename ? fopen(filename, "r") : NULL;
g_free(filename);
if (!f) {
- fprintf(stderr, "Could not read keymap file: '%s'\n", language);
+ error_setg(errp, "could not read keymap file: '%s'\n", language);
return NULL;
}
>
>>> }
>>>
>>> vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>>>> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
>>>> vd->connections_limit = 32;
>>>> qemu_mutex_init(&vd->mutex);
>>>> - vnc_start_worker_thread();
>>>> + if (!vnc_start_worker_thread(errp)) {
>>>> + return;
>>>> + }
>>>> vd->dcl.ops = &dcl_ops;
>>>> register_displaychangelistener(&vd->dcl);
>>>> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>>> char *id = (char *)qemu_opts_id(opts);
>>>> assert(id);
>>>> - vnc_display_init(id);
>>>> + vnc_display_init(id, &local_err);
>>>> + if (local_err) {
>>>> + error_reportf_err(local_err, "Failed to init VNC server: ");
>>>> + exit(1);
>>>> + }
>>>> vnc_display_open(id, &local_err);
>>>> if (local_err != NULL) {
>>>> error_reportf_err(local_err, "Failed to start VNC server: ");
>>> Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
>>> vnc_init_func()". Your patch shows that mine is incomplete.
>>>
>>> Without my patch, there's no real reason for yours, however. The two
>>> should be squashed together.
>> Ah, I noticed your patch 24/31. OK, let's squash.
>> Should I write a new patch by
>> - updating to error_propagate(...) if vnc_display_init() fails
>> &&
>> - modifying [Issue2] ?
>> Or you do this in your original patch?
> If you send a v2 along that lines, I'll probably pick your patch into my
> series. Should work even in case your series gets merged first.
Good idea. I will send a separate v2 patch which also include the
above change mentioned in [Issue1].
>
>> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
>> is &error_fatal,
>> then the system will exit(1) when running error_propagate(...) which calls
>> error_handle_fatal(...). This means the following two lines will not
>> be touched.
>> But if we want the following prepended error message, could we move it
>> earlier than
>> the error_propagare? I mean:
>>
>> if (local_err != NULL) {
>> - error_reportf_err(local_err, "Failed to start VNC server: ");
>> - exit(1);
>> + error_prepend(&local_err, "Failed to start VNC server: ");
>> + error_propagate(errp, local_err);
>> + return -1;
>> }
> Both
>
> error_propagate(errp, local_err);
> error_prepend(errp, "Failed to start VNC server: ");
>
> and
>
> error_prepend(&local_err, "Failed to start VNC server: ");
> error_propagate(errp, local_err);
>
> work. The former is slightly more efficient when errp is null. But
> you're right, it fails to include the "Failed to start VNC server: "
> prefix with &error_fatal. Thus, the latter is preferrable.
>
>
Have a nice day, thanks
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate
2018-10-12 8:24 ` Markus Armbruster
@ 2018-10-12 10:25 ` Fei Li
0 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-12 10:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: peterx, famz, qemu-devel, dgilbert, quintela
On 10/12/2018 04:24 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> On 10/11/2018 09:19 PM, Markus Armbruster wrote:
>>> Fei Li <fli@suse.com> writes:
>>>
>>>> The caller of qemu_init_vcpu() already passed the **errp to handle
>>> Which caller? There are many. Or do you mean "The callers"?
>> Oh, sorry, I mean "The callers" :)
>>>> errors. In view of this, add a new Error parameter to the following
>>>> call trace to propagate the error and let the further caller check it.
>>> Which "call trace"?
>> Actually, I want to say all functions called by qemu_init_vcpu()..
>>>> Besides, make qemu_init_vcpu() return a Boolean value to let its
>>>> callers know whether it succeeds.
>>>>
>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>> accel/tcg/user-exec-stub.c | 2 +-
>>>> cpus.c | 34 +++++++++++++++++++++-------------
>>>> include/qom/cpu.h | 2 +-
>>>> target/alpha/cpu.c | 4 +++-
>>>> target/arm/cpu.c | 4 +++-
>>>> target/cris/cpu.c | 4 +++-
>>>> target/hppa/cpu.c | 4 +++-
>>>> target/i386/cpu.c | 4 +++-
>>>> target/lm32/cpu.c | 4 +++-
>>>> target/m68k/cpu.c | 4 +++-
>>>> target/microblaze/cpu.c | 4 +++-
>>>> target/mips/cpu.c | 4 +++-
>>>> target/moxie/cpu.c | 4 +++-
>>>> target/nios2/cpu.c | 4 +++-
>>>> target/openrisc/cpu.c | 4 +++-
>>>> target/ppc/translate_init.inc.c | 4 +++-
>>>> target/riscv/cpu.c | 4 +++-
>>>> target/s390x/cpu.c | 4 +++-
>>>> target/sh4/cpu.c | 4 +++-
>>>> target/sparc/cpu.c | 4 +++-
>>>> target/tilegx/cpu.c | 4 +++-
>>>> target/tricore/cpu.c | 4 +++-
>>>> target/unicore32/cpu.c | 4 +++-
>>>> target/xtensa/cpu.c | 4 +++-
>>>> 24 files changed, 86 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>>>> index a32b4496af..38f6b928d4 100644
>>>> --- a/accel/tcg/user-exec-stub.c
>>>> +++ b/accel/tcg/user-exec-stub.c
>>>> @@ -10,7 +10,7 @@ void cpu_resume(CPUState *cpu)
>>>> {
>>>> }
>>>> -void qemu_init_vcpu(CPUState *cpu)
>>>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>>>> {
>>> You need to return a value here. Sure you compile-tested this?
>> Oops! I forget the TCG case.. The `return true` should be added.
>> Thanks for pointing this out!
> You're welcome.
>
>>>> }
>>>> diff --git a/cpus.c b/cpus.c
> [...]
>>> I see how you changed the code to pass an Error from the
>>> qemu_FOO_start_vcpu() through qemu_init_vcpu() to its callers. I can't
>>> see how such errors can be created. Without a way to create any, the
>>> patch is pointless. What am I missing?
>> This patch is also the pre-patch for the updated qemu_thread_create()
>> in patch 7/7
>> just as I explained in patch 2/7 [Issue1].
> Patches that make little sense on their own, but pave the way for a
> later patch are okay. But they should explain that purpose in their
> commit message.
Right. Thanks for the advice, will amend the commit message in next version.
Have a nice day
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
2018-10-12 10:23 ` Fei Li
@ 2018-10-12 10:50 ` Fei Li
0 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-12 10:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: quintela, famz, qemu-devel, peterx, dgilbert
On 10/12/2018 06:23 PM, Fei Li wrote:
>
>
> On 10/12/2018 04:18 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>>>> Fei Li <fli@suse.com> writes:
>>>>
>>>>> Add a new Error parameter for vnc_display_init() to handle errors
>>>>> in its caller: vnc_init_func(), just like vnc_display_open() does.
>>>>> And let the call trace propagate the Error.
>>>>>
>>>>> Besides, make vnc_start_worker_thread() return a bool to indicate
>>>>> whether it succeeds instead of returning nothing.
>>>>>
>>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>>> ---
>>>>> include/ui/console.h | 2 +-
>>>>> ui/vnc-jobs.c | 9 ++++++---
>>>>> ui/vnc-jobs.h | 2 +-
>>>>> ui/vnc.c | 12 +++++++++---
>>>>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/include/ui/console.h b/include/ui/console.h
>>>>> index fb969caf70..c17803c530 100644
>>>>> --- a/include/ui/console.h
>>>>> +++ b/include/ui/console.h
>>>>> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions
>>>>> *opts);
>>>>> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>>>>> /* vnc.c */
>>>>> -void vnc_display_init(const char *id);
>>>>> +void vnc_display_init(const char *id, Error **errp);
>>>>> void vnc_display_open(const char *id, Error **errp);
>>>>> void vnc_display_add_client(const char *id, int csock, bool
>>>>> skipauth);
>>>>> int vnc_display_password(const char *id, const char *password);
>>>>> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
>>>>> index 929391f85d..8807d7217c 100644
>>>>> --- a/ui/vnc-jobs.c
>>>>> +++ b/ui/vnc-jobs.c
>>>>> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
>>>>> return queue; /* Check global queue */
>>>>> }
>>>>> -void vnc_start_worker_thread(void)
>>>>> +bool vnc_start_worker_thread(Error **errp)
>>>>> {
>>>>> VncJobQueue *q;
>>>>> - if (vnc_worker_thread_running())
>>>>> - return ;
>>>>> + if (vnc_worker_thread_running()) {
>>>>> + goto out;
>>>>> + }
>>>>> q = vnc_queue_init();
>>>>> qemu_thread_create(&q->thread, "vnc_worker",
>>>>> vnc_worker_thread, q,
>>>>> QEMU_THREAD_DETACHED);
>>>>> queue = q; /* Set global queue */
>>>>> +out:
>>>>> + return true;
>>>>> }
>>>> This function cannot fail. Why convert it to Error, and complicate
>>>> its
>>>> caller?
>>> [Issue1]
>>> Actually, this is to pave the way for patch 7/7, in case
>>> qemu_thread_create() fails.
>>> Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to
>>> mainly
>>> connects the broken errp for callers who initially have the errp.
>>>
>>> [I am back... If the other codes in this patches be squashed, maybe
>>> merge [Issue1]
>>> into patch 7/7 looks more intuitive.]
>>>>> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
>>>>> index 59f66bcc35..14640593db 100644
>>>>> --- a/ui/vnc-jobs.h
>>>>> +++ b/ui/vnc-jobs.h
>>>>> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
>>>>> void vnc_jobs_join(VncState *vs);
>>>>> void vnc_jobs_consume_buffer(VncState *vs);
>>>>> -void vnc_start_worker_thread(void);
>>>>> +bool vnc_start_worker_thread(Error **errp);
>>>>> /* Locks */
>>>>> static inline int vnc_trylock_display(VncDisplay *vd)
>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>> index cf221c83cc..f3806e76db 100644
>>>>> --- a/ui/vnc.c
>>>>> +++ b/ui/vnc.c
>>>>> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps
>>>>> dcl_ops = {
>>>>> .dpy_cursor_define = vnc_dpy_cursor_define,
>>>>> };
>>>>> -void vnc_display_init(const char *id)
>>>>> +void vnc_display_init(const char *id, Error **errp)
>>>>> {
>>>>> VncDisplay *vd;
>>>> if (vnc_display_find(id) != NULL) {
>>>> return;
>>>> }
>>>> vd = g_malloc0(sizeof(*vd));
>>>>
>>>> vd->id = strdup(id);
>>>> QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
>>>>
>>>> QTAILQ_INIT(&vd->clients);
>>>> vd->expires = TIME_MAX;
>>>>
>>>> if (keyboard_layout) {
>>>> trace_vnc_key_map_init(keyboard_layout);
>>>> vd->kbd_layout = init_keyboard_layout(name2keysym,
>>>> keyboard_layout);
>>>> } else {
>>>> vd->kbd_layout = init_keyboard_layout(name2keysym,
>>>> "en-us");
>>>> }
>>>>
>>>> if (!vd->kbd_layout) {
>>>> exit(1);
>>>>
>>>> Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
>>>> here makes them fatal. Okay before this patch, but inappropriate
>>>> afterwards. I'm afraid you have to convert init_keyboard_layout() to
>>>> Error first.
>>> [Issue2]
>>> Right. But I notice the returned kbd_layout_t *kbd_layout is a static
>>> value and also
>>> will be used by others, how about passing the errp parameter to
>>> init_keyboard_layout()
>>> as follows? And for its other callers, just pass NULL.
>>>
>>> @@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error
>>> **errp)
>>>
>>> if (keyboard_layout) {
>>> trace_vnc_key_map_init(keyboard_layout);
>>> - vd->kbd_layout = init_keyboard_layout(name2keysym,
>>> keyboard_layout);
>>> + vd->kbd_layout = init_keyboard_layout(name2keysym,
>>> keyboard_layout, errp);
>>> } else {
>>> - vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
>>> + vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us",
>>> errp);
>>> }
>>>
>>> if (!vd->kbd_layout) {
>>> - exit(1);
>>> + return;
>>> }
>> Sounds good to me, except you should pass &error_fatal instead of NULL
>> to preserve "report error and exit" semantics.
> Yes, you are right. I should pass &error_fatal and let the following
> error_setg() handle this.
>
> @@ -94,7 +94,7 @@ static kbd_layout_t *parse_keyboard_layout(const
> name2keysym_t *table,
> f = filename ? fopen(filename, "r") : NULL;
> g_free(filename);
> if (!f) {
> - fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> + error_setg(errp, "could not read keymap file: '%s'\n",
> language);
> return NULL;
> }
>
>>
>>>> }
>>>>
>>>> vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>>>>> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
>>>>> vd->connections_limit = 32;
>>>>> qemu_mutex_init(&vd->mutex);
>>>>> - vnc_start_worker_thread();
>>>>> + if (!vnc_start_worker_thread(errp)) {
>>>>> + return;
>>>>> + }
>>>>> vd->dcl.ops = &dcl_ops;
>>>>> register_displaychangelistener(&vd->dcl);
>>>>> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts
>>>>> *opts, Error **errp)
>>>>> char *id = (char *)qemu_opts_id(opts);
>>>>> assert(id);
>>>>> - vnc_display_init(id);
>>>>> + vnc_display_init(id, &local_err);
>>>>> + if (local_err) {
>>>>> + error_reportf_err(local_err, "Failed to init VNC server: ");
>>>>> + exit(1);
>>>>> + }
>>>>> vnc_display_open(id, &local_err);
>>>>> if (local_err != NULL) {
>>>>> error_reportf_err(local_err, "Failed to start VNC
>>>>> server: ");
>>>> Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
>>>> vnc_init_func()". Your patch shows that mine is incomplete.
>>>>
>>>> Without my patch, there's no real reason for yours, however. The two
>>>> should be squashed together.
>>> Ah, I noticed your patch 24/31. OK, let's squash.
>>> Should I write a new patch by
>>> - updating to error_propagate(...) if vnc_display_init() fails
>>> &&
>>> - modifying [Issue2] ?
>>> Or you do this in your original patch?
>> If you send a v2 along that lines, I'll probably pick your patch into my
>> series. Should work even in case your series gets merged first.
> Good idea. I will send a separate v2 patch which also include the
> above change mentioned in [Issue1].
After thinking twice, I think move the above change mentioned in [Issue1]
to patch 7/7 is better. Thus my next separate v2 will not include it.
>>
>>> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
>>> is &error_fatal,
>>> then the system will exit(1) when running error_propagate(...) which
>>> calls
>>> error_handle_fatal(...). This means the following two lines will not
>>> be touched.
>>> But if we want the following prepended error message, could we move it
>>> earlier than
>>> the error_propagare? I mean:
>>>
>>> if (local_err != NULL) {
>>> - error_reportf_err(local_err, "Failed to start VNC server: ");
>>> - exit(1);
>>> + error_prepend(&local_err, "Failed to start VNC server: ");
>>> + error_propagate(errp, local_err);
>>> + return -1;
>>> }
>> Both
>>
>> error_propagate(errp, local_err);
>> error_prepend(errp, "Failed to start VNC server: ");
>>
>> and
>>
>> error_prepend(&local_err, "Failed to start VNC server: ");
>> error_propagate(errp, local_err);
>>
>> work. The former is slightly more efficient when errp is null. But
>> you're right, it fails to include the "Failed to start VNC server: "
>> prefix with &error_fatal. Thus, the latter is preferrable.
>>
>>
> Have a nice day, thanks
> Fei
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-12 9:42 ` Fei Li
@ 2018-10-12 13:26 ` Markus Armbruster
2018-10-17 8:17 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2018-10-12 13:26 UTC (permalink / raw)
To: Fei Li; +Cc: Markus Armbruster, quintela, famz, qemu-devel, peterx, dgilbert
Fei Li <fli@suse.com> writes:
> On 10/12/2018 03:56 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>>>> Fei Li <fli@suse.com> writes:
>>>>
>>>>> 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.
>>>> The bug is in qemu_init_main_loop():
>>>>
>>>> ret = qemu_signal_init();
>>>> if (ret) {
>>>> return ret;
>>>> }
>>>>
>>>> Fails without setting an error, unlike the other failures. Its callers
>>>> crash then.
>>> Thanks!
>> Consider working that into your commit message.
> Ok. I'd like to reword as follows:
> Currently in qemu_init_main_loop(), qemu_signal_init() fails without
> setting an error
> like the other failures. Its callers crash then when runs
> error_report_err(err).
Polishing the English a bit:
When qemu_signal_init() fails in qemu_init_main_loop(), we return
without setting an error. Its callers crash then when they try to
report the error with error_report_err().
>>
>>>>> 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>
>>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>>> ---
>>>>> include/qemu/osdep.h | 2 +-
>>>>> util/compatfd.c | 9 ++++++---
>>>>> util/main-loop.c | 9 ++++-----
>>>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>>>> index 4f8559e550..f1f56763a0 100644
>>>>> --- a/include/qemu/osdep.h
>>>>> +++ b/include/qemu/osdep.h
>>>>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>>>>> additional fields in the future) */
>>>>> };
>>>>> -int qemu_signalfd(const sigset_t *mask);
>>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>>>>> void sigaction_invoke(struct sigaction *action,
>>>>> struct qemu_signalfd_siginfo *info);
>>>>> #endif
>>>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>>>> index 980bd33e52..d3ed890405 100644
>>>>> --- a/util/compatfd.c
>>>>> +++ b/util/compatfd.c
>>>>> @@ -16,6 +16,7 @@
>>>>> #include "qemu/osdep.h"
>>>>> #include "qemu-common.h"
>>>>> #include "qemu/thread.h"
>>>>> +#include "qapi/error.h"
>>>>> #include <sys/syscall.h>
>>>>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>>>>> }
>>>>> }
>>>>> -static int qemu_signalfd_compat(const sigset_t *mask)
>>>>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>>>>> {
>>>>> struct sigfd_compat_info *info;
>>>>> QemuThread thread;
>>>>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>>> info = malloc(sizeof(*info));
>>>>> if (info == NULL) {
>>>>> + error_setg(errp, "Failed to allocate signalfd memory");
>>>>> errno = ENOMEM;
>>>>> return -1;
>>>>> }
>>>>> if (pipe(fds) == -1) {
>>>>> + error_setg(errp, "Failed to create signalfd pipe");
>>>>> free(info);
>>>>> return -1;
>>>>> }
>>>>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>>> return fds[0];
>>>>> }
>>>>> -int qemu_signalfd(const sigset_t *mask)
>>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>>>>> {
>>>>> #if defined(CONFIG_SIGNALFD)
>>>>> int ret;
>>>>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>>>>> }
>>>>> #endif
>>>>> - return qemu_signalfd_compat(mask);
>>>>> + return qemu_signalfd_compat(mask, errp);
>>>>> }
>>>> I think this takes the Error conversion too far.
>>>>
>>>> qemu_signalfd() is like the signalfd() system call, only portable, and
>>>> setting FD_CLOEXEC. In particular, it reports failure just like a
>>>> system call: it sets errno and returns -1. I'd prefer to keep it that
>>>> way. Instead...
>>>>
>>>>> diff --git a/util/main-loop.c b/util/main-loop.c
>>>>> index affe0403c5..9671b6d226 100644
>>>>> --- a/util/main-loop.c
>>>>> +++ b/util/main-loop.c
>>>>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>>>>> }
>>>>> }
>>>>> -static int qemu_signal_init(void)
>>>>> +static int qemu_signal_init(Error **errp)
>>>>> {
>>>>> int sigfd;
>>>>> sigset_t set;
>>>>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>>>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>>>> sigdelset(&set, SIG_IPI);
>>>>> - sigfd = qemu_signalfd(&set);
>>>>> + sigfd = qemu_signalfd(&set, errp);
>>>>> if (sigfd == -1) {
>>>>> - fprintf(stderr, "failed to create signalfd\n");
>>>>> return -errno;
>>>>> }
>>>>>
>>>> ... change this function so:
>>>>
>>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>>> sigdelset(&set, SIG_IPI);
>>>> sigfd = qemu_signalfd(&set);
>>>> if (sigfd == -1) {
>>>> - fprintf(stderr, "failed to create signalfd\n");
>>>> + error_setg_errno(errp, errno, "failed to create signalfd");
>>>> return -errno;
>>>> }
>>>>
>>>> Does this make sense?
>>> Yes, it looks more concise if we only have this patch, but triggers
>>> one errno-set
>>> issue after applying patch 7/7, which adds a Error **errp parameter for
>>> qemu_thread_create() to let its callers handle the error instead of
>>> exit() directly
>>> to add the robustness.
>> I guess you're talking about the qemu_thread_create() in
>> qemu_signalfd_compat(). Correct?
> Yes.
>>
>>> For the patch series' current implementation, the modified
>>> qemu_thread_create()
>>> in 7/7 patch returns a Boolean value to indicate whether it succeeds
>>> and set the
>>> error reason into the passed errp, and did not set the errno. Actually
>>> another
>>> similar errno-set issue has been talked in last patch. :)
>>> If we set the errno in future qemu_thread_create(), we need to
>>> distinguish the Linux
>>> and Windows implementation. For Linux, we can use error_setg_errno()
>>> to set errno.
>>> But for Windows, I am not sure if we can use
>>>
>>> "errno = GetLastError()"
>> No, that won't work.
>>
>>> to set errno, as this seems a little weird. Do you have any idea about this?
>>>
>>> BTW, if we have a decent errno-set way for Windows, I will adopt your above
>>> proposal for this patch.
>> According to MS docs, _beginthreadex() sets errno on failure:
>>
>> If successful, each of these functions returns a handle to the newly
>> created thread; however, if the newly created thread exits too
>> quickly, _beginthread might not return a valid handle. (See the
>> discussion in the Remarks section.) On an error, _beginthread
>> returns -1L, and errno is set to EAGAIN if there are too many
>> threads, to EINVAL if the argument is invalid or the stack size is
>> incorrect, or to EACCES if there are insufficient resources (such as
>> memory). On an error, _beginthreadex returns 0, and errno and
>> _doserrno are set.
>>
>> https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
>>
>> Looks like the current code's use of GetLastError() after
>> _beginthreadex() failure is wrong.
>>
>> Fix that, and both qemu_thread_create() implementations set errno on
>> failure, which in turn lets you make qemu_signalfd_compat() and thus
>> qemu_signalfd() behave sanely regardless of which qemu_thread_create()
>> implementation they use below the hood.
> Thanks for the detail explanation. :)
> To fix that, how about replacing the "GetLastError()" with the returned
> value "hThread" (actually returns 0)? I mean
> ...
> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
> data, 0, &thread->tid);
> if (!hThread) {
> if (data->mode != QEMU_THREAD_DETACHED) {
> DeleteCriticalSection(&data->cs);
> }
> error_setg_win32(errp, hThread,
> "failed to create win32_start_routine");
> g_free(data);
> return false;
> }
No. On failure, _beginthreadex() returns *zero*, not an error code. It
also sets errno. That's the error code you need to use:
hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
data, 0, &thread->tid);
if (!hThread) {
error_setg_errno(errp, errno, "can't create thread");
}
Except I really wouldn't convert qemu_thread_create() to Error! I'd
make it return zero on success, a negative errno code on failure, and
leave the Error business to its callers. Basically, replace
error_exit(err, ...) by return err.
The caller qemu_signalfd_compat() can then do
ret = qemu_thread_create(&thread, "signalfd_compat",
sigwait_compat, info, QEMU_THREAD_DETACHED);
if (ret < 0) {
errno = ret;
return -1;
}
A caller that already has an Error **errp parameter could do
ret = qemu_thread_create(...);
if (ret < 0) {
error_setg_errno(errp, -ret, "<error message goes here>");
}
Callers that want to continue aborting on failure simply do
ret = qemu_thread_create(...);
assert(ret >= 0);
If that turns out to be too much of a bother, you could create a
convenience wrapper for it:
void qemu_thread_create_nofail(QemuThread *thread, const char *name,
void *(*start_routine)(void*),
void *arg, int mode)
{
int err = qemu_thread_create(thread, name, start_routine, arg, mode);
if (err) {
error_exit(err, __func__);
}
}
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-12 13:26 ` Markus Armbruster
@ 2018-10-17 8:17 ` Fei Li
2018-10-19 3:14 ` Fei Li
0 siblings, 1 reply; 27+ messages in thread
From: Fei Li @ 2018-10-17 8:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: quintela, famz, qemu-devel, peterx, dgilbert
Sorry for the late reply! Omitted this one..
On 10/12/2018 09:26 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> On 10/12/2018 03:56 PM, Markus Armbruster wrote:
>>> Fei Li <fli@suse.com> writes:
>>>
>>>> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>>>>> Fei Li <fli@suse.com> writes:
>>>>>
>>>>>> 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.
>>>>> The bug is in qemu_init_main_loop():
>>>>>
>>>>> ret = qemu_signal_init();
>>>>> if (ret) {
>>>>> return ret;
>>>>> }
>>>>>
>>>>> Fails without setting an error, unlike the other failures. Its callers
>>>>> crash then.
>>>> Thanks!
>>> Consider working that into your commit message.
>> Ok. I'd like to reword as follows:
>> Currently in qemu_init_main_loop(), qemu_signal_init() fails without
>> setting an error
>> like the other failures. Its callers crash then when runs
>> error_report_err(err).
> Polishing the English a bit:
>
> When qemu_signal_init() fails in qemu_init_main_loop(), we return
> without setting an error. Its callers crash then when they try to
> report the error with error_report_err().
Thanks. :)
>>>>>> 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>
>>>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>>>> ---
>>>>>> include/qemu/osdep.h | 2 +-
>>>>>> util/compatfd.c | 9 ++++++---
>>>>>> util/main-loop.c | 9 ++++-----
>>>>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>>>>> index 4f8559e550..f1f56763a0 100644
>>>>>> --- a/include/qemu/osdep.h
>>>>>> +++ b/include/qemu/osdep.h
>>>>>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>>>>>> additional fields in the future) */
>>>>>> };
>>>>>> -int qemu_signalfd(const sigset_t *mask);
>>>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>>>>>> void sigaction_invoke(struct sigaction *action,
>>>>>> struct qemu_signalfd_siginfo *info);
>>>>>> #endif
>>>>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>>>>> index 980bd33e52..d3ed890405 100644
>>>>>> --- a/util/compatfd.c
>>>>>> +++ b/util/compatfd.c
>>>>>> @@ -16,6 +16,7 @@
>>>>>> #include "qemu/osdep.h"
>>>>>> #include "qemu-common.h"
>>>>>> #include "qemu/thread.h"
>>>>>> +#include "qapi/error.h"
>>>>>> #include <sys/syscall.h>
>>>>>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>>>>>> }
>>>>>> }
>>>>>> -static int qemu_signalfd_compat(const sigset_t *mask)
>>>>>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>>>>>> {
>>>>>> struct sigfd_compat_info *info;
>>>>>> QemuThread thread;
>>>>>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>>>> info = malloc(sizeof(*info));
>>>>>> if (info == NULL) {
>>>>>> + error_setg(errp, "Failed to allocate signalfd memory");
>>>>>> errno = ENOMEM;
>>>>>> return -1;
>>>>>> }
>>>>>> if (pipe(fds) == -1) {
>>>>>> + error_setg(errp, "Failed to create signalfd pipe");
>>>>>> free(info);
>>>>>> return -1;
>>>>>> }
>>>>>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>>>>> return fds[0];
>>>>>> }
>>>>>> -int qemu_signalfd(const sigset_t *mask)
>>>>>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>>>>>> {
>>>>>> #if defined(CONFIG_SIGNALFD)
>>>>>> int ret;
>>>>>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>>>>>> }
>>>>>> #endif
>>>>>> - return qemu_signalfd_compat(mask);
>>>>>> + return qemu_signalfd_compat(mask, errp);
>>>>>> }
>>>>> I think this takes the Error conversion too far.
>>>>>
>>>>> qemu_signalfd() is like the signalfd() system call, only portable, and
>>>>> setting FD_CLOEXEC. In particular, it reports failure just like a
>>>>> system call: it sets errno and returns -1. I'd prefer to keep it that
>>>>> way. Instead...
>>>>>
>>>>>> diff --git a/util/main-loop.c b/util/main-loop.c
>>>>>> index affe0403c5..9671b6d226 100644
>>>>>> --- a/util/main-loop.c
>>>>>> +++ b/util/main-loop.c
>>>>>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>>>>>> }
>>>>>> }
>>>>>> -static int qemu_signal_init(void)
>>>>>> +static int qemu_signal_init(Error **errp)
>>>>>> {
>>>>>> int sigfd;
>>>>>> sigset_t set;
>>>>>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>>>>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>>>>> sigdelset(&set, SIG_IPI);
>>>>>> - sigfd = qemu_signalfd(&set);
>>>>>> + sigfd = qemu_signalfd(&set, errp);
>>>>>> if (sigfd == -1) {
>>>>>> - fprintf(stderr, "failed to create signalfd\n");
>>>>>> return -errno;
>>>>>> }
>>>>>>
>>>>> ... change this function so:
>>>>>
>>>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>>>> sigdelset(&set, SIG_IPI);
>>>>> sigfd = qemu_signalfd(&set);
>>>>> if (sigfd == -1) {
>>>>> - fprintf(stderr, "failed to create signalfd\n");
>>>>> + error_setg_errno(errp, errno, "failed to create signalfd");
>>>>> return -errno;
>>>>> }
>>>>>
>>>>> Does this make sense?
>>>> Yes, it looks more concise if we only have this patch, but triggers
>>>> one errno-set
>>>> issue after applying patch 7/7, which adds a Error **errp parameter for
>>>> qemu_thread_create() to let its callers handle the error instead of
>>>> exit() directly
>>>> to add the robustness.
>>> I guess you're talking about the qemu_thread_create() in
>>> qemu_signalfd_compat(). Correct?
>> Yes.
>>>> For the patch series' current implementation, the modified
>>>> qemu_thread_create()
>>>> in 7/7 patch returns a Boolean value to indicate whether it succeeds
>>>> and set the
>>>> error reason into the passed errp, and did not set the errno. Actually
>>>> another
>>>> similar errno-set issue has been talked in last patch. :)
>>>> If we set the errno in future qemu_thread_create(), we need to
>>>> distinguish the Linux
>>>> and Windows implementation. For Linux, we can use error_setg_errno()
>>>> to set errno.
>>>> But for Windows, I am not sure if we can use
>>>>
>>>> "errno = GetLastError()"
>>> No, that won't work.
>>>
>>>> to set errno, as this seems a little weird. Do you have any idea about this?
>>>>
>>>> BTW, if we have a decent errno-set way for Windows, I will adopt your above
>>>> proposal for this patch.
>>> According to MS docs, _beginthreadex() sets errno on failure:
>>>
>>> If successful, each of these functions returns a handle to the newly
>>> created thread; however, if the newly created thread exits too
>>> quickly, _beginthread might not return a valid handle. (See the
>>> discussion in the Remarks section.) On an error, _beginthread
>>> returns -1L, and errno is set to EAGAIN if there are too many
>>> threads, to EINVAL if the argument is invalid or the stack size is
>>> incorrect, or to EACCES if there are insufficient resources (such as
>>> memory). On an error, _beginthreadex returns 0, and errno and
>>> _doserrno are set.
>>>
>>> https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
>>>
>>> Looks like the current code's use of GetLastError() after
>>> _beginthreadex() failure is wrong.
>>>
>>> Fix that, and both qemu_thread_create() implementations set errno on
>>> failure, which in turn lets you make qemu_signalfd_compat() and thus
>>> qemu_signalfd() behave sanely regardless of which qemu_thread_create()
>>> implementation they use below the hood.
>> Thanks for the detail explanation. :)
>> To fix that, how about replacing the "GetLastError()" with the returned
>> value "hThread" (actually returns 0)? I mean
>> ...
>> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>> data, 0, &thread->tid);
>> if (!hThread) {
>> if (data->mode != QEMU_THREAD_DETACHED) {
>> DeleteCriticalSection(&data->cs);
>> }
>> error_setg_win32(errp, hThread,
>> "failed to create win32_start_routine");
>> g_free(data);
>> return false;
>> }
> No. On failure, _beginthreadex() returns *zero*, not an error code. It
> also sets errno. That's the error code you need to use:
>
> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
> data, 0, &thread->tid);
> if (!hThread) {
> error_setg_errno(errp, errno, "can't create thread");
> }
Ok, clearer, we also want the errno message to be printed, along
with _beginthreadex() sets the errno's value by default.
Thanks.
> Except I really wouldn't convert qemu_thread_create() to Error! I'd
> make it return zero on success, a negative errno code on failure, and
> leave the Error business to its callers. Basically, replace
> error_exit(err, ...) by return err.
Emm, I am afraid not converting to Error means it is a little bit
trickier to
handle for the callers. Especially for migration callers [see patch 7/7],
they have no initial errp passed, thus error_setg_xx() seems less useful.
Instead in the caller I choose the error_reportf_err(local_err, ...) to
only print
the detail error message and return that caller's original failing tag.
(But for those callers who already have the errp passed, both "return ret"
and "convert qemu_thread_create() to Error" are fine to me.)
Besides, there is only one caller needs the errno value, that is
qemu_signal_init():
"return -errno". Other callers do not use errno to indicate if it succeeds.
Thus the current patches choose pass Error to hold the detail error
message and return a bool to indicate if the function succeeds.
Would you like to share your reason for not converting this function to
Error?
[1#begin]
> The caller qemu_signalfd_compat() can then do
>
> ret = qemu_thread_create(&thread, "signalfd_compat",
> sigwait_compat, info, QEMU_THREAD_DETACHED);
> if (ret < 0) {
> errno = ret;
> return -1;
> }
[1#end]
[2#begin]
> A caller that already has an Error **errp parameter could do
>
> ret = qemu_thread_create(...);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "<error message goes here>");
> }
[2#end]
[3#begin]
> Callers that want to continue aborting on failure simply do
>
> ret = qemu_thread_create(...);
> assert(ret >= 0);
[3#end]
> If that turns out to be too much of a bother, you could create a
> convenience wrapper for it:
>
> void qemu_thread_create_nofail(QemuThread *thread, const char *name,
> void *(*start_routine)(void*),
> void *arg, int mode)
> {
> int err = qemu_thread_create(thread, name, start_routine, arg, mode);
> if (err) {
> error_exit(err, __func__);
> }
> }
I am wondering the above qemu_thread_create_nofail is a convenience for
"[3#begin] => [3#end]" or "[1#begin] => [3#end]"..
If for "[3#begin] => [3#end]", I'd like to use the xxx_nofail wrapper as
the error
message is more detailed.
If for "[1#begin] => [3#end]", I'd like to explain more for this patch
series: we
want our qemu code to be more robust by not making qemu exit(-1) after
qemu_thread_create() fails and let the callers handle this. E.g. for hmp/qmp
callers, make qemu abort() seems too violent if they fails.
Have a nice day
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails
2018-10-17 8:17 ` Fei Li
@ 2018-10-19 3:14 ` Fei Li
0 siblings, 0 replies; 27+ messages in thread
From: Fei Li @ 2018-10-19 3:14 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
Kindly ping. :)
Main discuss whether adding the Error for qemu_thread_create() or not.
For details, please see blow:
On 10/17/2018 04:17 PM, Fei Li wrote:
> Sorry for the late reply! Omitted this one..
>
>
> On 10/12/2018 09:26 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> On 10/12/2018 03:56 PM, Markus Armbruster wrote:
>>>> Fei Li <fli@suse.com> writes:
>>>>
>>>>> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>>>>>> Fei Li <fli@suse.com> writes:
>>>>>>
...snip...
>>>>> For the patch series' current implementation, the modified
>>>>> qemu_thread_create()
>>>>> in 7/7 patch returns a Boolean value to indicate whether it succeeds
>>>>> and set the
>>>>> error reason into the passed errp, and did not set the errno.
>>>>> Actually
>>>>> another
>>>>> similar errno-set issue has been talked in last patch. :)
>>>>> If we set the errno in future qemu_thread_create(), we need to
>>>>> distinguish the Linux
>>>>> and Windows implementation. For Linux, we can use error_setg_errno()
>>>>> to set errno.
>>>>> But for Windows, I am not sure if we can use
>>>>>
>>>>> "errno = GetLastError()"
>>>> No, that won't work.
>>>>
>>>>> to set errno, as this seems a little weird. Do you have any idea
>>>>> about this?
>>>>>
>>>>> BTW, if we have a decent errno-set way for Windows, I will adopt
>>>>> your above
>>>>> proposal for this patch.
>>>> According to MS docs, _beginthreadex() sets errno on failure:
>>>>
>>>> If successful, each of these functions returns a handle to
>>>> the newly
>>>> created thread; however, if the newly created thread exits too
>>>> quickly, _beginthread might not return a valid handle. (See the
>>>> discussion in the Remarks section.) On an error, _beginthread
>>>> returns -1L, and errno is set to EAGAIN if there are too many
>>>> threads, to EINVAL if the argument is invalid or the stack
>>>> size is
>>>> incorrect, or to EACCES if there are insufficient resources
>>>> (such as
>>>> memory). On an error, _beginthreadex returns 0, and errno and
>>>> _doserrno are set.
>>>>
>>>> https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
>>>>
>>>>
>>>> Looks like the current code's use of GetLastError() after
>>>> _beginthreadex() failure is wrong.
>>>>
>>>> Fix that, and both qemu_thread_create() implementations set errno on
>>>> failure, which in turn lets you make qemu_signalfd_compat() and thus
>>>> qemu_signalfd() behave sanely regardless of which qemu_thread_create()
>>>> implementation they use below the hood.
>>> Thanks for the detail explanation. :)
>>> To fix that, how about replacing the "GetLastError()" with the returned
>>> value "hThread" (actually returns 0)? I mean
>>> ...
>>> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>>> data, 0, &thread->tid);
>>> if (!hThread) {
>>> if (data->mode != QEMU_THREAD_DETACHED) {
>>> DeleteCriticalSection(&data->cs);
>>> }
>>> error_setg_win32(errp, hThread,
>>> "failed to create win32_start_routine");
>>> g_free(data);
>>> return false;
>>> }
>> No. On failure, _beginthreadex() returns *zero*, not an error code. It
>> also sets errno. That's the error code you need to use:
>>
>> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>> data, 0, &thread->tid);
>> if (!hThread) {
>> error_setg_errno(errp, errno, "can't create thread");
>> }
> Ok, clearer, we also want the errno message to be printed, along
> with _beginthreadex() sets the errno's value by default.
> Thanks.
The below are my thoughts about why keeping the Error:
>> Except I really wouldn't convert qemu_thread_create() to Error! I'd
>> make it return zero on success, a negative errno code on failure, and
>> leave the Error business to its callers. Basically, replace
>> error_exit(err, ...) by return err.
> Emm, I am afraid not converting to Error means it is a little bit
> trickier to
> handle for the callers. Especially for migration callers [see patch 7/7],
> they have no initial errp passed, thus error_setg_xx() seems less useful.
> Instead in the caller I choose the error_reportf_err(local_err, ...)
> to only print
> the detail error message and return that caller's original failing tag.
> (But for those callers who already have the errp passed, both "return
> ret"
> and "convert qemu_thread_create() to Error" are fine to me.)
>
> Besides, there is only one caller needs the errno value, that is
> qemu_signal_init():
> "return -errno". Other callers do not use errno to indicate if it
> succeeds.
>
> Thus the current patches choose pass Error to hold the detail error
> message and return a bool to indicate if the function succeeds.
>
> Would you like to share your reason for not converting this function
> to Error?
> [1#begin]
>> The caller qemu_signalfd_compat() can then do
>>
>> ret = qemu_thread_create(&thread, "signalfd_compat",
>> sigwait_compat, info,
>> QEMU_THREAD_DETACHED);
>> if (ret < 0) {
>> errno = ret;
>> return -1;
>> }
> [1#end]
> [2#begin]
>> A caller that already has an Error **errp parameter could do
>>
>> ret = qemu_thread_create(...);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret, "<error message goes here>");
>> }
> [2#end]
> [3#begin]
>> Callers that want to continue aborting on failure simply do
>>
>> ret = qemu_thread_create(...);
>> assert(ret >= 0);
> [3#end]
>> If that turns out to be too much of a bother, you could create a
>> convenience wrapper for it:
>>
>> void qemu_thread_create_nofail(QemuThread *thread, const char
>> *name,
>> void *(*start_routine)(void*),
>> void *arg, int mode)
>> {
>> int err = qemu_thread_create(thread, name, start_routine,
>> arg, mode);
>> if (err) {
>> error_exit(err, __func__);
>> }
>> }
> I am wondering the above qemu_thread_create_nofail is a convenience for
> "[3#begin] => [3#end]" or "[1#begin] => [3#end]"..
> If for "[3#begin] => [3#end]", I'd like to use the xxx_nofail wrapper
> as the error
> message is more detailed.
> If for "[1#begin] => [3#end]", I'd like to explain more for this patch
> series: we
> want our qemu code to be more robust by not making qemu exit(-1) after
> qemu_thread_create() fails and let the callers handle this. E.g. for
> hmp/qmp
> callers, make qemu abort() seems too violent if they fails.
>
> Have a nice day
> Fei
Have a nice day, thanks
Fei
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-10-19 3:14 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 12:08 [Qemu-devel] [PATCH RFC v5 0/7] qemu_thread_create: propagate errors to callers to check Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 1/7] Fix segmentation fault when qemu_signal_init fails Fei Li
2018-10-11 10:02 ` Markus Armbruster
2018-10-12 4:24 ` Fei Li
2018-10-12 7:56 ` Markus Armbruster
2018-10-12 9:42 ` Fei Li
2018-10-12 13:26 ` Markus Armbruster
2018-10-17 8:17 ` Fei Li
2018-10-19 3:14 ` Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func Fei Li
2018-10-11 13:13 ` Markus Armbruster
2018-10-12 5:40 ` Fei Li
2018-10-12 8:18 ` Markus Armbruster
2018-10-12 10:23 ` Fei Li
2018-10-12 10:50 ` Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate Fei Li
2018-10-11 13:19 ` Markus Armbruster
2018-10-12 5:55 ` Fei Li
2018-10-12 8:24 ` Markus Armbruster
2018-10-12 10:25 ` Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 4/7] qemu_thread_join: fix segmentation fault Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 5/7] migration: fix the multifd code Fei Li
2018-10-11 13:28 ` Markus Armbruster
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 6/7] migration: fix some error handling Fei Li
2018-10-10 12:08 ` [Qemu-devel] [PATCH RFC v5 7/7] qemu_thread_create: propagate the error to callers to handle Fei Li
2018-10-11 13:45 ` Markus Armbruster
2018-10-12 6:00 ` Fei Li
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.