| Message ID | 1435670385-625-4-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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(). > + 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.
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
On Wed, Jul 08, 2015 at 09:15:57AM +0800, Fam Zheng wrote: > 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. You are right, I missed that aio_context_setup() has a void return type.
diff --git a/aio-posix.c b/aio-posix.c index f516de1..22406ce 100644 --- a/aio-posix.c +++ b/aio-posix.c @@ -42,6 +42,10 @@ static AioHandler *find_aio_handler(AioContext *ctx, int fd) return NULL; } +void aio_context_setup(AioContext *ctx, Error **errp) +{ +} + void aio_set_fd_handler_pri(AioContext *ctx, int fd, IOHandler *io_read, diff --git a/aio-win32.c b/aio-win32.c index 3c75896..852aa97 100644 --- a/aio-win32.c +++ b/aio-win32.c @@ -31,6 +31,10 @@ struct AioHandler { QLIST_ENTRY(AioHandler) node; }; +void aio_context_setup(AioContext *ctx, Error **errp) +{ +} + void aio_set_fd_handler_pri(AioContext *ctx, int fd, IOHandler *io_read, 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); + 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"); + return NULL; } void aio_context_ref(AioContext *ctx) diff --git a/include/block/aio.h b/include/block/aio.h index 82502e1..5120583 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -261,6 +261,9 @@ void aio_set_fd_handler_pri(AioContext *ctx, IOHandler *io_read_pri, void *opaque); +/* Initialize OS specific bits in AioContext */ +void aio_context_setup(AioContext *ctx, Error **errp); + /* Register an event notifier and associated callbacks. Behaves very similarly * to event_notifier_set_handler. Unlike event_notifier_set_handler, these callbacks * will be invoked when using aio_poll().
This is the place to initialize OS specific bits of AioContext. Signed-off-by: Fam Zheng <famz@redhat.com> --- aio-posix.c | 4 ++++ aio-win32.c | 4 ++++ async.c | 15 ++++++++++++--- include/block/aio.h | 3 +++ 4 files changed, 23 insertions(+), 3 deletions(-)