Re: [Qemu-devel] [PATCH for-4.0 v9 12/16] qemu_thread: supplement error handling for iothread_complete/qemu_signalfd_compat

[Markus Armbruster]

Fei Li <address@hidden> writes:

> 在 2019/1/9 上午12:18, fei 写道:
>>
>>> 在 2019年1月8日，01:50，Markus Armbruster <address@hidden> 写道：
>>>
>>> Fei Li <address@hidden> 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 <address@hidden>
>>>> Cc: Eric Blake <address@hidden>
>>>> Signed-off-by: Fei Li <address@hidden>
>>>> ---
>>>> 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.

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.

[...]