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/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()).
> But did not test all the callers, so let’s wait for Stefan’s feedback. :)
But again, I did not do all the test. Correct me if I am wrong. :)
> > 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;
> >        }

Ok, thanks.

Have a nice day
Fei
> > Matter of taste.
> >
> > Hmm, iothread.c has no maintainer.  Stefan, you created it, would you be
> > willing to serve as maintainer?