在 2019/1/14 下午8:53, Markus Armbruster 写道:
Fei Li <lifei1...@126.com> writes:
在 2019/1/9 上午12:18, fei 写道:
在 2019年1月8日，01:50，Markus Armbruster <arm...@redhat.com> 写道：
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.
Ok.
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);
I remember I checked the code, when ucc->complete() fails, there’s a finalize()
function to do the destroy.
To be specific, the qemu_xxx_destroy() is called by
object_unref() => object_finalize() => object_deinit() =>
type->instance_finalize(obj); (that is, iothread_instance_finalize).
For the iothread_complete(), it is only called in
user_creatable_complete() as ucc->complete().
I checked the code, when callers of user_creatable_complete() fails,
all of them will call
object_unref() to call the qemu_xxx_destroy(), except one &error_abort
case (e.i. desugar_shm()).
I'm not familiar with iothread.c. But like anyone capable of reading C,
I can find out stuff.
iothread_instance_finalize() guards its cleanups. In particular, it
cleans up ->init_done_cond and init_done_lock only when ->thread_id !=
-1.
Ah, sorry that I overlooked the "if (iothread->thread_id != -1)".
So embarrassed, and sorry for the trouble.. You are right, and I
will add the qemu_xxx_destroy() in the next version. ;)
Have a nice day, thanks so much!
Fei
iothread_instance_init() initializes ->thread_id = -1.
iothread_run() sets it to the actual thread ID.
When iothread_instance_complete() succeeds, it has waited for
->thread_id to become != -1, in the /* Wait for initialization to
complete */ loop.
When it fails, ->thread_id is still -1.
Therefore, you cannot rely on iothread_instance_finalize() for cleaning
up ->init_done_lock and ->init_done_cond on iothread_instance_complete()
failure.
I'm pretty sure you could've figured this out yourself instead of
relying on me.
[...]
