[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v3 7/7] qemu_thread_create: propagate the er
|
From: |
Fam Zheng |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC v3 7/7] qemu_thread_create: propagate the error to callers to handle |
|
Date: |
Fri, 21 Sep 2018 09:05:51 +0800 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Thu, 09/20 18:19, Fei Li wrote:
>
>
> 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)) {
> }
The latter is better IMO.
>
> >
> > > + }
> > > }
> > > 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) :)
Yeah this is fine.
> >
> > > + return H_RESOURCE;
> > > + }
> > > spapr->pending_hpt = pending;
> > > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> > > index d7a4bbd91f..e7cbb0c368 100644
> > > --- a/hw/rdma/rdma_backend.c
> > > +++ b/hw/rdma/rdma_backend.c
> > > @@ -165,7 +165,8 @@ static void start_comp_thread(RdmaBackendDev
> > > *backend_dev)
> > > ibv_get_device_name(backend_dev->ib_dev));
> > > backend_dev->comp_thread.run = true;
> > > qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
> > > - comp_handler_thread, backend_dev,
> > > QEMU_THREAD_DETACHED);
> > > + comp_handler_thread, backend_dev,
> > > + QEMU_THREAD_DETACHED, &error_abort);
> > 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.c
> > > index aff1281257..5b2a1df36d 100644
> > > --- a/iothread.c
> > > +++ b/iothread.c
> > > @@ -161,9 +161,7 @@ static void iothread_complete(UserCreatable *obj,
> > > Error **errp)
> > > &local_error);
> > > if (local_error) {
> > > error_propagate(errp, local_error);
> > > - aio_context_unref(iothread->ctx);
> > > - iothread->ctx = NULL;
> > > - return;
> > > + goto fail;
> > > }
> > > qemu_mutex_init(&iothread->init_done_lock);
> > > @@ -175,8 +173,12 @@ static void iothread_complete(UserCreatable *obj,
> > > Error **errp)
> > > */
> > > name = object_get_canonical_path_component(OBJECT(obj));
> > > thread_name = g_strdup_printf("IO %s", name);
> > > - qemu_thread_create(&iothread->thread, thread_name, iothread_run,
> > > - iothread, QEMU_THREAD_JOINABLE);
> > > + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run,
> > > + iothread, QEMU_THREAD_JOINABLE, errp)) {
> > > + g_free(thread_name);
> > > + g_free(name);
> 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!
It's an initializer:
#define G_ONCE_INIT { G_ONCE_STATUS_NOTCALLED, NULL }
So I don't think any clean up is necessary.
>
> > > + 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.c
> > > index 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?)
In a different patch. This one is long enough.
> >
> > > 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);
Yeah I think this is better. We don't want to rely on multifd_save_cleanup()
always succeeds.
>
> >
> > > + }
> > > + 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.c
> > > index 2f6c72f63a..338b9563e3 100644
> > > --- a/tests/atomic_add-bench.c
> > > +++ b/tests/atomic_add-bench.c
> > > @@ -2,6 +2,7 @@
> > > #include "qemu/thread.h"
> > > #include "qemu/host-utils.h"
> > > #include "qemu/processor.h"
> > > +#include "qapi/error.h"
> > > struct thread_info {
> > > uint64_t r;
> > > @@ -110,7 +111,7 @@ static void create_threads(void)
> > > info->r = (i + 1) ^ time(NULL);
> > > qemu_thread_create(&threads[i], NULL, thread_func, info,
> > > - QEMU_THREAD_JOINABLE);
> > > + QEMU_THREAD_JOINABLE, &error_abort);
> > > }
> > > }
> > > diff --git a/tests/iothread.c b/tests/iothread.c
> > > index 777d9eea46..f4ad992e61 100644
> > > --- a/tests/iothread.c
> > > +++ b/tests/iothread.c
> > > @@ -73,7 +73,7 @@ IOThread *iothread_new(void)
> > > qemu_mutex_init(&iothread->init_done_lock);
> > > qemu_cond_init(&iothread->init_done_cond);
> > > qemu_thread_create(&iothread->thread, NULL, iothread_run,
> > > - iothread, QEMU_THREAD_JOINABLE);
> > > + iothread, QEMU_THREAD_JOINABLE, &error_abort);
> > > /* Wait for initialization to complete */
> > > qemu_mutex_lock(&iothread->init_done_lock);
> > > diff --git a/tests/qht-bench.c b/tests/qht-bench.c
> > > index f492b3a20a..20a4101a17 100644
> > > --- a/tests/qht-bench.c
> > > +++ b/tests/qht-bench.c
> > > @@ -9,6 +9,7 @@
> > > #include "qemu/atomic.h"
> > > #include "qemu/qht.h"
> > > #include "qemu/rcu.h"
> > > +#include "qapi/error.h"
> > > #include "exec/tb-hash-xx.h"
> > > struct thread_stats {
> > > @@ -239,7 +240,7 @@ th_create_n(QemuThread **threads, struct thread_info
> > > **infos, const char *name,
> > > prepare_thread_info(&info[i], offset + i);
> > > info[i].func = func;
> > > qemu_thread_create(&th[i], name, thread_func, &info[i],
> > > - QEMU_THREAD_JOINABLE);
> > > + QEMU_THREAD_JOINABLE, &error_abort);
> > > }
> > > }
> > > diff --git a/tests/rcutorture.c b/tests/rcutorture.c
> > > index 49311c82ea..0e799ff256 100644
> > > --- a/tests/rcutorture.c
> > > +++ b/tests/rcutorture.c
> > > @@ -64,6 +64,7 @@
> > > #include "qemu/atomic.h"
> > > #include "qemu/rcu.h"
> > > #include "qemu/thread.h"
> > > +#include "qapi/error.h"
> > > long long n_reads = 0LL;
> > > long n_updates = 0L;
> > > @@ -90,7 +91,7 @@ static void create_thread(void *(*func)(void *))
> > > exit(-1);
> > > }
> > > qemu_thread_create(&threads[n_threads], "test", func,
> > > &data[n_threads],
> > > - QEMU_THREAD_JOINABLE);
> > > + QEMU_THREAD_JOINABLE, &error_abort);
> > > n_threads++;
> > > }
> > > diff --git a/tests/test-aio.c b/tests/test-aio.c
> > > index 86fb73b3d5..b3ac261724 100644
> > > --- a/tests/test-aio.c
> > > +++ b/tests/test-aio.c
> > > @@ -154,7 +154,7 @@ static void test_acquire(void)
> > > qemu_thread_create(&thread, "test_acquire_thread",
> > > test_acquire_thread,
> > > - &data, QEMU_THREAD_JOINABLE);
> > > + &data, QEMU_THREAD_JOINABLE, &error_abort);
> > > /* Block in aio_poll(), let other thread kick us and acquire
> > > context */
> > > aio_context_acquire(ctx);
> > > diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
> > > index 192bfbf02e..9ea35a3dad 100644
> > > --- a/tests/test-rcu-list.c
> > > +++ b/tests/test-rcu-list.c
> > > @@ -25,6 +25,7 @@
> > > #include "qemu/rcu.h"
> > > #include "qemu/thread.h"
> > > #include "qemu/rcu_queue.h"
> > > +#include "qapi/error.h"
> > > /*
> > > * Test variables.
> > > @@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *))
> > > exit(-1);
> > > }
> > > qemu_thread_create(&threads[n_threads], "test", func,
> > > &data[n_threads],
> > > - QEMU_THREAD_JOINABLE);
> > > + QEMU_THREAD_JOINABLE, &error_abort);
> > > n_threads++;
> > > }
> > > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > > index 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)
>
Fam
- Re: [Qemu-devel] [PATCH RFC v3 3/7] qemu_init_vcpu: add a new Error parameter to propagate, (continued)
Re: [Qemu-devel] [PATCH RFC v3 0/7] qemu_thread_create: propagate errors to callers to check, Fam Zheng, 2018/09/19
Re: [Qemu-devel] [PATCH RFC v3 0/7] qemu_thread_create: propagate errors to callers to check, Peter Xu, 2018/09/19