Fei Li <f...@suse.com> writes: > For iothread_complete: utilize the existed errp to propagate the > error and do the corresponding cleanup to replace the temporary > &error_abort. > > For qemu_signalfd_compat: add a local_err to hold the error, and > return the corresponding error code to replace the temporary > &error_abort.
I'd split the patch. > > Cc: Markus Armbruster <arm...@redhat.com> > Cc: Eric Blake <ebl...@redhat.com> > Signed-off-by: Fei Li <f...@suse.com> > --- > iothread.c | 17 +++++++++++------ > util/compatfd.c | 11 ++++++++--- > 2 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/iothread.c b/iothread.c > index 8e8aa01999..7335dacf0b 100644 > --- a/iothread.c > +++ b/iothread.c > @@ -164,9 +164,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,9 +176,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); > - /* 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); > + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, > + iothread, QEMU_THREAD_JOINABLE, errp)) { > + g_free(thread_name); > + g_free(name); I suspect you're missing cleanup here: qemu_cond_destroy(&iothread->init_done_cond); qemu_mutex_destroy(&iothread->init_done_lock); But I'm not 100% sure, to be honest. Stefan, can you help? > + goto fail; > + } > g_free(thread_name); > g_free(name); > I'd avoid the code duplication like this: 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) { qemu_cond_destroy(&iothread->init_done_cond); qemu_mutex_destroy(&iothread->init_done_lock); goto fail; } Matter of taste. Hmm, iothread.c has no maintainer. Stefan, you created it, would you be willing to serve as maintainer? > @@ -191,6 +192,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/util/compatfd.c b/util/compatfd.c > index c3d8448264..9cb13381e4 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]; > + Error *local_err = NULL; > > info = malloc(sizeof(*info)); > if (info == NULL) { > @@ -89,9 +90,13 @@ 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); > + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, > + info, QEMU_THREAD_DETACHED, &local_err)) { > + close(fds[0]); > + close(fds[1]); > + free(info); > + return -1; Leaks @local_err. Pass NULL instead. > + } > > return fds[0]; > }