* [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle
@ 2019-02-01 5:17 Fei Li
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument Fei Li
` (10 more replies)
0 siblings, 11 replies; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:17 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li
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 patch modifies the qemu_thread_create() by passing
&error_abort and returing a value to indicate if it succeeds. The next
10 patches will improve on &error_abort for callers who could handle
more properly.
Please help to review, thanks a lot!
v11:
- Resend as I sent the last version in a mess..
v10:
- Make qemu_thread_create() return -errno instead of a Boolean.
- Add more cleanup for pci_edu_realize()/emulated_realize().
- Polish for iothread_complete()/compress_threads_save_cleanup()/
vnc_start_worker_thread()/touch_all_pages.
- Change to return H_HARDWARE for h_resize_hpt_prepare().
- Remove five derivative patches as they have been merged.
v9:
- To ease the review and involve the appropriate maintainers, split
the previous 6/7 patch into 10 patches: the 6/16 patch passes
the &error_abort to qemu_thread_create() everywhere, and the next
9 patches will improve on &error_abort for callers who need.
- Add a new patch 5/7 to unify error handling for
process_incoming_migration_co().
- Merge the previous 2/7 to current 7/16 to collaboratively handle
for qemu_X_start_vcpu and for the qemu_init_vpcu in each arch.
- Add comment for multifd_recv_new_channel() in current patch 2/7.
v8:
- Remove previous two patches trying to fix the multifd issue on the
source side, as we are still waiting for maintainer's opinions.
- Use atomic_read to get multifd_recv_state->count in patch 3/7.
- Get three more "Reviewed-by:".
v7:
- Split the previous multifd-migration into two patches: the src and
the dst. For the dst, only dump the error instead of quitting.
- Safely do the cleanup for postcopy_ram_enable_notify().
- Split the previous migration-error-handling patch into two patches.
v6:
- Add a new migration-multifd related patch. BTW, delete the previous
vnc related patch as it has been upstreamed.
- Use error_setg_errno() to set the errno when qemu_thread_create()
fails for both Linux and Windows implementation.
- Optimize the first patch, less codes are needed
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
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 (11):
qemu_thread: make qemu_thread_create() take Error ** argument
qemu_thread: supplement error handling for qemu_X_start_vcpu
qemu_thread: supplement error handling for qmp_dump_guest_memory
qemu_thread: supplement error handling for pci_edu_realize
qemu_thread: supplement error handling for h_resize_hpt_prepare
qemu_thread: supplement error handling for emulated_realize
qemu_thread: supplement error handling for iothread_complete
qemu_thread: supplement error handling for qemu_signalfd_compat
qemu_thread: supplement error handling for migration
qemu_thread: supplement error handling for vnc_start_worker_thread
qemu_thread: supplement error handling for touch_all_pages
accel/tcg/user-exec-stub.c | 3 +-
cpus.c | 69 ++++++++++++++++++++-------------
dump.c | 2 +-
hw/misc/edu.c | 11 +++++-
hw/ppc/spapr_hcall.c | 10 ++++-
hw/rdma/rdma_backend.c | 3 +-
hw/usb/ccid-card-emulated.c | 14 +++++--
include/qemu/thread.h | 6 +--
include/qom/cpu.h | 2 +-
io/task.c | 3 +-
iothread.c | 18 ++++++---
migration/migration.c | 30 +++++++++++---
migration/postcopy-ram.c | 14 ++++++-
migration/ram.c | 66 +++++++++++++++++++++----------
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 | 4 +-
util/compatfd.c | 13 ++++++-
util/oslib-posix.c | 25 +++++++-----
util/qemu-thread-posix.c | 30 ++++++++++----
util/qemu-thread-win32.c | 13 +++++--
util/rcu.c | 3 +-
util/thread-pool.c | 4 +-
51 files changed, 337 insertions(+), 136 deletions(-)
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
@ 2019-02-01 5:17 ` Fei Li
2019-02-01 10:02 ` Markus Armbruster
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu Fei Li
` (9 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:17 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Markus Armbruster, Paolo Bonzini
From: Fei Li <fli@suse.com>
qemu_thread_create() abort()s on error. Not nice. Give it a return
value and an Error ** argument, so it can return success/failure.
Considering qemu_thread_create() is quite widely used in qemu, split
this into two steps: this patch passes the &error_abort to
qemu_thread_create() everywhere, and the next 10 patches will improve
on &error_abort for callers who need. To differentiate callers who
need the improvement, temporarily add the "TODO:" comment for them.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
cpus.c | 23 +++++++++++++++--------
dump.c | 3 ++-
hw/misc/edu.c | 4 +++-
hw/ppc/spapr_hcall.c | 4 +++-
hw/rdma/rdma_backend.c | 3 ++-
hw/usb/ccid-card-emulated.c | 5 +++--
include/qemu/thread.h | 6 +++---
io/task.c | 3 ++-
iothread.c | 3 ++-
migration/migration.c | 11 ++++++++---
migration/postcopy-ram.c | 4 +++-
migration/ram.c | 12 ++++++++----
migration/savevm.c | 3 ++-
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 | 6 ++++--
util/compatfd.c | 6 ++++--
util/oslib-posix.c | 3 ++-
util/qemu-thread-posix.c | 30 +++++++++++++++++++++++-------
util/qemu-thread-win32.c | 13 ++++++++++---
util/rcu.c | 3 ++-
util/thread-pool.c | 4 +++-
26 files changed, 114 insertions(+), 51 deletions(-)
diff --git a/cpus.c b/cpus.c
index b09b702712..843a0f06a2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1961,15 +1961,17 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
cpu->cpu_index);
+ /* TODO: let the callers handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
} else {
/* share a single thread for all cpus with TCG */
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
+ /* TODO: let the callers handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name,
qemu_tcg_rr_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
single_tcg_halt_cond = cpu->halt_cond;
single_tcg_cpu_thread = cpu->thread;
@@ -1997,8 +1999,9 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
cpu->cpu_index);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#endif
@@ -2013,8 +2016,9 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
cpu->cpu_index);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
}
static void qemu_hvf_start_vcpu(CPUState *cpu)
@@ -2031,8 +2035,9 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
cpu->cpu_index);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
}
static void qemu_whpx_start_vcpu(CPUState *cpu)
@@ -2044,8 +2049,9 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
cpu->cpu_index);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE);
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#endif
@@ -2060,8 +2066,9 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
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);
+ /* TODO: let the further caller handle the error instead of abort() here */
+ qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, &error_abort);
}
void qemu_init_vcpu(CPUState *cpu)
diff --git a/dump.c b/dump.c
index ef1d8025c9..e4886bc9c3 100644
--- a/dump.c
+++ b/dump.c
@@ -2020,8 +2020,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
if (detach_p) {
/* detached dump */
s->detached = true;
+ /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
- s, QEMU_THREAD_DETACHED);
+ s, QEMU_THREAD_DETACHED, &error_abort);
} else {
/* sync dump */
dump_process(s, errp);
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 91af452c9e..21adbfddce 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -28,6 +28,7 @@
#include "hw/pci/msi.h"
#include "qemu/timer.h"
#include "qemu/main-loop.h" /* iothread mutex */
+#include "qapi/error.h"
#include "qapi/visitor.h"
#define TYPE_PCI_EDU_DEVICE "edu"
@@ -355,8 +356,9 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
qemu_mutex_init(&edu->thr_mutex);
qemu_cond_init(&edu->thr_cond);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
- edu, QEMU_THREAD_JOINABLE);
+ edu, QEMU_THREAD_JOINABLE, &error_abort);
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 17bcaa3822..bee2895b94 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -538,8 +538,10 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
pending->shift = shift;
pending->ret = H_HARDWARE;
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
- hpt_prepare_thread, pending, QEMU_THREAD_DETACHED);
+ hpt_prepare_thread, pending,
+ QEMU_THREAD_DETACHED, &error_abort);
spapr->pending_hpt = pending;
diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index fd571f21e5..2a8d6c027b 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -252,7 +252,8 @@ static void start_comp_thread(RdmaBackendDev *backend_dev)
ibv_get_device_name(backend_dev->ib_dev));
backend_dev->comp_thread.run = true;
qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
- comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
+ comp_handler_thread, backend_dev,
+ QEMU_THREAD_DETACHED, &error_abort);
}
void rdma_backend_register_comp_handler(void (*handler)(void *ctx,
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 963373ba95..0b170f6328 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -544,10 +544,11 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
goto out2;
}
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
- card, QEMU_THREAD_JOINABLE);
+ card, QEMU_THREAD_JOINABLE, &error_abort);
qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
- card, QEMU_THREAD_JOINABLE);
+ card, QEMU_THREAD_JOINABLE, &error_abort);
return;
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 55d83a907c..ec18d8b171 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,
- void *(*start_routine)(void *),
- void *arg, int mode);
+int qemu_thread_create(QemuThread *thread, const char *name,
+ void *(*start_routine)(void *),
+ 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 2fb1cdf55d..8e8aa01999 100644
--- a/iothread.c
+++ b/iothread.c
@@ -178,8 +178,9 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
*/
name = object_get_canonical_path_component(OBJECT(obj));
thread_name = g_strdup_printf("IO %s", name);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&iothread->thread, thread_name, iothread_run,
- iothread, QEMU_THREAD_JOINABLE);
+ iothread, QEMU_THREAD_JOINABLE, &error_abort);
g_free(thread_name);
g_free(name);
diff --git a/migration/migration.c b/migration/migration.c
index 37e06b76dc..1da71211c8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -447,8 +447,10 @@ static void process_incoming_migration_co(void *opaque)
goto fail;
}
+ /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
- colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
+ colo_process_incoming_thread, mis,
+ QEMU_THREAD_JOINABLE, &error_abort);
mis->have_colo_incoming_thread = true;
qemu_coroutine_yield();
@@ -2360,8 +2362,10 @@ static int open_return_path_on_source(MigrationState *ms,
return 0;
}
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&ms->rp_state.rp_thread, "return path",
- source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ source_return_path_thread, ms,
+ QEMU_THREAD_JOINABLE, &error_abort);
trace_open_return_path_on_source_continue();
@@ -3197,8 +3201,9 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
migrate_fd_cleanup(s);
return;
}
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
s->migration_thread_running = true;
}
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index fa09dba534..221ea24919 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1109,8 +1109,10 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
}
qemu_sem_init(&mis->fault_thread_sem, 0);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&mis->fault_thread, "postcopy/fault",
- postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE);
+ postcopy_ram_fault_thread, mis,
+ QEMU_THREAD_JOINABLE, &error_abort);
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 59191c1ed2..700ea229e0 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -502,9 +502,10 @@ 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);
+ /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(compress_threads + i, "compress",
do_data_compress, comp_param + i,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
}
return 0;
@@ -1075,8 +1076,9 @@ 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;
+ /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
atomic_inc(&multifd_send_state->count);
}
@@ -1355,8 +1357,9 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
p->num_packets = 1;
p->running = true;
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
atomic_inc(&multifd_recv_state->count);
return atomic_read(&multifd_recv_state->count) ==
migrate_multifd_channels();
@@ -3649,9 +3652,10 @@ 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;
+ /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(decompress_threads + i, "decompress",
do_data_decompress, decomp_param + i,
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
}
return 0;
exit:
diff --git a/migration/savevm.c b/migration/savevm.c
index 322660438d..d5b45843b6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1747,9 +1747,10 @@ 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);
+ /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(&mis->listen_thread, "postcopy/listen",
postcopy_ram_listen_thread, NULL,
- QEMU_THREAD_DETACHED);
+ QEMU_THREAD_DETACHED, &error_abort);
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 5666f6bbff..52071585a9 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;
@@ -108,7 +109,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 e3b512f26f..728c2e06c7 100644
--- a/tests/qht-bench.c
+++ b/tests/qht-bench.c
@@ -10,6 +10,7 @@
#include "qemu/qht.h"
#include "qemu/rcu.h"
#include "qemu/xxhash.h"
+#include "qapi/error.h"
struct thread_stats {
size_t rd;
@@ -248,7 +249,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 6f076473e0..399744f03c 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 929391f85d..5712f1f501 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"
/*
@@ -339,7 +340,8 @@ void vnc_start_worker_thread(void)
return ;
q = vnc_queue_init();
- qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
- QEMU_THREAD_DETACHED);
+ /* TODO: let the further caller handle the error instead of abort() here */
+ qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread,
+ q, QEMU_THREAD_DETACHED, &error_abort);
queue = q; /* Set global queue */
}
diff --git a/util/compatfd.c b/util/compatfd.c
index 980bd33e52..c3d8448264 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>
@@ -88,8 +89,9 @@ static int qemu_signalfd_compat(const sigset_t *mask)
memcpy(&info->mask, mask, sizeof(*mask));
info->fd = fds[1];
- qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
- QEMU_THREAD_DETACHED);
+ /* TODO: let the further caller handle the error instead of abort() here */
+ qemu_thread_create(&thread, "signalfd_compat", sigwait_compat,
+ info, QEMU_THREAD_DETACHED, &error_abort);
return fds[0];
}
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 4ce1ba9ca4..b6c2ee270d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -452,9 +452,10 @@ 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;
+ /* TODO: let the callers handle the error instead of abort() here */
qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
do_touch_pages, &memset_thread[i],
- QEMU_THREAD_JOINABLE);
+ QEMU_THREAD_JOINABLE, &error_abort);
addr += size_per_thread;
numpages -= numpages_per_thread;
}
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 1bf5e65dea..06a9e78ba8 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,18 @@ static void *qemu_thread_start(void *args)
return r;
}
-void qemu_thread_create(QemuThread *thread, const char *name,
- void *(*start_routine)(void*),
- void *arg, int mode)
+/*
+ * Create a new thread with name @name
+ * The thread executes @start_routine() with argument @arg.
+ * The thread will be created in a detached state if @mode is
+ * QEMU_THREAD_DETACHED, and in a jounable state if it's
+ * QEMU_THREAD_JOINABLE.
+ * On success, return 0.
+ * On failure, store an error through @errp and return negative errno.
+ */
+int 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 +525,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");
+ return -err;
}
if (mode == QEMU_THREAD_DETACHED) {
@@ -538,13 +549,18 @@ void qemu_thread_create(QemuThread *thread, const char *name,
err = pthread_create(&thread->thread, &attr,
qemu_thread_start, qemu_thread_args);
-
- if (err)
- error_exit(err, __func__);
+ if (err) {
+ error_setg(errp, "pthread_create failed");
+ pthread_attr_destroy(&attr);
+ g_free(qemu_thread_args->name);
+ g_free(qemu_thread_args);
+ return -err;
+ }
pthread_sigmask(SIG_SETMASK, &oldset, NULL);
pthread_attr_destroy(&attr);
+ return 0;
}
void qemu_thread_get_self(QemuThread *thread)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 572f88535d..f2deea5250 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -16,6 +16,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;
@@ -384,9 +385,9 @@ void *qemu_thread_join(QemuThread *thread)
return ret;
}
-void qemu_thread_create(QemuThread *thread, const char *name,
+int qemu_thread_create(QemuThread *thread, const char *name,
void *(*start_routine)(void *),
- void *arg, int mode)
+ void *arg, int mode, Error **errp)
{
HANDLE hThread;
struct QemuThreadData *data;
@@ -405,10 +406,16 @@ 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(errp, "failed to create win32_start_routine");
+ g_free(data);
+ return -errno;
}
CloseHandle(hThread);
thread->data = data;
+ return 0;
}
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.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument Fei Li
@ 2019-02-01 5:17 ` Fei Li
2019-02-01 12:33 ` Markus Armbruster
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory Fei Li
` (8 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:17 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Paolo Bonzini
From: Fei Li <fli@suse.com>
The callers of qemu_init_vcpu() already passed the **errp to handle
errors. In view of this, add a new Error parameter to qemu_init_vcpu()
and all qemu_X_start_vcpu() functions called by qemu_init_vcpu() to
propagate the error and let the further callers check it.
Besides, make qemu_init_vcpu() return a Boolean value to let its
callers know whether it succeeds.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
accel/tcg/user-exec-stub.c | 3 +-
cpus.c | 74 +++++++++++++++++++--------------
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, 108 insertions(+), 55 deletions(-)
diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index a32b4496af..f8c38a375c 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -10,8 +10,9 @@ void cpu_resume(CPUState *cpu)
{
}
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
{
+ return true;
}
/* User mode emulation does not support record/replay yet. */
diff --git a/cpus.c b/cpus.c
index 843a0f06a2..4ed7d62e58 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1931,7 +1931,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;
@@ -1961,17 +1961,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
cpu->cpu_index);
- /* TODO: let the callers handle the error instead of abort() here */
- qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(cpu->thread, thread_name,
+ qemu_tcg_cpu_thread_fn, cpu,
+ QEMU_THREAD_JOINABLE, errp) < 0) {
+ return;
+ }
} else {
/* share a single thread for all cpus with TCG */
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
- /* TODO: let the callers handle the error instead of abort() here */
- qemu_thread_create(cpu->thread, thread_name,
- qemu_tcg_rr_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(cpu->thread, thread_name,
+ qemu_tcg_rr_cpu_thread_fn, cpu,
+ QEMU_THREAD_JOINABLE, errp) < 0) {
+ return;
+ }
single_tcg_halt_cond = cpu->halt_cond;
single_tcg_cpu_thread = cpu->thread;
@@ -1989,7 +1992,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];
@@ -1999,15 +2002,16 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
cpu->cpu_index);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
+ return;
+ }
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#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];
@@ -2016,12 +2020,11 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
cpu->cpu_index);
- /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ cpu, QEMU_THREAD_JOINABLE, errp);
}
-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];
@@ -2035,12 +2038,11 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
cpu->cpu_index);
- /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ cpu, QEMU_THREAD_JOINABLE, errp);
}
-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];
@@ -2049,15 +2051,16 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
cpu->cpu_index);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
+ cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
+ return;
+ }
#ifdef _WIN32
cpu->hThread = qemu_thread_get_handle(cpu->thread);
#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];
@@ -2066,16 +2069,16 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
qemu_cond_init(cpu->halt_cond);
snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
cpu->cpu_index);
- /* TODO: let the further caller handle the error instead of abort() here */
qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
- cpu, QEMU_THREAD_JOINABLE, &error_abort);
+ cpu, QEMU_THREAD_JOINABLE, errp);
}
-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,
@@ -2086,22 +2089,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 4c2feb9c17..bbd90bfebc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -1014,7 +1014,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 1fd95d6c0f..7d24ee1f31 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 d6da3f4fed..22b66a6936 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1139,7 +1139,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 7483daef58..b5c5bc370a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5324,7 +5324,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 5596cd5485..310e3d8fd5 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 e217fb3e36..1e5aa69c57 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -145,7 +145,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 541b2a66c7..f8ec7deb38 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 59e0b86762..cfd7777479 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9705,7 +9705,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 28d7e5302f..a4cd7428dd 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -311,7 +311,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 4a4445bdf5..a511c90dab 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -772,7 +772,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 e8d37e4040..67e3eb03d9 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 2b49d1ca40..0c737c3187 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.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument Fei Li
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu Fei Li
@ 2019-02-01 5:17 ` Fei Li
2019-02-01 12:34 ` Markus Armbruster
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize Fei Li
` (7 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:17 UTC (permalink / raw)
To: qemu-devel, shirley17fei
Cc: Fei Li, Markus Armbruster, Marc-André Lureau
From: Fei Li <fli@suse.com>
Utilize the existed errp to propagate the error instead of the
temporary &error_abort.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
dump.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/dump.c b/dump.c
index e4886bc9c3..92cc277015 100644
--- a/dump.c
+++ b/dump.c
@@ -2020,9 +2020,8 @@ void qmp_dump_guest_memory(bool paging, const char *file,
if (detach_p) {
/* detached dump */
s->detached = true;
- /* TODO: let the further caller handle the error instead of abort() */
qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
- s, QEMU_THREAD_DETACHED, &error_abort);
+ s, QEMU_THREAD_DETACHED, errp);
} else {
/* sync dump */
dump_process(s, errp);
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (2 preceding siblings ...)
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory Fei Li
@ 2019-02-01 5:17 ` Fei Li
2019-02-01 12:58 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare Fei Li
` (6 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:17 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Markus Armbruster, Jiri Slaby
From: Fei Li <fli@suse.com>
Utilize the existed errp to propagate the error and do the
corresponding cleanup to replace the temporary &error_abort.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Fei Li <fli@suse.com>
---
hw/misc/edu.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 21adbfddce..8fe232b6d6 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -356,9 +356,14 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
qemu_mutex_init(&edu->thr_mutex);
qemu_cond_init(&edu->thr_cond);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
- edu, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+ edu, QEMU_THREAD_JOINABLE, errp) < 0) {
+ qemu_cond_destroy(&edu->thr_cond);
+ qemu_mutex_destroy(&edu->thr_mutex);
+ timer_del(&edu->dma_timer);
+ msi_uninit(pdev);
+ return;
+ }
memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
"edu-mmio", 1 * MiB);
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (3 preceding siblings ...)
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 13:01 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize Fei Li
` (5 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Markus Armbruster, David Gibson
From: Fei Li <fli@suse.com>
Add a local_err to hold the error, and return the corresponding
error code to replace the temporary &error_abort.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Fei Li <fli@suse.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_hcall.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index bee2895b94..5ef73bef0e 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,10 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
pending->shift = shift;
pending->ret = H_HARDWARE;
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
- hpt_prepare_thread, pending,
- QEMU_THREAD_DETACHED, &error_abort);
+ if (qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+ hpt_prepare_thread, pending,
+ QEMU_THREAD_DETACHED, &local_err) < 0) {
+ error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+ g_free(pending);
+ return H_HARDWARE;
+ }
spapr->pending_hpt = pending;
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (4 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 13:04 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete Fei Li
` (4 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Markus Armbruster, Gerd Hoffmann
From: Fei Li <fli@suse.com>
Utilize the existed errp to propagate the error and do the
corresponding cleanup to replace the temporary &error_abort.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
hw/usb/ccid-card-emulated.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 0b170f6328..19b4b9a8fa 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
goto out2;
}
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
- card, QEMU_THREAD_JOINABLE, &error_abort);
- qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
- card, QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
+ card, QEMU_THREAD_JOINABLE, errp) < 0) {
+ goto out2;
+ }
+ if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
+ handle_apdu_thread, card,
+ QEMU_THREAD_JOINABLE, errp) < 0) {
+ qemu_thread_join(&card->event_thread_id);
+ goto out2;
+ }
return;
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (5 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 14:03 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat Fei Li
` (3 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei
Cc: Fei Li, Markus Armbruster, Stefan Hajnoczi, Eric Blake
From: Fei Li <fli@suse.com>
For iothread_complete: utilize the existed errp to propagate the
error and do the corresponding cleanup to replace the temporary
&error_abort.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
iothread.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/iothread.c b/iothread.c
index 8e8aa01999..ea2e553dc5 100644
--- a/iothread.c
+++ b/iothread.c
@@ -148,6 +148,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
Error *local_error = NULL;
IOThread *iothread = IOTHREAD(obj);
char *name, *thread_name;
+ int thread_ok;
iothread->stopping = false;
iothread->running = true;
@@ -164,9 +165,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);
@@ -178,11 +177,15 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
*/
name = object_get_canonical_path_component(OBJECT(obj));
thread_name = g_strdup_printf("IO %s", name);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&iothread->thread, thread_name, iothread_run,
- iothread, QEMU_THREAD_JOINABLE, &error_abort);
+ thread_ok = qemu_thread_create(&iothread->thread, thread_name, iothread_run,
+ iothread, QEMU_THREAD_JOINABLE, errp);
g_free(thread_name);
g_free(name);
+ if (thread_ok < 0) {
+ qemu_cond_destroy(&iothread->init_done_cond);
+ qemu_mutex_destroy(&iothread->init_done_lock);
+ goto fail;
+ }
/* Wait for initialization to complete */
qemu_mutex_lock(&iothread->init_done_lock);
@@ -191,6 +194,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 {
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (6 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 14:13 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration Fei Li
` (2 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Markus Armbruster, Eric Blake
From: Fei Li <shirley17fei@gmail.com>
For qemu_signalfd_compat: set errno, do some cleanup, and return
-1 to replace the temporary &error_abort when failing to create
sigwait_compat.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Fei Li <shirley17fei@gmail.com>
---
util/compatfd.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/util/compatfd.c b/util/compatfd.c
index c3d8448264..9d642475fc 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -71,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
struct sigfd_compat_info *info;
QemuThread thread;
int fds[2];
+ int ret;
info = malloc(sizeof(*info));
if (info == NULL) {
@@ -89,9 +90,15 @@ static int qemu_signalfd_compat(const sigset_t *mask)
memcpy(&info->mask, mask, sizeof(*mask));
info->fd = fds[1];
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&thread, "signalfd_compat", sigwait_compat,
- info, QEMU_THREAD_DETACHED, &error_abort);
+ ret = qemu_thread_create(&thread, "signalfd_compat", sigwait_compat,
+ info, QEMU_THREAD_DETACHED, NULL);
+ if (ret < 0) {
+ close(fds[0]);
+ close(fds[1]);
+ free(info);
+ errno = -ret;
+ return -1;
+ }
return fds[0];
}
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (7 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 5:35 ` Fei Li
2019-02-01 15:34 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread Fei Li
2019-07-12 5:57 ` [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Markus Armbruster
10 siblings, 2 replies; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei
Cc: Fei Li, Markus Armbruster, Dr . David Alan Gilbert
From: Fei Li <fli@suse.com>
Update qemu_thread_create()'s callers by
- setting an error on qemu_thread_create() failure for callers that
set an error on failure;
- reporting the error and returning failure for callers that return
an error code on failure;
- reporting the error and setting some state for callers that just
report errors and choose not to continue on.
Besides, make compress_threads_save_cleanup() cope with partially
initialized comp_param[i] to adapt to the new qemu_thread_create()
failure case.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
migration/migration.c | 35 +++++++++++++-------
migration/postcopy-ram.c | 16 ++++++---
migration/ram.c | 70 ++++++++++++++++++++++++++--------------
migration/savevm.c | 12 ++++---
4 files changed, 89 insertions(+), 44 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 1da71211c8..0034ca1334 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
goto fail;
}
- /* TODO: let the further caller handle the error instead of abort() */
- qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
- colo_process_incoming_thread, mis,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
+ colo_process_incoming_thread, mis,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ error_reportf_err(local_err, "failed to create "
+ "colo_process_incoming_thread: ");
+ goto fail;
+ }
mis->have_colo_incoming_thread = true;
qemu_coroutine_yield();
@@ -2349,6 +2352,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) {
@@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
return 0;
}
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&ms->rp_state.rp_thread, "return path",
- source_return_path_thread, ms,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
+ source_return_path_thread, ms,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ error_reportf_err(local_err,
+ "failed to create source_return_path_thread: ");
+ qemu_fclose(ms->rp_state.from_dst_file);
+ ms->rp_state.from_dst_file = NULL;
+ return -1;
+ }
trace_open_return_path_on_source_continue();
@@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
migrate_fd_cleanup(s);
return;
}
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
+ QEMU_THREAD_JOINABLE, &error_in) < 0) {
+ 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 221ea24919..0934a1403a 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1083,6 +1083,8 @@ retry:
int postcopy_ram_enable_notify(MigrationIncomingState *mis)
{
+ Error *local_err = NULL;
+
/* Open the fd for the kernel to give us userfaults */
mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
if (mis->userfault_fd == -1) {
@@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
}
qemu_sem_init(&mis->fault_thread_sem, 0);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&mis->fault_thread, "postcopy/fault",
- postcopy_ram_fault_thread, mis,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
+ postcopy_ram_fault_thread, mis,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ 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 700ea229e0..66b8b764f1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
thread_count = migrate_compress_threads();
for (i = 0; i < thread_count; i++) {
+ qemu_mutex_lock(&comp_param[i].mutex);
+ comp_param[i].quit = true;
+ qemu_cond_signal(&comp_param[i].cond);
+ qemu_mutex_unlock(&comp_param[i].mutex);
+
+ qemu_mutex_destroy(&comp_param[i].mutex);
+ qemu_cond_destroy(&comp_param[i].cond);
+
/*
* we use it as a indicator which shows if the thread is
* properly init'd or not
@@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
if (!comp_param[i].file) {
break;
}
-
- qemu_mutex_lock(&comp_param[i].mutex);
- comp_param[i].quit = true;
- qemu_cond_signal(&comp_param[i].cond);
- qemu_mutex_unlock(&comp_param[i].mutex);
-
qemu_thread_join(compress_threads + i);
- qemu_mutex_destroy(&comp_param[i].mutex);
- qemu_cond_destroy(&comp_param[i].cond);
deflateEnd(&comp_param[i].stream);
g_free(comp_param[i].originbuf);
qemu_fclose(comp_param[i].file);
@@ -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;
@@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
qemu_cond_init(&comp_done_cond);
qemu_mutex_init(&comp_done_lock);
for (i = 0; i < thread_count; i++) {
+ qemu_mutex_init(&comp_param[i].mutex);
+ qemu_cond_init(&comp_param[i].cond);
+ comp_param[i].quit = false;
comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
if (!comp_param[i].originbuf) {
goto exit;
@@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
*/
comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
comp_param[i].done = true;
- comp_param[i].quit = false;
- qemu_mutex_init(&comp_param[i].mutex);
- qemu_cond_init(&comp_param[i].cond);
- /* TODO: let the further caller handle the error instead of abort() */
- qemu_thread_create(compress_threads + i, "compress",
- do_data_compress, comp_param + i,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(compress_threads + i, "compress",
+ do_data_compress, comp_param + i,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ error_reportf_err(local_err, "failed to create do_data_compress: ");
+ deflateEnd(&comp_param[i].stream);
+ g_free(comp_param[i].originbuf);
+ qemu_fclose(comp_param[i].file);
+ comp_param[i].file = NULL;
+ goto exit;
+ }
}
return 0;
@@ -1076,9 +1083,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;
- /* TODO: let the further caller handle the error instead of abort() */
- qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ migrate_set_error(migrate_get_current(), local_err);
+ error_reportf_err(local_err,
+ "failed to create multifd_send_thread: ");
+ multifd_save_cleanup();
+ return;
+ }
atomic_inc(&multifd_send_state->count);
}
@@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
p->num_packets = 1;
p->running = true;
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ error_propagate_prepend(errp, local_err,
+ "failed to create multifd_recv_thread: ");
+ multifd_recv_terminate_threads(local_err);
+ return false;
+ }
atomic_inc(&multifd_recv_state->count);
return atomic_read(&multifd_recv_state->count) ==
migrate_multifd_channels();
@@ -3631,6 +3647,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;
@@ -3652,10 +3669,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;
- /* TODO: let the further caller handle the error instead of abort() */
- qemu_thread_create(decompress_threads + i, "decompress",
- do_data_decompress, decomp_param + i,
- QEMU_THREAD_JOINABLE, &error_abort);
+ if (qemu_thread_create(decompress_threads + i, "decompress",
+ do_data_decompress, decomp_param + i,
+ QEMU_THREAD_JOINABLE, &local_err) < 0) {
+ 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 d5b45843b6..310cecbf8f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1747,10 +1747,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);
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&mis->listen_thread, "postcopy/listen",
- postcopy_ram_listen_thread, NULL,
- QEMU_THREAD_DETACHED, &error_abort);
+ if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
+ postcopy_ram_listen_thread, NULL,
+ QEMU_THREAD_DETACHED, &local_err) < 0) {
+ 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);
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (8 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration Fei Li
@ 2019-02-01 5:18 ` Fei Li
2019-02-01 14:16 ` Markus Armbruster
2019-07-12 5:57 ` [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Markus Armbruster
10 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:18 UTC (permalink / raw)
To: qemu-devel, shirley17fei; +Cc: Fei Li, Markus Armbruster, Gerd Hoffmann
From: Fei Li <fli@suse.com>
Supplement the error handling for vnc_thread_worker_thread: add
an Error parameter for it to propagate the error to its caller to
handle in case it fails, and make it return a Boolean to indicate
whether it succeeds.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
ui/vnc-jobs.c | 17 +++++++++++------
ui/vnc-jobs.h | 2 +-
ui/vnc.c | 4 +++-
3 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 5712f1f501..1371895513 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -332,16 +332,21 @@ 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()) {
+ return true;
+ }
q = vnc_queue_init();
- /* TODO: let the further caller handle the error instead of abort() here */
- qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread,
- q, QEMU_THREAD_DETACHED, &error_abort);
+ if (qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread,
+ q, QEMU_THREAD_DETACHED, errp) < 0) {
+ vnc_queue_clear(q);
+ return false;
+ }
queue = q; /* Set global queue */
+
+ 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 6002d09407..2ab920f1a7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3255,7 +3255,9 @@ void vnc_display_init(const char *id, Error **errp)
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);
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration Fei Li
@ 2019-02-01 5:35 ` Fei Li
2019-02-01 15:34 ` Markus Armbruster
1 sibling, 0 replies; 34+ messages in thread
From: Fei Li @ 2019-02-01 5:35 UTC (permalink / raw)
To: Fei Li, qemu-devel; +Cc: Fei Li, Markus Armbruster, Dr . David Alan Gilbert
在 2019/2/1 下午1:18, Fei Li 写道:
> From: Fei Li <fli@suse.com>
>
> Update qemu_thread_create()'s callers by
> - setting an error on qemu_thread_create() failure for callers that
> set an error on failure;
> - reporting the error and returning failure for callers that return
> an error code on failure;
> - reporting the error and setting some state for callers that just
> report errors and choose not to continue on.
>
> Besides, make compress_threads_save_cleanup() cope with partially
> initialized comp_param[i] to adapt to the new qemu_thread_create()
> failure case.
Hi David,
I got your Reviewed-by in last version at an early stage, but later one
issue is raised
by Markus due to the improper “[patch v9 16/16] qemu_thread_join: fix
segmentation fault”.
Thus in this v11, I do an update for the following two functions:
compress_threads_save_cleanup() & compress_threads_save_setup()
Please help to review again, thanks :)
Have a nice day
Fei
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 35 +++++++++++++-------
> migration/postcopy-ram.c | 16 ++++++---
> migration/ram.c | 70 ++++++++++++++++++++++++++--------------
> migration/savevm.c | 12 ++++---
> 4 files changed, 89 insertions(+), 44 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1da71211c8..0034ca1334 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
> goto fail;
> }
>
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> - colo_process_incoming_thread, mis,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> + colo_process_incoming_thread, mis,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err, "failed to create "
> + "colo_process_incoming_thread: ");
> + goto fail;
> + }
> mis->have_colo_incoming_thread = true;
> qemu_coroutine_yield();
>
> @@ -2349,6 +2352,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) {
> @@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
> return 0;
> }
>
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> - source_return_path_thread, ms,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> + source_return_path_thread, ms,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err,
> + "failed to create source_return_path_thread: ");
> + qemu_fclose(ms->rp_state.from_dst_file);
> + ms->rp_state.from_dst_file = NULL;
> + return -1;
> + }
>
> trace_open_return_path_on_source_continue();
>
> @@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> migrate_fd_cleanup(s);
> return;
> }
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> + QEMU_THREAD_JOINABLE, &error_in) < 0) {
> + 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 221ea24919..0934a1403a 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1083,6 +1083,8 @@ retry:
>
> int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> {
> + Error *local_err = NULL;
> +
> /* Open the fd for the kernel to give us userfaults */
> mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> if (mis->userfault_fd == -1) {
> @@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> }
>
> qemu_sem_init(&mis->fault_thread_sem, 0);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> - postcopy_ram_fault_thread, mis,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> + postcopy_ram_fault_thread, mis,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + 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 700ea229e0..66b8b764f1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
>
> thread_count = migrate_compress_threads();
> for (i = 0; i < thread_count; i++) {
> + qemu_mutex_lock(&comp_param[i].mutex);
> + comp_param[i].quit = true;
> + qemu_cond_signal(&comp_param[i].cond);
> + qemu_mutex_unlock(&comp_param[i].mutex);
> +
> + qemu_mutex_destroy(&comp_param[i].mutex);
> + qemu_cond_destroy(&comp_param[i].cond);
> +
> /*
> * we use it as a indicator which shows if the thread is
> * properly init'd or not
> @@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
> if (!comp_param[i].file) {
> break;
> }
> -
> - qemu_mutex_lock(&comp_param[i].mutex);
> - comp_param[i].quit = true;
> - qemu_cond_signal(&comp_param[i].cond);
> - qemu_mutex_unlock(&comp_param[i].mutex);
> -
> qemu_thread_join(compress_threads + i);
> - qemu_mutex_destroy(&comp_param[i].mutex);
> - qemu_cond_destroy(&comp_param[i].cond);
> deflateEnd(&comp_param[i].stream);
> g_free(comp_param[i].originbuf);
> qemu_fclose(comp_param[i].file);
> @@ -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;
> @@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
> qemu_cond_init(&comp_done_cond);
> qemu_mutex_init(&comp_done_lock);
> for (i = 0; i < thread_count; i++) {
> + qemu_mutex_init(&comp_param[i].mutex);
> + qemu_cond_init(&comp_param[i].cond);
> + comp_param[i].quit = false;
> comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
> if (!comp_param[i].originbuf) {
> goto exit;
> @@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
> */
> comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> comp_param[i].done = true;
> - comp_param[i].quit = false;
> - qemu_mutex_init(&comp_param[i].mutex);
> - qemu_cond_init(&comp_param[i].cond);
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(compress_threads + i, "compress",
> - do_data_compress, comp_param + i,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(compress_threads + i, "compress",
> + do_data_compress, comp_param + i,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err, "failed to create do_data_compress: ");
> + deflateEnd(&comp_param[i].stream);
> + g_free(comp_param[i].originbuf);
> + qemu_fclose(comp_param[i].file);
> + comp_param[i].file = NULL;
> + goto exit;
> + }
> }
> return 0;
>
> @@ -1076,9 +1083,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;
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + migrate_set_error(migrate_get_current(), local_err);
> + error_reportf_err(local_err,
> + "failed to create multifd_send_thread: ");
> + multifd_save_cleanup();
> + return;
> + }
>
> atomic_inc(&multifd_send_state->count);
> }
> @@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
> p->num_packets = 1;
>
> p->running = true;
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_propagate_prepend(errp, local_err,
> + "failed to create multifd_recv_thread: ");
> + multifd_recv_terminate_threads(local_err);
> + return false;
> + }
> atomic_inc(&multifd_recv_state->count);
> return atomic_read(&multifd_recv_state->count) ==
> migrate_multifd_channels();
> @@ -3631,6 +3647,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;
> @@ -3652,10 +3669,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;
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(decompress_threads + i, "decompress",
> - do_data_decompress, decomp_param + i,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(decompress_threads + i, "decompress",
> + do_data_decompress, decomp_param + i,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + 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 d5b45843b6..310cecbf8f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1747,10 +1747,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);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> - postcopy_ram_listen_thread, NULL,
> - QEMU_THREAD_DETACHED, &error_abort);
> + if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> + postcopy_ram_listen_thread, NULL,
> + QEMU_THREAD_DETACHED, &local_err) < 0) {
> + 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);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument Fei Li
@ 2019-02-01 10:02 ` Markus Armbruster
0 siblings, 0 replies; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 10:02 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Fei Li, Paolo Bonzini
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> qemu_thread_create() abort()s on error. Not nice. Give it a return
> value and an Error ** argument, so it can return success/failure.
>
> Considering qemu_thread_create() is quite widely used in qemu, split
> this into two steps: this patch passes the &error_abort to
> qemu_thread_create() everywhere, and the next 10 patches will improve
> on &error_abort for callers who need. To differentiate callers who
> need the improvement, temporarily add the "TODO:" comment for them.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu Fei Li
@ 2019-02-01 12:33 ` Markus Armbruster
2019-02-02 4:47 ` fei
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 12:33 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Fei Li, Paolo Bonzini
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> The callers of qemu_init_vcpu() already passed the **errp to handle
> errors. In view of this, add a new Error parameter to qemu_init_vcpu()
> and all qemu_X_start_vcpu() functions called by qemu_init_vcpu() to
> propagate the error and let the further callers check it.
>
> Besides, make qemu_init_vcpu() return a Boolean value to let its
> callers know whether it succeeds.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
> accel/tcg/user-exec-stub.c | 3 +-
> cpus.c | 74 +++++++++++++++++++--------------
> 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, 108 insertions(+), 55 deletions(-)
>
> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
> index a32b4496af..f8c38a375c 100644
> --- a/accel/tcg/user-exec-stub.c
> +++ b/accel/tcg/user-exec-stub.c
> @@ -10,8 +10,9 @@ void cpu_resume(CPUState *cpu)
> {
> }
>
> -void qemu_init_vcpu(CPUState *cpu)
> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
> {
> + return true;
> }
>
> /* User mode emulation does not support record/replay yet. */
> diff --git a/cpus.c b/cpus.c
> index 843a0f06a2..4ed7d62e58 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1931,7 +1931,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;
> @@ -1961,17 +1961,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
> cpu->cpu_index);
>
> - /* TODO: let the callers handle the error instead of abort() here */
> - qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(cpu->thread, thread_name,
> + qemu_tcg_cpu_thread_fn, cpu,
> + QEMU_THREAD_JOINABLE, errp) < 0) {
> + return;
> + }
>
> } else {
> /* share a single thread for all cpus with TCG */
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
> - /* TODO: let the callers handle the error instead of abort() here */
> - qemu_thread_create(cpu->thread, thread_name,
> - qemu_tcg_rr_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(cpu->thread, thread_name,
> + qemu_tcg_rr_cpu_thread_fn, cpu,
> + QEMU_THREAD_JOINABLE, errp) < 0) {
> + return;
> + }
>
> single_tcg_halt_cond = cpu->halt_cond;
> single_tcg_cpu_thread = cpu->thread;
> @@ -1989,7 +1992,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];
>
> @@ -1999,15 +2002,16 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
>
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
> cpu->cpu_index);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
> + cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
> + return;
> + }
> #ifdef _WIN32
> cpu->hThread = qemu_thread_get_handle(cpu->thread);
> #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];
>
> @@ -2016,12 +2020,11 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
> qemu_cond_init(cpu->halt_cond);
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
> cpu->cpu_index);
> - /* TODO: let the further caller handle the error instead of abort() here */
> qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + cpu, QEMU_THREAD_JOINABLE, errp);
> }
>
> -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];
>
> @@ -2035,12 +2038,11 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
>
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
> cpu->cpu_index);
> - /* TODO: let the further caller handle the error instead of abort() here */
> qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + cpu, QEMU_THREAD_JOINABLE, errp);
> }
>
> -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];
>
> @@ -2049,15 +2051,16 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
> qemu_cond_init(cpu->halt_cond);
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
> cpu->cpu_index);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
> + cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
> + return;
> + }
> #ifdef _WIN32
> cpu->hThread = qemu_thread_get_handle(cpu->thread);
> #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];
>
> @@ -2066,16 +2069,16 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
> qemu_cond_init(cpu->halt_cond);
> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
> cpu->cpu_index);
> - /* TODO: let the further caller handle the error instead of abort() here */
> qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
> + cpu, QEMU_THREAD_JOINABLE, errp);
> }
>
> -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,
> @@ -2086,22 +2089,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)
If the qemu_FOO_init_vcpu() returned success / failure like their callee
qemu_thread_create() and their caller qemu_init_vcpu() do, then this
code would be simpler.
But it's not wrong, and we're at v11, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory Fei Li
@ 2019-02-01 12:34 ` Markus Armbruster
0 siblings, 0 replies; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 12:34 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Fei Li, Marc-André Lureau
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Utilize the existed errp to propagate the error instead of the
> temporary &error_abort.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize Fei Li
@ 2019-02-01 12:58 ` Markus Armbruster
2019-02-02 4:44 ` fei
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 12:58 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Jiri Slaby
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Utilize the existed errp to propagate the error and do the
> corresponding cleanup to replace the temporary &error_abort.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
> hw/misc/edu.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 21adbfddce..8fe232b6d6 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -356,9 +356,14 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
>
> qemu_mutex_init(&edu->thr_mutex);
> qemu_cond_init(&edu->thr_cond);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
> - edu, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
> + edu, QEMU_THREAD_JOINABLE, errp) < 0) {
> + qemu_cond_destroy(&edu->thr_cond);
> + qemu_mutex_destroy(&edu->thr_mutex);
> + timer_del(&edu->dma_timer);
> + msi_uninit(pdev);
> + return;
> + }
>
> memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
> "edu-mmio", 1 * MiB);
In review of v9, I pointed out that pci_edu_uninit() neglects to call
msix_uninit(), and you offered to fix it. Not in this series. Do you
still intend to fix it?
That's a separate patch, though, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare Fei Li
@ 2019-02-01 13:01 ` Markus Armbruster
0 siblings, 0 replies; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 13:01 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, David Gibson
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Add a local_err to hold the error, and return the corresponding
> error code to replace the temporary &error_abort.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Fei Li <fli@suse.com>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize Fei Li
@ 2019-02-01 13:04 ` Markus Armbruster
2019-02-03 7:17 ` Fei Li
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 13:04 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Gerd Hoffmann
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Utilize the existed errp to propagate the error and do the
> corresponding cleanup to replace the temporary &error_abort.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
> hw/usb/ccid-card-emulated.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index 0b170f6328..19b4b9a8fa 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
> goto out2;
> }
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> - card, QEMU_THREAD_JOINABLE, &error_abort);
> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> - card, QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> + card, QEMU_THREAD_JOINABLE, errp) < 0) {
> + goto out2;
> + }
> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
> + handle_apdu_thread, card,
> + QEMU_THREAD_JOINABLE, errp) < 0) {
> + qemu_thread_join(&card->event_thread_id);
What makes event_thread terminate here?
I'm asking because if it doesn't, the join will hang.
> + goto out2;
> + }
>
> return;
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete Fei Li
@ 2019-02-01 14:03 ` Markus Armbruster
2019-02-02 4:51 ` fei
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 14:03 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Stefan Hajnoczi
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> For iothread_complete: utilize the existed errp to propagate the
"For iothread_complete" is redundant, isn't it?
> error and do the corresponding cleanup to replace the temporary
> &error_abort.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat Fei Li
@ 2019-02-01 14:13 ` Markus Armbruster
2019-02-02 4:52 ` fei
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 14:13 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <shirley17fei@gmail.com>
>
> For qemu_signalfd_compat: set errno, do some cleanup, and return
"For iothread_complete" is redundant, isn't it?
> -1 to replace the temporary &error_abort when failing to create
> sigwait_compat.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Fei Li <shirley17fei@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread Fei Li
@ 2019-02-01 14:16 ` Markus Armbruster
0 siblings, 0 replies; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 14:16 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Fei Li, Markus Armbruster,
Gerd Hoffmann
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Supplement the error handling for vnc_thread_worker_thread: add
> an Error parameter for it to propagate the error to its caller to
> handle in case it fails, and make it return a Boolean to indicate
> whether it succeeds.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration Fei Li
2019-02-01 5:35 ` Fei Li
@ 2019-02-01 15:34 ` Markus Armbruster
2019-02-03 7:55 ` Fei Li
2019-02-04 16:34 ` Dr. David Alan Gilbert
1 sibling, 2 replies; 34+ messages in thread
From: Markus Armbruster @ 2019-02-01 15:34 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei, Dr . David Alan Gilbert
Dave, I tried to review the error paths, in particular resource cleanup,
but there's a lot going on, and I'm not feeling confident. Please have
a close look.
Fei Li <lifei1214@126.com> writes:
> From: Fei Li <fli@suse.com>
>
> Update qemu_thread_create()'s callers by
> - setting an error on qemu_thread_create() failure for callers that
> set an error on failure;
> - reporting the error and returning failure for callers that return
> an error code on failure;
> - reporting the error and setting some state for callers that just
> report errors and choose not to continue on.
>
> Besides, make compress_threads_save_cleanup() cope with partially
> initialized comp_param[i] to adapt to the new qemu_thread_create()
> failure case.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 35 +++++++++++++-------
> migration/postcopy-ram.c | 16 ++++++---
> migration/ram.c | 70 ++++++++++++++++++++++++++--------------
> migration/savevm.c | 12 ++++---
> 4 files changed, 89 insertions(+), 44 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1da71211c8..0034ca1334 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
> goto fail;
> }
>
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> - colo_process_incoming_thread, mis,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> + colo_process_incoming_thread, mis,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err, "failed to create "
> + "colo_process_incoming_thread: ");
> + goto fail;
> + }
> mis->have_colo_incoming_thread = true;
> qemu_coroutine_yield();
>
> @@ -2349,6 +2352,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) {
> @@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
> return 0;
> }
>
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> - source_return_path_thread, ms,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> + source_return_path_thread, ms,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err,
> + "failed to create source_return_path_thread: ");
> + qemu_fclose(ms->rp_state.from_dst_file);
> + ms->rp_state.from_dst_file = NULL;
> + return -1;
> + }
>
> trace_open_return_path_on_source_continue();
>
> @@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
if (multifd_save_setup() != 0) {
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
> migrate_fd_cleanup(s);
> return;
> }
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> + QEMU_THREAD_JOINABLE, &error_in) < 0) {
> + error_reportf_err(error_in, "failed to create migration_thread: ");
> + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> + migrate_fd_cleanup(s);
Is there anything to clean up for multifd_save_setup()? Dave?
> + return;
> + }
> s->migration_thread_running = true;
> }
>
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 221ea24919..0934a1403a 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1083,6 +1083,8 @@ retry:
>
> int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> {
> + Error *local_err = NULL;
> +
> /* Open the fd for the kernel to give us userfaults */
> mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> if (mis->userfault_fd == -1) {
> @@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> }
>
> qemu_sem_init(&mis->fault_thread_sem, 0);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> - postcopy_ram_fault_thread, mis,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> + postcopy_ram_fault_thread, mis,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + 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;
/* 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");
return -1;
Not this patch's problem, but here goes anyway: where are
mis->userfault_event_fd and mis->userfault_fd closed?
}
> diff --git a/migration/ram.c b/migration/ram.c
> index 700ea229e0..66b8b764f1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
>
> thread_count = migrate_compress_threads();
> for (i = 0; i < thread_count; i++) {
> + qemu_mutex_lock(&comp_param[i].mutex);
> + comp_param[i].quit = true;
> + qemu_cond_signal(&comp_param[i].cond);
> + qemu_mutex_unlock(&comp_param[i].mutex);
> +
> + qemu_mutex_destroy(&comp_param[i].mutex);
> + qemu_cond_destroy(&comp_param[i].cond);
> +
> /*
> * we use it as a indicator which shows if the thread is
> * properly init'd or not
> @@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
> if (!comp_param[i].file) {
> break;
> }
> -
> - qemu_mutex_lock(&comp_param[i].mutex);
> - comp_param[i].quit = true;
> - qemu_cond_signal(&comp_param[i].cond);
> - qemu_mutex_unlock(&comp_param[i].mutex);
> -
> qemu_thread_join(compress_threads + i);
> - qemu_mutex_destroy(&comp_param[i].mutex);
> - qemu_cond_destroy(&comp_param[i].cond);
> deflateEnd(&comp_param[i].stream);
> g_free(comp_param[i].originbuf);
> qemu_fclose(comp_param[i].file);
> @@ -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;
> @@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
> qemu_cond_init(&comp_done_cond);
> qemu_mutex_init(&comp_done_lock);
> for (i = 0; i < thread_count; i++) {
> + qemu_mutex_init(&comp_param[i].mutex);
> + qemu_cond_init(&comp_param[i].cond);
> + comp_param[i].quit = false;
> comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
> if (!comp_param[i].originbuf) {
> goto exit;
> @@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
> */
> comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> comp_param[i].done = true;
> - comp_param[i].quit = false;
> - qemu_mutex_init(&comp_param[i].mutex);
> - qemu_cond_init(&comp_param[i].cond);
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(compress_threads + i, "compress",
> - do_data_compress, comp_param + i,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(compress_threads + i, "compress",
> + do_data_compress, comp_param + i,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_reportf_err(local_err, "failed to create do_data_compress: ");
> + deflateEnd(&comp_param[i].stream);
> + g_free(comp_param[i].originbuf);
> + qemu_fclose(comp_param[i].file);
> + comp_param[i].file = NULL;
> + goto exit;
> + }
> }
> return 0;
>
> @@ -1076,9 +1083,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;
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + migrate_set_error(migrate_get_current(), local_err);
> + error_reportf_err(local_err,
> + "failed to create multifd_send_thread: ");
> + multifd_save_cleanup();
> + return;
> + }
>
> atomic_inc(&multifd_send_state->count);
> }
> @@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
> p->num_packets = 1;
>
> p->running = true;
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + error_propagate_prepend(errp, local_err,
> + "failed to create multifd_recv_thread: ");
This prepends only if @errp isn't null. Fine from the caller's point of
view. But it also affects multifd_recv_thread():
> + multifd_recv_terminate_threads(local_err);
That's at least unclean.
I think you should use error_prepend() and error_propagate() here.
> + return false;
> + }
> atomic_inc(&multifd_recv_state->count);
> return atomic_read(&multifd_recv_state->count) ==
> migrate_multifd_channels();
> @@ -3631,6 +3647,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;
> @@ -3652,10 +3669,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;
> - /* TODO: let the further caller handle the error instead of abort() */
> - qemu_thread_create(decompress_threads + i, "decompress",
> - do_data_decompress, decomp_param + i,
> - QEMU_THREAD_JOINABLE, &error_abort);
> + if (qemu_thread_create(decompress_threads + i, "decompress",
> + do_data_decompress, decomp_param + i,
> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> + 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 d5b45843b6..310cecbf8f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1747,10 +1747,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);
> - /* TODO: let the further caller handle the error instead of abort() here */
> - qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> - postcopy_ram_listen_thread, NULL,
> - QEMU_THREAD_DETACHED, &error_abort);
> + if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> + postcopy_ram_listen_thread, NULL,
> + QEMU_THREAD_DETACHED, &local_err) < 0) {
> + 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);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize
2019-02-01 12:58 ` Markus Armbruster
@ 2019-02-02 4:44 ` fei
0 siblings, 0 replies; 34+ messages in thread
From: fei @ 2019-02-02 4:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei, Jiri Slaby
> 在 2019年2月1日，20:58，Markus Armbruster <armbru@redhat.com> 写道：
>
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <fli@suse.com>
>>
>> Utilize the existed errp to propagate the error and do the
>> corresponding cleanup to replace the temporary &error_abort.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Jiri Slaby <jslaby@suse.cz>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> hw/misc/edu.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>> index 21adbfddce..8fe232b6d6 100644
>> --- a/hw/misc/edu.c
>> +++ b/hw/misc/edu.c
>> @@ -356,9 +356,14 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
>>
>> qemu_mutex_init(&edu->thr_mutex);
>> qemu_cond_init(&edu->thr_cond);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
>> - edu, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
>> + edu, QEMU_THREAD_JOINABLE, errp) < 0) {
>> + qemu_cond_destroy(&edu->thr_cond);
>> + qemu_mutex_destroy(&edu->thr_mutex);
>> + timer_del(&edu->dma_timer);
>> + msi_uninit(pdev);
>> + return;
>> + }
>>
>> memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
>> "edu-mmio", 1 * MiB);
>
> In review of v9, I pointed out that pci_edu_uninit() neglects to call
> msix_uninit(), and you offered to fix it. Not in this series. Do you
> still intend to fix it?
Yes, I have already sent that patch separately as I consider it is little relevant. And that patch has been merged. :)
>
> That's a separate patch, though, so
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks for the review!
Have a nice day
Fei
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu
2019-02-01 12:33 ` Markus Armbruster
@ 2019-02-02 4:47 ` fei
0 siblings, 0 replies; 34+ messages in thread
From: fei @ 2019-02-02 4:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei, Fei Li, Paolo Bonzini
> 在 2019年2月1日，20:33，Markus Armbruster <armbru@redhat.com> 写道：
>
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <fli@suse.com>
>>
>> The callers of qemu_init_vcpu() already passed the **errp to handle
>> errors. In view of this, add a new Error parameter to qemu_init_vcpu()
>> and all qemu_X_start_vcpu() functions called by qemu_init_vcpu() to
>> propagate the error and let the further callers check it.
>>
>> Besides, make qemu_init_vcpu() return a Boolean value to let its
>> callers know whether it succeeds.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> ---
>> accel/tcg/user-exec-stub.c | 3 +-
>> cpus.c | 74 +++++++++++++++++++--------------
>> 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, 108 insertions(+), 55 deletions(-)
>>
>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>> index a32b4496af..f8c38a375c 100644
>> --- a/accel/tcg/user-exec-stub.c
>> +++ b/accel/tcg/user-exec-stub.c
>> @@ -10,8 +10,9 @@ void cpu_resume(CPUState *cpu)
>> {
>> }
>>
>> -void qemu_init_vcpu(CPUState *cpu)
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>> {
>> + return true;
>> }
>>
>> /* User mode emulation does not support record/replay yet. */
>> diff --git a/cpus.c b/cpus.c
>> index 843a0f06a2..4ed7d62e58 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1931,7 +1931,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;
>> @@ -1961,17 +1961,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
>> cpu->cpu_index);
>>
>> - /* TODO: let the callers handle the error instead of abort() here */
>> - qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(cpu->thread, thread_name,
>> + qemu_tcg_cpu_thread_fn, cpu,
>> + QEMU_THREAD_JOINABLE, errp) < 0) {
>> + return;
>> + }
>>
>> } else {
>> /* share a single thread for all cpus with TCG */
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
>> - /* TODO: let the callers handle the error instead of abort() here */
>> - qemu_thread_create(cpu->thread, thread_name,
>> - qemu_tcg_rr_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(cpu->thread, thread_name,
>> + qemu_tcg_rr_cpu_thread_fn, cpu,
>> + QEMU_THREAD_JOINABLE, errp) < 0) {
>> + return;
>> + }
>>
>> single_tcg_halt_cond = cpu->halt_cond;
>> single_tcg_cpu_thread = cpu->thread;
>> @@ -1989,7 +1992,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];
>>
>> @@ -1999,15 +2002,16 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
>>
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
>> cpu->cpu_index);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
>> + cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
>> + return;
>> + }
>> #ifdef _WIN32
>> cpu->hThread = qemu_thread_get_handle(cpu->thread);
>> #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];
>>
>> @@ -2016,12 +2020,11 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
>> qemu_cond_init(cpu->halt_cond);
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
>> cpu->cpu_index);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>>
>> -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];
>>
>> @@ -2035,12 +2038,11 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
>>
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
>> cpu->cpu_index);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>>
>> -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];
>>
>> @@ -2049,15 +2051,16 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
>> qemu_cond_init(cpu->halt_cond);
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
>> cpu->cpu_index);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
>> + cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
>> + return;
>> + }
>> #ifdef _WIN32
>> cpu->hThread = qemu_thread_get_handle(cpu->thread);
>> #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];
>>
>> @@ -2066,16 +2069,16 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>> qemu_cond_init(cpu->halt_cond);
>> snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
>> cpu->cpu_index);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
>> - cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> + cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>>
>> -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,
>> @@ -2086,22 +2089,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)
>
> If the qemu_FOO_init_vcpu() returned success / failure like their callee
> qemu_thread_create() and their caller qemu_init_vcpu() do, then this
> code would be simpler.
>
> But it's not wrong, and we're at v11, so
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> [...]
Ok, and thanks for the review.
Have a nice day
Fei
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete
2019-02-01 14:03 ` Markus Armbruster
@ 2019-02-02 4:51 ` fei
0 siblings, 0 replies; 34+ messages in thread
From: fei @ 2019-02-02 4:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei, Stefan Hajnoczi
> 在 2019年2月1日，22:03，Markus Armbruster <armbru@redhat.com> 写道：
>
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <fli@suse.com>
>>
>> For iothread_complete: utilize the existed errp to propagate the
>
> "For iothread_complete" is redundant, isn't it?
Emm, right. Will remove it in the next version.
>
>> error and do the corresponding cleanup to replace the temporary
>> &error_abort.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks!
Have a nice day
Fei
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat
2019-02-01 14:13 ` Markus Armbruster
@ 2019-02-02 4:52 ` fei
0 siblings, 0 replies; 34+ messages in thread
From: fei @ 2019-02-02 4:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei
> 在 2019年2月1日，22:13，Markus Armbruster <armbru@redhat.com> 写道：
>
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <shirley17fei@gmail.com>
>>
>> For qemu_signalfd_compat: set errno, do some cleanup, and return
>
> "For iothread_complete" is redundant, isn't it?
Yep, will remove it. Thanks for pointing this out. :)
>
>> -1 to replace the temporary &error_abort when failing to create
>> sigwait_compat.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Fei Li <shirley17fei@gmail.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Have a nice day
Fei
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize
2019-02-01 13:04 ` Markus Armbruster
@ 2019-02-03 7:17 ` Fei Li
2019-02-04 13:30 ` Markus Armbruster
0 siblings, 1 reply; 34+ messages in thread
From: Fei Li @ 2019-02-03 7:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei, Gerd Hoffmann
在 2019/2/1 下午9:04, Markus Armbruster 写道:
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <fli@suse.com>
>>
>> Utilize the existed errp to propagate the error and do the
>> corresponding cleanup to replace the temporary &error_abort.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> hw/usb/ccid-card-emulated.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
>> index 0b170f6328..19b4b9a8fa 100644
>> --- a/hw/usb/ccid-card-emulated.c
>> +++ b/hw/usb/ccid-card-emulated.c
>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
>> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
>> goto out2;
>> }
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>> + card, QEMU_THREAD_JOINABLE, errp) < 0) {
>> + goto out2;
>> + }
>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
>> + handle_apdu_thread, card,
>> + QEMU_THREAD_JOINABLE, errp) < 0) {
>> + qemu_thread_join(&card->event_thread_id);
> What makes event_thread terminate here?
>
> I'm asking because if it doesn't, the join will hang.
Oops, my neglect.. Could we add a
`qemu_thread_cancel(card->event_thread_id);` here
before the join()?
Have a nice day, thanks
Fei
>
>> + goto out2;
>> + }
>>
>> return;
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-01 15:34 ` Markus Armbruster
@ 2019-02-03 7:55 ` Fei Li
2019-02-04 16:34 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 34+ messages in thread
From: Fei Li @ 2019-02-03 7:55 UTC (permalink / raw)
To: Markus Armbruster, Fei Li; +Cc: qemu-devel, Dr . David Alan Gilbert
在 2019/2/1 下午11:34, Markus Armbruster 写道:
> Dave, I tried to review the error paths, in particular resource cleanup,
> but there's a lot going on, and I'm not feeling confident. Please have
> a close look.
>
> Fei Li <lifei1214@126.com> writes:
>
>> From: Fei Li <fli@suse.com>
>>
>> Update qemu_thread_create()'s callers by
>> - setting an error on qemu_thread_create() failure for callers that
>> set an error on failure;
>> - reporting the error and returning failure for callers that return
>> an error code on failure;
>> - reporting the error and setting some state for callers that just
>> report errors and choose not to continue on.
>>
>> Besides, make compress_threads_save_cleanup() cope with partially
>> initialized comp_param[i] to adapt to the new qemu_thread_create()
>> failure case.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>> migration/migration.c | 35 +++++++++++++-------
>> migration/postcopy-ram.c | 16 ++++++---
>> migration/ram.c | 70 ++++++++++++++++++++++++++--------------
>> migration/savevm.c | 12 ++++---
>> 4 files changed, 89 insertions(+), 44 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1da71211c8..0034ca1334 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
>> goto fail;
>> }
>>
>> - /* TODO: let the further caller handle the error instead of abort() */
>> - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
>> - colo_process_incoming_thread, mis,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
>> + colo_process_incoming_thread, mis,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + error_reportf_err(local_err, "failed to create "
>> + "colo_process_incoming_thread: ");
>> + goto fail;
>> + }
>> mis->have_colo_incoming_thread = true;
>> qemu_coroutine_yield();
>>
>> @@ -2349,6 +2352,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) {
>> @@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
>> return 0;
>> }
>>
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>> - source_return_path_thread, ms,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>> + source_return_path_thread, ms,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + error_reportf_err(local_err,
>> + "failed to create source_return_path_thread: ");
>> + qemu_fclose(ms->rp_state.from_dst_file);
>> + ms->rp_state.from_dst_file = NULL;
>> + return -1;
>> + }
>>
>> trace_open_return_path_on_source_continue();
>>
>> @@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> if (multifd_save_setup() != 0) {
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_FAILED);
>> migrate_fd_cleanup(s);
>> return;
>> }
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>> + QEMU_THREAD_JOINABLE, &error_in) < 0) {
>> + error_reportf_err(error_in, "failed to create migration_thread: ");
>> + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> + migrate_fd_cleanup(s);
> Is there anything to clean up for multifd_save_setup()? Dave?
>
>> + return;
>> + }
>> s->migration_thread_running = true;
>> }
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 221ea24919..0934a1403a 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -1083,6 +1083,8 @@ retry:
>>
>> int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>> {
>> + Error *local_err = NULL;
>> +
>> /* Open the fd for the kernel to give us userfaults */
>> mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
>> if (mis->userfault_fd == -1) {
>> @@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>> }
>>
>> qemu_sem_init(&mis->fault_thread_sem, 0);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&mis->fault_thread, "postcopy/fault",
>> - postcopy_ram_fault_thread, mis,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
>> + postcopy_ram_fault_thread, mis,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + 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;
> /* 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");
> return -1;
>
> Not this patch's problem, but here goes anyway: where are
> mis->userfault_event_fd and mis->userfault_fd closed?
They are actually cleaned in the caller when
postcopy_ram_enable_notify() fails:
call postcopy_ram_incoming_cleanup() to do the cleanup when
mis->have_fault_thread=true
The merged patch: 91b02dc is to fix this.
>
> }
>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 700ea229e0..66b8b764f1 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
>>
>> thread_count = migrate_compress_threads();
>> for (i = 0; i < thread_count; i++) {
>> + qemu_mutex_lock(&comp_param[i].mutex);
>> + comp_param[i].quit = true;
>> + qemu_cond_signal(&comp_param[i].cond);
>> + qemu_mutex_unlock(&comp_param[i].mutex);
>> +
>> + qemu_mutex_destroy(&comp_param[i].mutex);
>> + qemu_cond_destroy(&comp_param[i].cond);
>> +
>> /*
>> * we use it as a indicator which shows if the thread is
>> * properly init'd or not
>> @@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
>> if (!comp_param[i].file) {
>> break;
>> }
>> -
>> - qemu_mutex_lock(&comp_param[i].mutex);
>> - comp_param[i].quit = true;
>> - qemu_cond_signal(&comp_param[i].cond);
>> - qemu_mutex_unlock(&comp_param[i].mutex);
>> -
>> qemu_thread_join(compress_threads + i);
>> - qemu_mutex_destroy(&comp_param[i].mutex);
>> - qemu_cond_destroy(&comp_param[i].cond);
>> deflateEnd(&comp_param[i].stream);
>> g_free(comp_param[i].originbuf);
>> qemu_fclose(comp_param[i].file);
>> @@ -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;
>> @@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
>> qemu_cond_init(&comp_done_cond);
>> qemu_mutex_init(&comp_done_lock);
>> for (i = 0; i < thread_count; i++) {
>> + qemu_mutex_init(&comp_param[i].mutex);
>> + qemu_cond_init(&comp_param[i].cond);
>> + comp_param[i].quit = false;
>> comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
>> if (!comp_param[i].originbuf) {
>> goto exit;
>> @@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
>> */
>> comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
>> comp_param[i].done = true;
>> - comp_param[i].quit = false;
>> - qemu_mutex_init(&comp_param[i].mutex);
>> - qemu_cond_init(&comp_param[i].cond);
>> - /* TODO: let the further caller handle the error instead of abort() */
>> - qemu_thread_create(compress_threads + i, "compress",
>> - do_data_compress, comp_param + i,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(compress_threads + i, "compress",
>> + do_data_compress, comp_param + i,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + error_reportf_err(local_err, "failed to create do_data_compress: ");
>> + deflateEnd(&comp_param[i].stream);
>> + g_free(comp_param[i].originbuf);
>> + qemu_fclose(comp_param[i].file);
>> + comp_param[i].file = NULL;
>> + goto exit;
>> + }
>> }
>> return 0;
>>
>> @@ -1076,9 +1083,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;
>> - /* TODO: let the further caller handle the error instead of abort() */
>> - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + migrate_set_error(migrate_get_current(), local_err);
>> + error_reportf_err(local_err,
>> + "failed to create multifd_send_thread: ");
>> + multifd_save_cleanup();
>> + return;
>> + }
>>
>> atomic_inc(&multifd_send_state->count);
>> }
>> @@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>> p->num_packets = 1;
>>
>> p->running = true;
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + error_propagate_prepend(errp, local_err,
>> + "failed to create multifd_recv_thread: ");
> This prepends only if @errp isn't null. Fine from the caller's point of
> view. But it also affects multifd_recv_thread():
>
>> + multifd_recv_terminate_threads(local_err);
> That's at least unclean.
>
> I think you should use error_prepend() and error_propagate() here.
Right, I will change these two sentences' order. Thanks for pointing
this out. :)
Have a nice day
Fei
>
>> + return false;
>> + }
>> atomic_inc(&multifd_recv_state->count);
>> return atomic_read(&multifd_recv_state->count) ==
>> migrate_multifd_channels();
>> @@ -3631,6 +3647,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;
>> @@ -3652,10 +3669,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;
>> - /* TODO: let the further caller handle the error instead of abort() */
>> - qemu_thread_create(decompress_threads + i, "decompress",
>> - do_data_decompress, decomp_param + i,
>> - QEMU_THREAD_JOINABLE, &error_abort);
>> + if (qemu_thread_create(decompress_threads + i, "decompress",
>> + do_data_decompress, decomp_param + i,
>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>> + 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 d5b45843b6..310cecbf8f 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1747,10 +1747,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);
>> - /* TODO: let the further caller handle the error instead of abort() here */
>> - qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>> - postcopy_ram_listen_thread, NULL,
>> - QEMU_THREAD_DETACHED, &error_abort);
>> + if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>> + postcopy_ram_listen_thread, NULL,
>> + QEMU_THREAD_DETACHED, &local_err) < 0) {
>> + 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);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize
2019-02-03 7:17 ` Fei Li
@ 2019-02-04 13:30 ` Markus Armbruster
2019-02-15 12:35 ` Fei Li
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-02-04 13:30 UTC (permalink / raw)
To: Fei Li
Cc: Gerd Hoffmann, qemu-devel, shirley17fei, Christophe Fergeau,
Marc-André Lureau
Fei Li <lifei1214@126.com> writes:
> 在 2019/2/1 下午9:04, Markus Armbruster 写道:
>> Fei Li <lifei1214@126.com> writes:
>>
>>> From: Fei Li <fli@suse.com>
>>>
>>> Utilize the existed errp to propagate the error and do the
>>> corresponding cleanup to replace the temporary &error_abort.
>>>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Signed-off-by: Fei Li <fli@suse.com>
>>> ---
>>> hw/usb/ccid-card-emulated.c | 15 ++++++++++-----
>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
>>> index 0b170f6328..19b4b9a8fa 100644
>>> --- a/hw/usb/ccid-card-emulated.c
>>> +++ b/hw/usb/ccid-card-emulated.c
>>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
>>> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
>>> goto out2;
>>> }
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
>>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>>> + card, QEMU_THREAD_JOINABLE, errp) < 0) {
>>> + goto out2;
>>> + }
>>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
>>> + handle_apdu_thread, card,
>>> + QEMU_THREAD_JOINABLE, errp) < 0) {
>>> + qemu_thread_join(&card->event_thread_id);
>> What makes event_thread terminate here?
>>
>> I'm asking because if it doesn't, the join will hang.
> Oops, my neglect.. Could we add a
> `qemu_thread_cancel(card->event_thread_id);` here
> before the join()?
pthread_cancel() is difficult to use correctly, and we don't use it in
QEMU so far. Instead, we tell threads to stop, e.g. by setting a flag
the thread checks in its main loop, and making sure the thread actually
loops in bounded time. How to best achieve that for this thread I don't
know. Christophe, Marc-André, can you help?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-01 15:34 ` Markus Armbruster
2019-02-03 7:55 ` Fei Li
@ 2019-02-04 16:34 ` Dr. David Alan Gilbert
2019-02-15 13:23 ` Fei Li
1 sibling, 1 reply; 34+ messages in thread
From: Dr. David Alan Gilbert @ 2019-02-04 16:34 UTC (permalink / raw)
To: Markus Armbruster, quintela; +Cc: Fei Li, qemu-devel, shirley17fei
* Markus Armbruster (armbru@redhat.com) wrote:
> Dave, I tried to review the error paths, in particular resource cleanup,
> but there's a lot going on, and I'm not feeling confident. Please have
> a close look.
>
> Fei Li <lifei1214@126.com> writes:
>
> > From: Fei Li <fli@suse.com>
> >
> > Update qemu_thread_create()'s callers by
> > - setting an error on qemu_thread_create() failure for callers that
> > set an error on failure;
> > - reporting the error and returning failure for callers that return
> > an error code on failure;
> > - reporting the error and setting some state for callers that just
> > report errors and choose not to continue on.
> >
> > Besides, make compress_threads_save_cleanup() cope with partially
> > initialized comp_param[i] to adapt to the new qemu_thread_create()
> > failure case.
> >
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Fei Li <fli@suse.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > migration/migration.c | 35 +++++++++++++-------
> > migration/postcopy-ram.c | 16 ++++++---
> > migration/ram.c | 70 ++++++++++++++++++++++++++--------------
> > migration/savevm.c | 12 ++++---
> > 4 files changed, 89 insertions(+), 44 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 1da71211c8..0034ca1334 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
> > goto fail;
> > }
> >
> > - /* TODO: let the further caller handle the error instead of abort() */
> > - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> > - colo_process_incoming_thread, mis,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> > + colo_process_incoming_thread, mis,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + error_reportf_err(local_err, "failed to create "
> > + "colo_process_incoming_thread: ");
> > + goto fail;
> > + }
> > mis->have_colo_incoming_thread = true;
> > qemu_coroutine_yield();
> >
> > @@ -2349,6 +2352,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) {
> > @@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
> > return 0;
> > }
> >
> > - /* TODO: let the further caller handle the error instead of abort() here */
> > - qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> > - source_return_path_thread, ms,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> > + source_return_path_thread, ms,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + error_reportf_err(local_err,
> > + "failed to create source_return_path_thread: ");
> > + qemu_fclose(ms->rp_state.from_dst_file);
> > + ms->rp_state.from_dst_file = NULL;
> > + return -1;
> > + }
> >
> > trace_open_return_path_on_source_continue();
> >
> > @@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> if (multifd_save_setup() != 0) {
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_FAILED);
> > migrate_fd_cleanup(s);
> > return;
> > }
> > - /* TODO: let the further caller handle the error instead of abort() here */
> > - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> > + QEMU_THREAD_JOINABLE, &error_in) < 0) {
> > + error_reportf_err(error_in, "failed to create migration_thread: ");
> > + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> > + migrate_fd_cleanup(s);
>
> Is there anything to clean up for multifd_save_setup()? Dave?
I need to bounce that one to Juan; he knows the multifd stuff; cc'd
> > + return;
> > + }
> > s->migration_thread_running = true;
> > }
> >
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 221ea24919..0934a1403a 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -1083,6 +1083,8 @@ retry:
> >
> > int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> > {
> > + Error *local_err = NULL;
> > +
> > /* Open the fd for the kernel to give us userfaults */
> > mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > if (mis->userfault_fd == -1) {
> > @@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> > }
> >
> > qemu_sem_init(&mis->fault_thread_sem, 0);
> > - /* TODO: let the further caller handle the error instead of abort() here */
> > - qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> > - postcopy_ram_fault_thread, mis,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
> > + postcopy_ram_fault_thread, mis,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + 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;
>
> /* 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");
> return -1;
>
> Not this patch's problem, but here goes anyway: where are
> mis->userfault_event_fd and mis->userfault_fd closed?
As Fei replied, see postcopy_ram_incoming_cleanup; once the fault thread
is running it does a bigger cleanup.
Dave
> }
>
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 700ea229e0..66b8b764f1 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
> >
> > thread_count = migrate_compress_threads();
> > for (i = 0; i < thread_count; i++) {
> > + qemu_mutex_lock(&comp_param[i].mutex);
> > + comp_param[i].quit = true;
> > + qemu_cond_signal(&comp_param[i].cond);
> > + qemu_mutex_unlock(&comp_param[i].mutex);
> > +
> > + qemu_mutex_destroy(&comp_param[i].mutex);
> > + qemu_cond_destroy(&comp_param[i].cond);
> > +
> > /*
> > * we use it as a indicator which shows if the thread is
> > * properly init'd or not
> > @@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
> > if (!comp_param[i].file) {
> > break;
> > }
> > -
> > - qemu_mutex_lock(&comp_param[i].mutex);
> > - comp_param[i].quit = true;
> > - qemu_cond_signal(&comp_param[i].cond);
> > - qemu_mutex_unlock(&comp_param[i].mutex);
> > -
> > qemu_thread_join(compress_threads + i);
> > - qemu_mutex_destroy(&comp_param[i].mutex);
> > - qemu_cond_destroy(&comp_param[i].cond);
> > deflateEnd(&comp_param[i].stream);
> > g_free(comp_param[i].originbuf);
> > qemu_fclose(comp_param[i].file);
> > @@ -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;
> > @@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
> > qemu_cond_init(&comp_done_cond);
> > qemu_mutex_init(&comp_done_lock);
> > for (i = 0; i < thread_count; i++) {
> > + qemu_mutex_init(&comp_param[i].mutex);
> > + qemu_cond_init(&comp_param[i].cond);
> > + comp_param[i].quit = false;
> > comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
> > if (!comp_param[i].originbuf) {
> > goto exit;
> > @@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
> > */
> > comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> > comp_param[i].done = true;
> > - comp_param[i].quit = false;
> > - qemu_mutex_init(&comp_param[i].mutex);
> > - qemu_cond_init(&comp_param[i].cond);
> > - /* TODO: let the further caller handle the error instead of abort() */
> > - qemu_thread_create(compress_threads + i, "compress",
> > - do_data_compress, comp_param + i,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(compress_threads + i, "compress",
> > + do_data_compress, comp_param + i,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + error_reportf_err(local_err, "failed to create do_data_compress: ");
> > + deflateEnd(&comp_param[i].stream);
> > + g_free(comp_param[i].originbuf);
> > + qemu_fclose(comp_param[i].file);
> > + comp_param[i].file = NULL;
> > + goto exit;
> > + }
> > }
> > return 0;
> >
> > @@ -1076,9 +1083,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;
> > - /* TODO: let the further caller handle the error instead of abort() */
> > - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + migrate_set_error(migrate_get_current(), local_err);
> > + error_reportf_err(local_err,
> > + "failed to create multifd_send_thread: ");
> > + multifd_save_cleanup();
> > + return;
> > + }
> >
> > atomic_inc(&multifd_send_state->count);
> > }
> > @@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
> > p->num_packets = 1;
> >
> > p->running = true;
> > - /* TODO: let the further caller handle the error instead of abort() here */
> > - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + error_propagate_prepend(errp, local_err,
> > + "failed to create multifd_recv_thread: ");
>
> This prepends only if @errp isn't null. Fine from the caller's point of
> view. But it also affects multifd_recv_thread():
>
> > + multifd_recv_terminate_threads(local_err);
>
> That's at least unclean.
>
> I think you should use error_prepend() and error_propagate() here.
>
> > + return false;
> > + }
> > atomic_inc(&multifd_recv_state->count);
> > return atomic_read(&multifd_recv_state->count) ==
> > migrate_multifd_channels();
> > @@ -3631,6 +3647,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;
> > @@ -3652,10 +3669,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;
> > - /* TODO: let the further caller handle the error instead of abort() */
> > - qemu_thread_create(decompress_threads + i, "decompress",
> > - do_data_decompress, decomp_param + i,
> > - QEMU_THREAD_JOINABLE, &error_abort);
> > + if (qemu_thread_create(decompress_threads + i, "decompress",
> > + do_data_decompress, decomp_param + i,
> > + QEMU_THREAD_JOINABLE, &local_err) < 0) {
> > + 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 d5b45843b6..310cecbf8f 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1747,10 +1747,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);
> > - /* TODO: let the further caller handle the error instead of abort() here */
> > - qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> > - postcopy_ram_listen_thread, NULL,
> > - QEMU_THREAD_DETACHED, &error_abort);
> > + if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> > + postcopy_ram_listen_thread, NULL,
> > + QEMU_THREAD_DETACHED, &local_err) < 0) {
> > + 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);
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize
2019-02-04 13:30 ` Markus Armbruster
@ 2019-02-15 12:35 ` Fei Li
0 siblings, 0 replies; 34+ messages in thread
From: Fei Li @ 2019-02-15 12:35 UTC (permalink / raw)
To: Christophe Fergeau, Marc-André Lureau
Cc: Markus Armbruster, Gerd Hoffmann, qemu-devel, shirley17fei
在 2019/2/4 下午9:30, Markus Armbruster 写道:
> Fei Li <lifei1214@126.com> writes:
>
>> 在 2019/2/1 下午9:04, Markus Armbruster 写道:
>>> Fei Li <lifei1214@126.com> writes:
>>>
>>>> From: Fei Li <fli@suse.com>
>>>>
>>>> Utilize the existed errp to propagate the error and do the
>>>> corresponding cleanup to replace the temporary &error_abort.
>>>>
>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>> ---
>>>> hw/usb/ccid-card-emulated.c | 15 ++++++++++-----
>>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
>>>> index 0b170f6328..19b4b9a8fa 100644
>>>> --- a/hw/usb/ccid-card-emulated.c
>>>> +++ b/hw/usb/ccid-card-emulated.c
>>>> @@ -544,11 +544,16 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
>>>> error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
>>>> goto out2;
>>>> }
>>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>>> - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>>>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>>>> - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
>>>> - card, QEMU_THREAD_JOINABLE, &error_abort);
>>>> + if (qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
>>>> + card, QEMU_THREAD_JOINABLE, errp) < 0) {
>>>> + goto out2;
>>>> + }
>>>> + if (qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
>>>> + handle_apdu_thread, card,
>>>> + QEMU_THREAD_JOINABLE, errp) < 0) {
>>>> + qemu_thread_join(&card->event_thread_id);
>>> What makes event_thread terminate here?
>>>
>>> I'm asking because if it doesn't, the join will hang.
>> Oops, my neglect.. Could we add a
>> `qemu_thread_cancel(card->event_thread_id);` here
>> before the join()?
> pthread_cancel() is difficult to use correctly, and we don't use it in
> QEMU so far. Instead, we tell threads to stop, e.g. by setting a flag
> the thread checks in its main loop, and making sure the thread actually
> loops in bounded time. How to best achieve that for this thread I don't
> know. Christophe, Marc-André, can you help?
Hi, Christophe, Marc-André,
Would you like to share your views and give some suggestions? :)
That would be very helpful, thanks a lot!
Have a nice day
Fei
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration
2019-02-04 16:34 ` Dr. David Alan Gilbert
@ 2019-02-15 13:23 ` Fei Li
0 siblings, 0 replies; 34+ messages in thread
From: Fei Li @ 2019-02-15 13:23 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Markus Armbruster, quintela, Peter Xu
Cc: qemu-devel, shirley17fei
在 2019/2/5 上午12:34, Dr. David Alan Gilbert 写道:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Dave, I tried to review the error paths, in particular resource cleanup,
>> but there's a lot going on, and I'm not feeling confident. Please have
>> a close look.
>>
>> Fei Li <lifei1214@126.com> writes:
>>
>>> From: Fei Li <fli@suse.com>
>>>
>>> Update qemu_thread_create()'s callers by
>>> - setting an error on qemu_thread_create() failure for callers that
>>> set an error on failure;
>>> - reporting the error and returning failure for callers that return
>>> an error code on failure;
>>> - reporting the error and setting some state for callers that just
>>> report errors and choose not to continue on.
>>>
>>> Besides, make compress_threads_save_cleanup() cope with partially
>>> initialized comp_param[i] to adapt to the new qemu_thread_create()
>>> failure case.
>>>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Signed-off-by: Fei Li <fli@suse.com>
>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>> migration/migration.c | 35 +++++++++++++-------
>>> migration/postcopy-ram.c | 16 ++++++---
>>> migration/ram.c | 70 ++++++++++++++++++++++++++--------------
>>> migration/savevm.c | 12 ++++---
>>> 4 files changed, 89 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 1da71211c8..0034ca1334 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -447,10 +447,13 @@ static void process_incoming_migration_co(void *opaque)
>>> goto fail;
>>> }
>>>
>>> - /* TODO: let the further caller handle the error instead of abort() */
>>> - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
>>> - colo_process_incoming_thread, mis,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
>>> + colo_process_incoming_thread, mis,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + error_reportf_err(local_err, "failed to create "
>>> + "colo_process_incoming_thread: ");
>>> + goto fail;
>>> + }
>>> mis->have_colo_incoming_thread = true;
>>> qemu_coroutine_yield();
>>>
>>> @@ -2349,6 +2352,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) {
>>> @@ -2362,10 +2366,15 @@ static int open_return_path_on_source(MigrationState *ms,
>>> return 0;
>>> }
>>>
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>>> - source_return_path_thread, ms,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>>> + source_return_path_thread, ms,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + error_reportf_err(local_err,
>>> + "failed to create source_return_path_thread: ");
>>> + qemu_fclose(ms->rp_state.from_dst_file);
>>> + ms->rp_state.from_dst_file = NULL;
>>> + return -1;
>>> + }
>>>
>>> trace_open_return_path_on_source_continue();
>>>
>>> @@ -3201,9 +3210,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> if (multifd_save_setup() != 0) {
>> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> MIGRATION_STATUS_FAILED);
>>> migrate_fd_cleanup(s);
>>> return;
>>> }
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>>> + QEMU_THREAD_JOINABLE, &error_in) < 0) {
>>> + error_reportf_err(error_in, "failed to create migration_thread: ");
>>> + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>>> + migrate_fd_cleanup(s);
>> Is there anything to clean up for multifd_save_setup()? Dave?
> I need to bounce that one to Juan; he knows the multifd stuff; cc'd
Hi Juan, Peter,
I think this may involve the sequential order between migration_thread() and
multifd_save_setup() we talked earlier :
https://www.mail-archive.com/qemu-devel@nongnu.org/msg571178.html
To be specific, the current code can not tell whether all channels have
been sent via multifd_new_send_channel_async() before/during/after the
migration_thread() runs.
What do you think?
Have a nice day, thanks :)
Fei
>
>>> + return;
>>> + }
>>> s->migration_thread_running = true;
>>> }
>>>
>>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>>> index 221ea24919..0934a1403a 100644
>>> --- a/migration/postcopy-ram.c
>>> +++ b/migration/postcopy-ram.c
>>> @@ -1083,6 +1083,8 @@ retry:
>>>
>>> int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>>> {
>>> + Error *local_err = NULL;
>>> +
>>> /* Open the fd for the kernel to give us userfaults */
>>> mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
>>> if (mis->userfault_fd == -1) {
>>> @@ -1109,10 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>>> }
>>>
>>> qemu_sem_init(&mis->fault_thread_sem, 0);
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&mis->fault_thread, "postcopy/fault",
>>> - postcopy_ram_fault_thread, mis,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&mis->fault_thread, "postcopy/fault",
>>> + postcopy_ram_fault_thread, mis,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + 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;
>> /* 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");
>> return -1;
>>
>> Not this patch's problem, but here goes anyway: where are
>> mis->userfault_event_fd and mis->userfault_fd closed?
> As Fei replied, see postcopy_ram_incoming_cleanup; once the fault thread
> is running it does a bigger cleanup.
>
> Dave
>
>> }
>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 700ea229e0..66b8b764f1 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -441,6 +441,14 @@ static void compress_threads_save_cleanup(void)
>>>
>>> thread_count = migrate_compress_threads();
>>> for (i = 0; i < thread_count; i++) {
>>> + qemu_mutex_lock(&comp_param[i].mutex);
>>> + comp_param[i].quit = true;
>>> + qemu_cond_signal(&comp_param[i].cond);
>>> + qemu_mutex_unlock(&comp_param[i].mutex);
>>> +
>>> + qemu_mutex_destroy(&comp_param[i].mutex);
>>> + qemu_cond_destroy(&comp_param[i].cond);
>>> +
>>> /*
>>> * we use it as a indicator which shows if the thread is
>>> * properly init'd or not
>>> @@ -448,15 +456,7 @@ static void compress_threads_save_cleanup(void)
>>> if (!comp_param[i].file) {
>>> break;
>>> }
>>> -
>>> - qemu_mutex_lock(&comp_param[i].mutex);
>>> - comp_param[i].quit = true;
>>> - qemu_cond_signal(&comp_param[i].cond);
>>> - qemu_mutex_unlock(&comp_param[i].mutex);
>>> -
>>> qemu_thread_join(compress_threads + i);
>>> - qemu_mutex_destroy(&comp_param[i].mutex);
>>> - qemu_cond_destroy(&comp_param[i].cond);
>>> deflateEnd(&comp_param[i].stream);
>>> g_free(comp_param[i].originbuf);
>>> qemu_fclose(comp_param[i].file);
>>> @@ -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;
>>> @@ -483,6 +484,9 @@ static int compress_threads_save_setup(void)
>>> qemu_cond_init(&comp_done_cond);
>>> qemu_mutex_init(&comp_done_lock);
>>> for (i = 0; i < thread_count; i++) {
>>> + qemu_mutex_init(&comp_param[i].mutex);
>>> + qemu_cond_init(&comp_param[i].cond);
>>> + comp_param[i].quit = false;
>>> comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
>>> if (!comp_param[i].originbuf) {
>>> goto exit;
>>> @@ -499,13 +503,16 @@ static int compress_threads_save_setup(void)
>>> */
>>> comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
>>> comp_param[i].done = true;
>>> - comp_param[i].quit = false;
>>> - qemu_mutex_init(&comp_param[i].mutex);
>>> - qemu_cond_init(&comp_param[i].cond);
>>> - /* TODO: let the further caller handle the error instead of abort() */
>>> - qemu_thread_create(compress_threads + i, "compress",
>>> - do_data_compress, comp_param + i,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(compress_threads + i, "compress",
>>> + do_data_compress, comp_param + i,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + error_reportf_err(local_err, "failed to create do_data_compress: ");
>>> + deflateEnd(&comp_param[i].stream);
>>> + g_free(comp_param[i].originbuf);
>>> + qemu_fclose(comp_param[i].file);
>>> + comp_param[i].file = NULL;
>>> + goto exit;
>>> + }
>>> }
>>> return 0;
>>>
>>> @@ -1076,9 +1083,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;
>>> - /* TODO: let the further caller handle the error instead of abort() */
>>> - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + migrate_set_error(migrate_get_current(), local_err);
>>> + error_reportf_err(local_err,
>>> + "failed to create multifd_send_thread: ");
>>> + multifd_save_cleanup();
>>> + return;
>>> + }
>>>
>>> atomic_inc(&multifd_send_state->count);
>>> }
>>> @@ -1357,9 +1369,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>>> p->num_packets = 1;
>>>
>>> p->running = true;
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + error_propagate_prepend(errp, local_err,
>>> + "failed to create multifd_recv_thread: ");
>> This prepends only if @errp isn't null. Fine from the caller's point of
>> view. But it also affects multifd_recv_thread():
>>
>>> + multifd_recv_terminate_threads(local_err);
>> That's at least unclean.
>>
>> I think you should use error_prepend() and error_propagate() here.
>>
>>> + return false;
>>> + }
>>> atomic_inc(&multifd_recv_state->count);
>>> return atomic_read(&multifd_recv_state->count) ==
>>> migrate_multifd_channels();
>>> @@ -3631,6 +3647,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;
>>> @@ -3652,10 +3669,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;
>>> - /* TODO: let the further caller handle the error instead of abort() */
>>> - qemu_thread_create(decompress_threads + i, "decompress",
>>> - do_data_decompress, decomp_param + i,
>>> - QEMU_THREAD_JOINABLE, &error_abort);
>>> + if (qemu_thread_create(decompress_threads + i, "decompress",
>>> + do_data_decompress, decomp_param + i,
>>> + QEMU_THREAD_JOINABLE, &local_err) < 0) {
>>> + 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 d5b45843b6..310cecbf8f 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -1747,10 +1747,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);
>>> - /* TODO: let the further caller handle the error instead of abort() here */
>>> - qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>>> - postcopy_ram_listen_thread, NULL,
>>> - QEMU_THREAD_DETACHED, &error_abort);
>>> + if (qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>>> + postcopy_ram_listen_thread, NULL,
>>> + QEMU_THREAD_DETACHED, &local_err) < 0) {
>>> + 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);
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
` (9 preceding siblings ...)
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread Fei Li
@ 2019-07-12 5:57 ` Markus Armbruster
2019-07-16 12:25 ` fei
10 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2019-07-12 5:57 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, shirley17fei
Did this get stuck?
Fei Li <lifei1214@126.com> writes:
> 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 patch modifies the qemu_thread_create() by passing
> &error_abort and returing a value to indicate if it succeeds. The next
> 10 patches will improve on &error_abort for callers who could handle
> more properly.
>
> Please help to review, thanks a lot!
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle
2019-07-12 5:57 ` [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Markus Armbruster
@ 2019-07-16 12:25 ` fei
0 siblings, 0 replies; 34+ messages in thread
From: fei @ 2019-07-16 12:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, shirley17fei
Sorry for the late reply.
Yes, so sorry that this gets stuck for a long while. Actually I needs to send a v14 with a little update from v13. I will send a new version ASAP.
发自我的 iPhone
> 在 2019年7月12日，13:57，Markus Armbruster <armbru@redhat.com> 写道：
>
> Did this get stuck?
>
> Fei Li <lifei1214@126.com> writes:
>
>> 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 patch modifies the qemu_thread_create() by passing
>> &error_abort and returing a value to indicate if it succeeds. The next
>> 10 patches will improve on &error_abort for callers who could handle
>> more properly.
>>
>> Please help to review, thanks a lot!
> [...]
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-07-16 12:25 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-01 5:17 [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Fei Li
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 01/11] qemu_thread: make qemu_thread_create() take Error ** argument Fei Li
2019-02-01 10:02 ` Markus Armbruster
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu Fei Li
2019-02-01 12:33 ` Markus Armbruster
2019-02-02 4:47 ` fei
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 03/11] qemu_thread: supplement error handling for qmp_dump_guest_memory Fei Li
2019-02-01 12:34 ` Markus Armbruster
2019-02-01 5:17 ` [Qemu-devel] [PATCH v11 for-4.0 04/11] qemu_thread: supplement error handling for pci_edu_realize Fei Li
2019-02-01 12:58 ` Markus Armbruster
2019-02-02 4:44 ` fei
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 05/11] qemu_thread: supplement error handling for h_resize_hpt_prepare Fei Li
2019-02-01 13:01 ` Markus Armbruster
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 06/11] qemu_thread: supplement error handling for emulated_realize Fei Li
2019-02-01 13:04 ` Markus Armbruster
2019-02-03 7:17 ` Fei Li
2019-02-04 13:30 ` Markus Armbruster
2019-02-15 12:35 ` Fei Li
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 07/11] qemu_thread: supplement error handling for iothread_complete Fei Li
2019-02-01 14:03 ` Markus Armbruster
2019-02-02 4:51 ` fei
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 08/11] qemu_thread: supplement error handling for qemu_signalfd_compat Fei Li
2019-02-01 14:13 ` Markus Armbruster
2019-02-02 4:52 ` fei
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 09/11] qemu_thread: supplement error handling for migration Fei Li
2019-02-01 5:35 ` Fei Li
2019-02-01 15:34 ` Markus Armbruster
2019-02-03 7:55 ` Fei Li
2019-02-04 16:34 ` Dr. David Alan Gilbert
2019-02-15 13:23 ` Fei Li
2019-02-01 5:18 ` [Qemu-devel] [PATCH v11 for-4.0 10/11] qemu_thread: supplement error handling for vnc_start_worker_thread Fei Li
2019-02-01 14:16 ` Markus Armbruster
2019-07-12 5:57 ` [Qemu-devel] [PATCH v11 for-4.0 00/11] qemu_thread_create: propagate the error to callers to handle Markus Armbruster
2019-07-16 12:25 ` fei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).