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

[Fei Li]

在 2019/1/14 下午8:53, 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.
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.
>
> [...]