From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:49419)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1ZCdy2-0002EO-QH
	for qemu-devel@nongnu.org; Tue, 07 Jul 2015 21:16:08 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1ZCdy1-0001yo-OL
	for qemu-devel@nongnu.org; Tue, 07 Jul 2015 21:16:06 -0400
Date: Wed, 8 Jul 2015 09:15:57 +0800
From: Fam Zheng <famz@redhat.com>
Message-ID: <20150708011557.GE10382@ad.nay.redhat.com>
References: <1435670385-625-1-git-send-email-famz@redhat.com>
	<1435670385-625-4-git-send-email-famz@redhat.com>
	<20150707143549.GF28673@stefanha-thinkpad.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150707143549.GF28673@stefanha-thinkpad.redhat.com>
Subject: Re: [Qemu-devel] [PATCH RFC 3/4] aio: Introduce aio_context_setup
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org

On Tue, 07/07 15:35, Stefan Hajnoczi wrote:
> On Tue, Jun 30, 2015 at 09:19:44PM +0800, Fam Zheng wrote:
> > diff --git a/async.c b/async.c
> > index 06971f4..1d70cfd 100644
> > --- a/async.c
> > +++ b/async.c
> > @@ -290,12 +290,17 @@ AioContext *aio_context_new(Error **errp)
> >  {
> >      int ret;
> >      AioContext *ctx;
> > +    Error *local_err = NULL;
> > +
> >      ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
> > +    aio_context_setup(ctx, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> 
> Is there any reason to introduce local_err?  errp can be passed directly
> into aio_context_setup().

It's used for catching failure of aio_context_setup, because the convention is
errp can be NULL.

> 
> > +        goto fail;
> > +    }
> >      ret = event_notifier_init(&ctx->notifier, false);
> >      if (ret < 0) {
> > -        g_source_destroy(&ctx->source);
> > -        error_setg_errno(errp, -ret, "Failed to initialize event notifier");
> > -        return NULL;
> > +        goto fail;
> >      }
> >      g_source_set_can_recurse(&ctx->source, true);
> >      aio_set_event_notifier(ctx, &ctx->notifier,
> > @@ -307,6 +312,10 @@ AioContext *aio_context_new(Error **errp)
> >      timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx);
> >  
> >      return ctx;
> > +fail:
> > +    g_source_destroy(&ctx->source);
> > +    error_setg_errno(errp, -ret, "Failed to initialize event notifier");
> 
> If aio_context_setup() failed then this hits the *errp == NULL assertion
> failure in error_setg_errno().  This call shouldn't be moved away from
> the event_notifier_init() call.

Yes, will fix.

Fam