|
| From: | Fei Li |
| Subject: | Re: [Qemu-devel] [PATCH RFC v3 7/7] qemu_thread_create: propagate the error to callers to handle |
| Date: | Thu, 20 Sep 2018 18:19:12 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 09/19/2018 11:51 PM, Fam Zheng wrote:
On Wed, 09/19 21:35, Fei Li wrote:Make qemu_thread_create() return a Boolean to indicate if it succeeds rather than failing with an error. And add an Error parameter to hold the error message and let the callers handle it. Signed-off-by: Fei Li <address@hidden> --- cpus.c | 45 +++++++++++++++++++++++++++-------------- dump.c | 6 ++++-- hw/misc/edu.c | 6 ++++-- hw/ppc/spapr_hcall.c | 9 +++++++-- hw/rdma/rdma_backend.c | 3 ++- hw/usb/ccid-card-emulated.c | 13 ++++++++---- include/qemu/thread.h | 4 ++-- io/task.c | 3 ++- iothread.c | 16 ++++++++++----- migration/migration.c | 49 +++++++++++++++++++++++++++++++-------------- migration/postcopy-ram.c | 14 +++++++++++-- migration/ram.c | 40 +++++++++++++++++++++++++++--------- migration/savevm.c | 11 +++++++--- tests/atomic_add-bench.c | 3 ++- tests/iothread.c | 2 +- tests/qht-bench.c | 3 ++- tests/rcutorture.c | 3 ++- tests/test-aio.c | 2 +- tests/test-rcu-list.c | 3 ++- ui/vnc-jobs.c | 8 ++++++-- util/compatfd.c | 9 +++++++-- util/oslib-posix.c | 17 ++++++++++++---- util/qemu-thread-posix.c | 18 +++++++++++------ util/qemu-thread-win32.c | 13 ++++++++---- util/rcu.c | 3 ++- util/thread-pool.c | 4 +++- 26 files changed, 217 insertions(+), 90 deletions(-) diff --git a/cpus.c b/cpus.c index 1feb308123..40db5c378f 100644 --- a/cpus.c +++ b/cpus.c @@ -1928,15 +1928,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp) snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG", cpu->cpu_index);- qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,- cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, + qemu_tcg_cpu_thread_fn, cpu, + QEMU_THREAD_JOINABLE, errp)) { + return; + }} else {/* share a single thread for all cpus with TCG */ snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG"); - qemu_thread_create(cpu->thread, thread_name, - qemu_tcg_rr_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, + qemu_tcg_rr_cpu_thread_fn, cpu, + QEMU_THREAD_JOINABLE, errp)) { + return; + }single_tcg_halt_cond = cpu->halt_cond;single_tcg_cpu_thread = cpu->thread; @@ -1964,8 +1969,10 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } #ifdef _WIN32 cpu->hThread = qemu_thread_get_handle(cpu->thread); #endif @@ -1980,8 +1987,10 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return;This is the last statement of the function body so "if" and "return" are unnecessary. But keep the 'if' here with an empty body probably makes sense so that it's easier to notice there is error handling logic here when making future changes, e.g. adding more lines after the qemu_thread_create call.
OK, which one of the followings do you think looks better?if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
cpu, QEMU_THREAD_JOINABLE, errp)) {}
or
if (!qemu_thread_create(cpu->thread, thread_name,
qemu_kvm_cpu_thread_fn,
cpu, QEMU_THREAD_JOINABLE, errp)) {
}
+ } }static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)@@ -1998,8 +2007,10 @@ static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + }Ditto here.}static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)@@ -2011,8 +2022,10 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return; + } #ifdef _WIN32 cpu->hThread = qemu_thread_get_handle(cpu->thread); #endif @@ -2027,8 +2040,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp) qemu_cond_init(cpu->halt_cond); snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY", cpu->cpu_index); - qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, + cpu, QEMU_THREAD_JOINABLE, errp)) { + return;Ditto.+ } }bool qemu_init_vcpu(CPUState *cpu, Error **errp)diff --git a/dump.c b/dump.c index 500b554523..4175b95d12 100644 --- a/dump.c +++ b/dump.c @@ -2021,8 +2021,10 @@ void qmp_dump_guest_memory(bool paging, const char *file, if (detach_p) { /* detached dump */ s->detached = true; - qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, - s, QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread, + s, QEMU_THREAD_DETACHED, errp)) { + return;Ditto.+ } } else { /* sync dump */ dump_process(s, errp); diff --git a/hw/misc/edu.c b/hw/misc/edu.c index df26a4d046..2810192b1f 100644 --- a/hw/misc/edu.c +++ b/hw/misc/edu.c @@ -354,8 +354,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)qemu_mutex_init(&edu->thr_mutex);qemu_cond_init(&edu->thr_cond); - qemu_thread_create(&edu->thread, "edu", edu_fact_thread, - edu, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread, + edu, QEMU_THREAD_JOINABLE, errp)) { + return; + }memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,"edu-mmio", 1 * MiB); diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index ae913d070f..94df1e72ab 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, sPAPRPendingHPT *pending = spapr->pending_hpt; uint64_t current_ram_size; int rc; + Error *local_err = NULL;if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {return H_AUTHORITY; @@ -538,8 +539,12 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, pending->shift = shift; pending->ret = H_HARDWARE;- qemu_thread_create(&pending->thread, "sPAPR HPT prepare",- hpt_prepare_thread, pending, QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare", + hpt_prepare_thread, pending, + QEMU_THREAD_DETACHED, &local_err)) { + error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");I think a free_pending_hpt() is missing here.
Right, thanks for the reminder. Maybe just add one line like below? g_free(pending); (As pending->hpt is obviously not initialized in above code) :)
+ return H_RESOURCE; + }spapr->pending_hpt = pending; diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.cindex d7a4bbd91f..e7cbb0c368 100644 --- a/hw/rdma/rdma_backend.c +++ b/hw/rdma/rdma_backend.c @@ -165,7 +165,8 @@ static void start_comp_thread(RdmaBackendDev *backend_dev) ibv_get_device_name(backend_dev->ib_dev)); backend_dev->comp_thread.run = true; qemu_thread_create(&backend_dev->comp_thread.thread, thread_name, - comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED); + comp_handler_thread, backend_dev, + QEMU_THREAD_DETACHED, &error_abort);Previously we don't abort() QEMU if a new thread cannot be created. I think we want some more robustness here. Peter?}void rdma_backend_register_comp_handler(void (*handler)(int status,diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c index 5c8b3c9907..0d630c27db 100644 --- a/hw/usb/ccid-card-emulated.c +++ b/hw/usb/ccid-card-emulated.c @@ -538,10 +538,15 @@ static void emulated_realize(CCIDCardState *base, Error **errp) error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID); return; } - qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, - card, QEMU_THREAD_JOINABLE); - qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread, - card, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread, + card, QEMU_THREAD_JOINABLE, errp)) { + return; + } + if (!qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", + handle_apdu_thread, card, + QEMU_THREAD_JOINABLE, errp)) { + return; + } }
Will delete the above second "return;" too.
static void emulated_unrealize(CCIDCardState *base, Error **errp)diff --git a/include/qemu/thread.h b/include/qemu/thread.h index dacebcfff0..1fb84a07d2 100644 --- a/include/qemu/thread.h +++ b/include/qemu/thread.h @@ -135,9 +135,9 @@ void qemu_event_reset(QemuEvent *ev); void qemu_event_wait(QemuEvent *ev); void qemu_event_destroy(QemuEvent *ev);-void qemu_thread_create(QemuThread *thread, const char *name,+bool qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void *), - void *arg, int mode); + void *arg, int mode, Error **errp); void *qemu_thread_join(QemuThread *thread); void qemu_thread_get_self(QemuThread *thread); bool qemu_thread_is_self(QemuThread *thread); diff --git a/io/task.c b/io/task.c index 2886a2c1bc..6d3a18ab80 100644 --- a/io/task.c +++ b/io/task.c @@ -149,7 +149,8 @@ void qio_task_run_in_thread(QIOTask *task, "io-task-worker", qio_task_thread_worker, data, - QEMU_THREAD_DETACHED); + QEMU_THREAD_DETACHED, + &error_abort); }diff --git a/iothread.c b/iothread.cindex aff1281257..5b2a1df36d 100644 --- a/iothread.c +++ b/iothread.c @@ -161,9 +161,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &local_error); if (local_error) { error_propagate(errp, local_error); - aio_context_unref(iothread->ctx); - iothread->ctx = NULL; - return; + goto fail; }qemu_mutex_init(&iothread->init_done_lock);@@ -175,8 +173,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) */ name = object_get_canonical_path_component(OBJECT(obj)); thread_name = g_strdup_printf("IO %s", name); - qemu_thread_create(&iothread->thread, thread_name, iothread_run, - iothread, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, + iothread, QEMU_THREAD_JOINABLE, errp)) { + g_free(thread_name); + g_free(name);
I think + qemu_mutex_destroy(&iothread->init_done_lock); + qemu_cond_destroy(&iothread->init_done_cond); should also be added to be cleaned too. Sorry for the omit.. But one uncertain thing is about whether we should do anything about the below GOnce: iothread->once = (GOnce) G_ONCE_INIT; I did not find enough valid information about this, could someone shed light on me? Thanks!
+ goto fail; + } g_free(thread_name); g_free(name);@@ -187,6 +189,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp)&iothread->init_done_lock); } qemu_mutex_unlock(&iothread->init_done_lock); + return; +fail: + aio_context_unref(iothread->ctx); + iothread->ctx = NULL; }typedef struct {diff --git a/migration/migration.c b/migration/migration.c index 4b316ec343..bfc7a8f015 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -388,6 +388,7 @@ static void process_incoming_migration_co(void *opaque) MigrationIncomingState *mis = migration_incoming_get_current(); PostcopyState ps; int ret; + Error *local_err = NULL;assert(mis->from_src_file);mis->migration_incoming_co = qemu_coroutine_self(); @@ -420,8 +421,13 @@ static void process_incoming_migration_co(void *opaque)/* we get COLO info, and know if we are in COLO mode */if (!ret && migration_incoming_enable_colo()) { - qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", - colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming", + colo_process_incoming_thread, mis, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "failed to create " + "colo_process_incoming_thread: "); + goto fail; + } mis->have_colo_incoming_thread = true; qemu_coroutine_yield();@@ -430,20 +436,22 @@ static void process_incoming_migration_co(void *opaque)}if (ret < 0) {- Error *local_err = NULL; - - migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, - MIGRATION_STATUS_FAILED); error_report("load of migration failed: %s", strerror(-ret)); - qemu_fclose(mis->from_src_file); - if (multifd_load_cleanup(&local_err) != 0) { - error_report_err(local_err); - } - exit(EXIT_FAILURE); + goto fail; } mis->bh = qemu_bh_new(process_incoming_migration_bh, mis); qemu_bh_schedule(mis->bh); mis->migration_incoming_co = NULL; + return; +fail: + local_err = NULL; + migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, + MIGRATION_STATUS_FAILED); + qemu_fclose(mis->from_src_file); + if (multifd_load_cleanup(&local_err) != 0) { + error_report_err(local_err); + } + exit(EXIT_FAILURE); }static void migration_incoming_setup(QEMUFile *f)@@ -2288,6 +2296,7 @@ out: static int open_return_path_on_source(MigrationState *ms, bool create_thread) { + Error *local_err = NULL;ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);if (!ms->rp_state.from_dst_file) { @@ -2301,8 +2310,13 @@ static int open_return_path_on_source(MigrationState *ms, return 0; }- qemu_thread_create(&ms->rp_state.rp_thread, "return path",- source_return_path_thread, ms, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&ms->rp_state.rp_thread, "return path", + source_return_path_thread, ms, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "failed to create source_return_path_thread: "); + return -1; + }trace_open_return_path_on_source_continue(); @@ -3127,8 +3141,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)migrate_fd_cleanup(s); return; } - qemu_thread_create(&s->thread, "live_migration", migration_thread, s, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&s->thread, "live_migration", migration_thread, + s, QEMU_THREAD_JOINABLE, &error_in)) { + error_reportf_err(error_in, "failed to create migration_thread: "); + migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); + migrate_fd_cleanup(s); + return; + } s->migration_thread_running = true; }diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.cindex 853d8b32ca..fbbd3c9a96 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1082,6 +1082,8 @@ retry:int postcopy_ram_enable_notify(MigrationIncomingState *mis){ + Error *local_err = NULL; + /* Open the fd for the kernel to give us userfaults */ mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK); if (mis->userfault_fd == -1) { @@ -1108,8 +1110,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis) }qemu_sem_init(&mis->fault_thread_sem, 0);- qemu_thread_create(&mis->fault_thread, "postcopy/fault", - postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&mis->fault_thread, "postcopy/fault", + postcopy_ram_fault_thread, mis, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "failed to create postcopy_ram_fault_thread: "); + close(mis->userfault_event_fd); + close(mis->userfault_fd); + qemu_sem_destroy(&mis->fault_thread_sem); + return -1; + }Side note unrelated to this patch: maybe the error handling of qemu_ram_foreach_migratable_block() needs some clean up too?
You mean + close(mis->userfault_event_fd); + close(mis->userfault_fd); right? Could I add them in this patch, or write a new patch? (Maybe write a new patch together with the below adjust?)
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 8338ffd63b..dcb7d92d3c 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -473,6 +473,7 @@ static void compress_threads_save_cleanup(void) static int compress_threads_save_setup(void) { int i, thread_count; + Error *local_err = NULL;if (!migrate_use_compression()) {return 0; @@ -502,9 +503,12 @@ static int compress_threads_save_setup(void) comp_param[i].quit = false; qemu_mutex_init(&comp_param[i].mutex); qemu_cond_init(&comp_param[i].cond); - qemu_thread_create(compress_threads + i, "compress", - do_data_compress, comp_param + i, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(compress_threads + i, "compress", + do_data_compress, comp_param + i, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "failed to create do_data_compress: "); + goto exit; + } } return 0;@@ -1087,8 +1091,15 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)p->c = QIO_CHANNEL(sioc); qio_channel_set_delay(p->c, false); p->running = true; - qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&p->thread, p->name, multifd_send_thread, p, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "failed to create multifd_send_thread: ");You need to set local_err = NULL before passing it to the next callee.
Ok.
+ if (multifd_save_cleanup(&local_err) != 0) { + migrate_set_error(migrate_get_current(), local_err);Even if multifd_save_cleanup() failed, migrate_set_error() should still be called, no?
Emm, a little obscure in our current code. As I see the passed &local_err is never used and multifd_save_cleanup() always return 0. Maybe there is some unknown reason for keeping this? If not, could we adjust as below? (And do the same for other involved) + multifd_save_cleanup(); + migrate_set_error(migrate_get_current(), local_err);
+ } + return; + }atomic_inc(&multifd_send_state->count);} @@ -1362,8 +1373,12 @@ bool multifd_recv_new_channel(QIOChannel *ioc) p->num_packets = 1;p->running = true;- qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&p->thread, p->name, multifd_recv_thread, + p, QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "failed to create multifd_recv_thread: "); + multifd_recv_terminate_threads(local_err, true); + return false; + } atomic_inc(&multifd_recv_state->count); return multifd_recv_state->count == migrate_multifd_channels(); } @@ -3559,6 +3574,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; @@ -3580,9 +3596,13 @@ static int compress_threads_load_setup(QEMUFile *f) qemu_cond_init(&decomp_param[i].cond); decomp_param[i].done = true; decomp_param[i].quit = false; - qemu_thread_create(decompress_threads + i, "decompress", - do_data_decompress, decomp_param + i, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(decompress_threads + i, "decompress", + do_data_decompress, decomp_param + i, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, + "failed to create do_data_decompress: "); + goto exit; + } } return 0; exit: diff --git a/migration/savevm.c b/migration/savevm.c index 13e51f0e34..fc26a10e68 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1727,9 +1727,14 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) mis->have_listen_thread = true; /* Start up the listening thread and wait for it to signal ready */ qemu_sem_init(&mis->listen_thread_sem, 0); - qemu_thread_create(&mis->listen_thread, "postcopy/listen", - postcopy_ram_listen_thread, NULL, - QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&mis->listen_thread, "postcopy/listen", + postcopy_ram_listen_thread, NULL, + QEMU_THREAD_DETACHED, &local_err)) { + error_reportf_err(local_err, + "failed to create postcopy_ram_listen_thread: "); + qemu_sem_destroy(&mis->listen_thread_sem); + return -1; + } qemu_sem_wait(&mis->listen_thread_sem); qemu_sem_destroy(&mis->listen_thread_sem);diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.cindex 2f6c72f63a..338b9563e3 100644 --- a/tests/atomic_add-bench.c +++ b/tests/atomic_add-bench.c @@ -2,6 +2,7 @@ #include "qemu/thread.h" #include "qemu/host-utils.h" #include "qemu/processor.h" +#include "qapi/error.h"struct thread_info {uint64_t r; @@ -110,7 +111,7 @@ static void create_threads(void)info->r = (i + 1) ^ time(NULL);qemu_thread_create(&threads[i], NULL, thread_func, info, - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); } }diff --git a/tests/iothread.c b/tests/iothread.cindex 777d9eea46..f4ad992e61 100644 --- a/tests/iothread.c +++ b/tests/iothread.c @@ -73,7 +73,7 @@ IOThread *iothread_new(void) qemu_mutex_init(&iothread->init_done_lock); qemu_cond_init(&iothread->init_done_cond); qemu_thread_create(&iothread->thread, NULL, iothread_run, - iothread, QEMU_THREAD_JOINABLE); + iothread, QEMU_THREAD_JOINABLE, &error_abort);/* Wait for initialization to complete */qemu_mutex_lock(&iothread->init_done_lock); diff --git a/tests/qht-bench.c b/tests/qht-bench.c index f492b3a20a..20a4101a17 100644 --- a/tests/qht-bench.c +++ b/tests/qht-bench.c @@ -9,6 +9,7 @@ #include "qemu/atomic.h" #include "qemu/qht.h" #include "qemu/rcu.h" +#include "qapi/error.h" #include "exec/tb-hash-xx.h"struct thread_stats {@@ -239,7 +240,7 @@ th_create_n(QemuThread **threads, struct thread_info **infos, const char *name, prepare_thread_info(&info[i], offset + i); info[i].func = func; qemu_thread_create(&th[i], name, thread_func, &info[i], - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); } }diff --git a/tests/rcutorture.c b/tests/rcutorture.cindex 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.cindex 86fb73b3d5..b3ac261724 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -154,7 +154,7 @@ static void test_acquire(void)qemu_thread_create(&thread, "test_acquire_thread",test_acquire_thread, - &data, QEMU_THREAD_JOINABLE); + &data, QEMU_THREAD_JOINABLE, &error_abort);/* Block in aio_poll(), let other thread kick us and acquire context */aio_context_acquire(ctx); diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c index 192bfbf02e..9ea35a3dad 100644 --- a/tests/test-rcu-list.c +++ b/tests/test-rcu-list.c @@ -25,6 +25,7 @@ #include "qemu/rcu.h" #include "qemu/thread.h" #include "qemu/rcu_queue.h" +#include "qapi/error.h"/** Test variables. @@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *)) exit(-1); } qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads], - QEMU_THREAD_JOINABLE); + QEMU_THREAD_JOINABLE, &error_abort); n_threads++; }diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.cindex 8807d7217c..35a652d1fd 100644 --- a/ui/vnc-jobs.c +++ b/ui/vnc-jobs.c @@ -31,6 +31,7 @@ #include "vnc-jobs.h" #include "qemu/sockets.h" #include "qemu/main-loop.h" +#include "qapi/error.h" #include "block/aio.h"/*@@ -340,8 +341,11 @@ bool vnc_start_worker_thread(Error **errp) }q = vnc_queue_init();- qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q, - QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, + q, QEMU_THREAD_DETACHED, errp)) { + vnc_queue_clear(q); + return false; + } queue = q; /* Set global queue */ out: return true; diff --git a/util/compatfd.c b/util/compatfd.c index d3ed890405..cedae5370d 100644 --- a/util/compatfd.c +++ b/util/compatfd.c @@ -91,8 +91,13 @@ static int qemu_signalfd_compat(const sigset_t *mask, Error **errp) memcpy(&info->mask, mask, sizeof(*mask)); info->fd = fds[1];- qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,- QEMU_THREAD_DETACHED); + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, + info, QEMU_THREAD_DETACHED, errp)) { + free(info); + close(fds[0]); + close(fds[1]); + return -1; + }return fds[0];} diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 13b6f8d776..85d0504f5a 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -364,9 +364,12 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, size_t size_per_thread; char *addr = area; int i = 0; + int started_thread = 0; + Error *local_err = NULL;memset_thread_failed = false;memset_num_threads = get_memset_num_threads(smp_cpus); + started_thread = memset_num_threads; memset_thread = g_new0(MemsetThread, memset_num_threads); numpages_per_thread = (numpages / memset_num_threads); size_per_thread = (hpagesize * numpages_per_thread); @@ -375,13 +378,19 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages, memset_thread[i].numpages = (i == (memset_num_threads - 1)) ? numpages : numpages_per_thread; memset_thread[i].hpagesize = hpagesize; - qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", - do_touch_pages, &memset_thread[i], - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages", + do_touch_pages, &memset_thread[i], + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "failed to create do_touch_pages: "); + memset_thread_failed = true; + started_thread = i; + goto out; + } addr += size_per_thread; numpages -= numpages_per_thread; } - for (i = 0; i < memset_num_threads; i++) { +out: + for (i = 0; i < started_thread; i++) { qemu_thread_join(&memset_thread[i].pgthread); } g_free(memset_thread); diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index 289af4fab5..a968f6e7c9 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -15,6 +15,7 @@ #include "qemu/atomic.h" #include "qemu/notify.h" #include "qemu-thread-common.h" +#include "qapi/error.h"static bool name_threads; @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)return start_routine(arg); }-void qemu_thread_create(QemuThread *thread, const char *name,- void *(*start_routine)(void*), - void *arg, int mode) +bool qemu_thread_create(QemuThread *thread, const char *name, + void *(*start_routine)(void *), + void *arg, int mode, Error **errp) { sigset_t set, oldset; int err; @@ -515,7 +516,7 @@ void qemu_thread_create(QemuThread *thread, const char *name,err = pthread_attr_init(&attr);if (err) { - error_exit(err, __func__);Please call error_setg() here,
ok.
+ goto fail; }if (mode == QEMU_THREAD_DETACHED) {@@ -534,12 +535,17 @@ void qemu_thread_create(QemuThread *thread, const char *name, err = pthread_create(&thread->thread, &attr, qemu_thread_start, qemu_thread_args);- if (err)- error_exit(err, __func__); + if (err) {and here, with a different error message.
ok.
+ goto fail; + }pthread_sigmask(SIG_SETMASK, &oldset, NULL); pthread_attr_destroy(&attr);+ return true; +fail: + error_setg(errp, "qemu_thread_create failed: %s", strerror(err));And remove this one. And pthread_attr_destroy() is needed here as well if pthread_attr_init() has succeeded. Remember that a failed function must clean up all the resources it has already initialized before returning, otherwise the resource is leaked.
Ah, right! Thanks for pointing this out.
+ return false; }void qemu_thread_get_self(QemuThread *thread)diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c index 1a27e1cf6f..f4e6344e34 100644 --- a/util/qemu-thread-win32.c +++ b/util/qemu-thread-win32.c @@ -20,6 +20,7 @@ #include "qemu/thread.h" #include "qemu/notify.h" #include "qemu-thread-common.h" +#include "qapi/error.h" #include <process.h>static bool name_threads;@@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread) return ret; }-void qemu_thread_create(QemuThread *thread, const char *name,- void *(*start_routine)(void *), - void *arg, int mode) +bool qemu_thread_create(QemuThread *thread, const char *name, + void *(*start_routine)(void *), + void *arg, int mode, Error **errp) { HANDLE hThread; struct QemuThreadData *data; @@ -409,10 +410,14 @@ void qemu_thread_create(QemuThread *thread, const char *name, hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine, data, 0, &thread->tid); if (!hThread) { - error_exit(GetLastError(), __func__);I think you need to call DeleteCriticalSection here depending on data->mode.
ok, will add this as below:
+ if (data->mode != QEMU_THREAD_DETACHED) {
+ DeleteCriticalSection(&data->cs);
+ }
Thanks a lot for the review. :)
Have a nice day
Fei
+ g_free(data); + error_setg_win32(errp, GetLastError(), + "failed to create win32_start_routine"); + return false; } CloseHandle(hThread); thread->data = data; + return true; }void qemu_thread_get_self(QemuThread *thread)
| [Prev in Thread] | Current Thread | [Next in Thread] |