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

[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. But did not test all the callers, so let’s wait for 
Stefan’s feedback. :)
> 
> 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.
Ok, thanks!

Have a nice day 
Fei
> 
>> +    }
>> 
>>     return fds[0];
>> }