| Message ID | 1410690193-21813-2-git-send-email-cnanakos@grnet.gr |
|---|---|
| State | New |
| Headers | show |
The Sunday 14 Sep 2014 à 13:23:13 (+0300), Chrysostomos Nanakos wrote : > If event_notifier_init fails QEMU exits without printing > any error information to the user. This commit adds an error > message on failure: > > # qemu [...] > qemu: event_notifier_init failed: Too many open files in system > qemu: qemu_init_main_loop failed > > Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> > --- > async.c | 19 +++++++++++++------ > include/block/aio.h | 2 +- > include/qemu/main-loop.h | 2 +- > iothread.c | 12 +++++++++++- > main-loop.c | 11 +++++++++-- > qemu-img.c | 11 ++++++++++- > qemu-io.c | 10 +++++++++- > qemu-nbd.c | 12 +++++++++--- > tests/test-aio.c | 12 +++++++++++- > tests/test-thread-pool.c | 10 +++++++++- > tests/test-throttle.c | 12 +++++++++++- > vl.c | 7 +++++-- > 12 files changed, 99 insertions(+), 21 deletions(-) > > diff --git a/async.c b/async.c > index a99e7f6..d4b6687 100644 > --- a/async.c > +++ b/async.c > @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) > aio_notify(opaque); > } > > -AioContext *aio_context_new(void) > +int aio_context_new(AioContext **context, Error **errp) > { > + int ret; > AioContext *ctx; > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > + ret = event_notifier_init(&ctx->notifier, false); > + if (ret < 0) { > + g_source_destroy(&ctx->source); > + error_setg_errno(errp, -ret, "event_notifier_init failed"); aio_context_new does not seems to be guest triggered so it may be actually correct to bake an error message in it. I don't have enough AIO knowledge to juge this. However your current error message: "event_notifier_init failed" look more like a trace than an actual QEMU error message. Please grep the code for example error messages: they are usually more plaintext than this one Also switching to returning a -errno make the caller's code convoluted. Maybe returning NULL would be enough. I see further in the patch that the return code is not really used on the top most level maybe it's a hint that return NULL here would be better. > + return ret; > + } > + aio_set_event_notifier(ctx, &ctx->notifier, > + (EventNotifierHandler *) > + event_notifier_test_and_clear); > iothread->stopping = false; > - iothread->ctx = aio_context_new(); > + ret = aio_context_new(&iothread->ctx, &local_error); > + if (ret < 0) { > + errno = -ret; You don't seem to reuse errno further down in the code. > gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > - qemu_aio_context = aio_context_new(); > + ret = aio_context_new(&qemu_aio_context, &local_error); > + if (ret < 0) { > + errno = -ret; Same here errno does not seems used by error propagate. Also you seems to leak gpollfds but probably you count on the fact that the kernel will collect it for you because QEMU will die. I think you could move down the gpollfds = g_array_new after the end of the test. > + error_propagate(errp, local_error); > + return ret; > + } > + > > - qemu_init_main_loop(); > + ret = qemu_init_main_loop(&local_error); > + if (ret < 0) { > + errno = -ret; > + error_report("%s", error_get_pretty(local_error)); > + error_free(local_error); > + error_exit("qemu_init_main_loop failed"); > + } > + > bdrv_init(); > if (argc < 2) { > error_exit("Not enough arguments"); > diff --git a/qemu-io.c b/qemu-io.c > index 33c96c4..f10dab5 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -387,8 +387,10 @@ int main(int argc, char **argv) > { NULL, 0, NULL, 0 } > }; > int c; > + int ret; > int opt_index = 0; > int flags = BDRV_O_UNMAP; > + Error *local_error = NULL; > > #ifdef CONFIG_POSIX > signal(SIGPIPE, SIG_IGN); > @@ -454,7 +456,13 @@ int main(int argc, char **argv) > exit(1); > } > > - qemu_init_main_loop(); > + ret = qemu_init_main_loop(&local_error); > + if (ret < 0) { > + error_report("%s", error_get_pretty(local_error)); > + error_report("qemu_init_main_loop failed"); > + error_free(local_error); > + return ret; > + } > bdrv_init(); > > /* initialize commands */ > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 9bc152e..4ba9635 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -498,13 +498,13 @@ int main(int argc, char **argv) > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, > &local_err); > if (local_err) { > - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > error_get_pretty(local_err)); > } > if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > !(flags & BDRV_O_UNMAP)) { > errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " > - "without setting discard operation to unmap"); > + "without setting discard operation to unmap"); > } > break; > case 'b': > @@ -674,7 +674,13 @@ int main(int argc, char **argv) > snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > } > > - qemu_init_main_loop(); > + ret = qemu_init_main_loop(&local_err); > + if (ret < 0) { > + errno = -ret; > + error_report("%s", error_get_pretty(local_err)); > + error_free(local_err); > + err(EXIT_FAILURE, "qemu_init_main_loop failed"); > + } > bdrv_init(); > atexit(bdrv_close_all); > > diff --git a/tests/test-aio.c b/tests/test-aio.c > index c6a8713..a9c9073 100644
The Monday 15 Sep 2014 à 21:03:18 (+0200), Benoît Canet wrote : > The Sunday 14 Sep 2014 à 13:23:13 (+0300), Chrysostomos Nanakos wrote : > > If event_notifier_init fails QEMU exits without printing > > any error information to the user. This commit adds an error > > message on failure: > > > > # qemu [...] > > qemu: event_notifier_init failed: Too many open files in system > > qemu: qemu_init_main_loop failed > > > > Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> > > --- > > async.c | 19 +++++++++++++------ > > include/block/aio.h | 2 +- > > include/qemu/main-loop.h | 2 +- > > iothread.c | 12 +++++++++++- > > main-loop.c | 11 +++++++++-- > > qemu-img.c | 11 ++++++++++- > > qemu-io.c | 10 +++++++++- > > qemu-nbd.c | 12 +++++++++--- > > tests/test-aio.c | 12 +++++++++++- > > tests/test-thread-pool.c | 10 +++++++++- > > tests/test-throttle.c | 12 +++++++++++- > > vl.c | 7 +++++-- > > 12 files changed, 99 insertions(+), 21 deletions(-) > > > > diff --git a/async.c b/async.c > > index a99e7f6..d4b6687 100644 > > --- a/async.c > > +++ b/async.c > > @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) > > aio_notify(opaque); > > } > > > > -AioContext *aio_context_new(void) > > +int aio_context_new(AioContext **context, Error **errp) > > { > > + int ret; > > AioContext *ctx; > > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > > + ret = event_notifier_init(&ctx->notifier, false); > > + if (ret < 0) { > > + g_source_destroy(&ctx->source); > > + error_setg_errno(errp, -ret, "event_notifier_init failed"); > > aio_context_new does not seems to be guest triggered so it may be actually correct > to bake an error message in it. I don't have enough AIO knowledge to juge this. > > However your current error message: "event_notifier_init failed" look more like > a trace than an actual QEMU error message. > > Please grep the code for example error messages: they are usually more plaintext > than this one > > Also switching to returning a -errno make the caller's code convoluted. > Maybe returning NULL would be enough. > > I see further in the patch that the return code is not really used on the > top most level maybe it's a hint that return NULL here would be better. I though a bit more about this. You could just do if (ret < 0) { g_source_destroy(&ctx->source); error_setg_errno(errp, -ret, "event_notifier_init failed"); errno = -ret; return NULL; } when the test fail. This way you have both a straightforward return path _and_ a regular errno usage to propagate it to the callers if you need so. > > > + return ret; > > + } > > + aio_set_event_notifier(ctx, &ctx->notifier, > > + (EventNotifierHandler *) > > + event_notifier_test_and_clear); > > iothread->stopping = false; > > - iothread->ctx = aio_context_new(); > > + ret = aio_context_new(&iothread->ctx, &local_error); > > + if (ret < 0) { > > > + errno = -ret; > You don't seem to reuse errno further down in the code. > > > gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > > - qemu_aio_context = aio_context_new(); > > + ret = aio_context_new(&qemu_aio_context, &local_error); > > + if (ret < 0) { > > > + errno = -ret; > > Same here errno does not seems used by error propagate. > > Also you seems to leak gpollfds but probably you count > on the fact that the kernel will collect it for you > because QEMU will die. > > I think you could move down the gpollfds = g_array_new > after the end of the test. > > > + error_propagate(errp, local_error); > > + return ret; > > + } > > + > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_error); > > + if (ret < 0) { > > + errno = -ret; > > + error_report("%s", error_get_pretty(local_error)); > > + error_free(local_error); > > + error_exit("qemu_init_main_loop failed"); > > + } > > + > > bdrv_init(); > > if (argc < 2) { > > error_exit("Not enough arguments"); > > diff --git a/qemu-io.c b/qemu-io.c > > index 33c96c4..f10dab5 100644 > > --- a/qemu-io.c > > +++ b/qemu-io.c > > @@ -387,8 +387,10 @@ int main(int argc, char **argv) > > { NULL, 0, NULL, 0 } > > }; > > int c; > > + int ret; > > int opt_index = 0; > > int flags = BDRV_O_UNMAP; > > + Error *local_error = NULL; > > > > #ifdef CONFIG_POSIX > > signal(SIGPIPE, SIG_IGN); > > @@ -454,7 +456,13 @@ int main(int argc, char **argv) > > exit(1); > > } > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_error); > > + if (ret < 0) { > > + error_report("%s", error_get_pretty(local_error)); > > + error_report("qemu_init_main_loop failed"); > > + error_free(local_error); > > + return ret; > > + } > > bdrv_init(); > > > > /* initialize commands */ > > diff --git a/qemu-nbd.c b/qemu-nbd.c > > index 9bc152e..4ba9635 100644 > > --- a/qemu-nbd.c > > +++ b/qemu-nbd.c > > @@ -498,13 +498,13 @@ int main(int argc, char **argv) > > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, > > &local_err); > > if (local_err) { > > - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > error_get_pretty(local_err)); > > } > > if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > > !(flags & BDRV_O_UNMAP)) { > > errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " > > - "without setting discard operation to unmap"); > > + "without setting discard operation to unmap"); > > } > > break; > > case 'b': > > @@ -674,7 +674,13 @@ int main(int argc, char **argv) > > snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > > } > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_err); > > + if (ret < 0) { > > + errno = -ret; > > + error_report("%s", error_get_pretty(local_err)); > > + error_free(local_err); > > + err(EXIT_FAILURE, "qemu_init_main_loop failed"); > > + } > > bdrv_init(); > > atexit(bdrv_close_all); > > > > diff --git a/tests/test-aio.c b/tests/test-aio.c > > index c6a8713..a9c9073 100644 >
Hi Benoit, thanks for reviewing and replying. On Mon, Sep 15, 2014 at 09:03:18PM +0200, Benoît Canet wrote: > The Sunday 14 Sep 2014 à 13:23:13 (+0300), Chrysostomos Nanakos wrote : > > If event_notifier_init fails QEMU exits without printing > > any error information to the user. This commit adds an error > > message on failure: > > > > # qemu [...] > > qemu: event_notifier_init failed: Too many open files in system > > qemu: qemu_init_main_loop failed > > > > Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> > > --- > > async.c | 19 +++++++++++++------ > > include/block/aio.h | 2 +- > > include/qemu/main-loop.h | 2 +- > > iothread.c | 12 +++++++++++- > > main-loop.c | 11 +++++++++-- > > qemu-img.c | 11 ++++++++++- > > qemu-io.c | 10 +++++++++- > > qemu-nbd.c | 12 +++++++++--- > > tests/test-aio.c | 12 +++++++++++- > > tests/test-thread-pool.c | 10 +++++++++- > > tests/test-throttle.c | 12 +++++++++++- > > vl.c | 7 +++++-- > > 12 files changed, 99 insertions(+), 21 deletions(-) > > > > diff --git a/async.c b/async.c > > index a99e7f6..d4b6687 100644 > > --- a/async.c > > +++ b/async.c > > @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) > > aio_notify(opaque); > > } > > > > -AioContext *aio_context_new(void) > > +int aio_context_new(AioContext **context, Error **errp) > > { > > + int ret; > > AioContext *ctx; > > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > > + ret = event_notifier_init(&ctx->notifier, false); > > + if (ret < 0) { > > + g_source_destroy(&ctx->source); > > + error_setg_errno(errp, -ret, "event_notifier_init failed"); > > aio_context_new does not seems to be guest triggered so it may be actually correct > to bake an error message in it. I don't have enough AIO knowledge to juge this. Mean either but I am pretty sure that aio_context_new is not guest triggered. > > However your current error message: "event_notifier_init failed" look more like > a trace than an actual QEMU error message. > > Please grep the code for example error messages: they are usually more plaintext > than this one Yes you are right, I will try to find something more descriptive. > > Also switching to returning a -errno make the caller's code convoluted. > Maybe returning NULL would be enough. > > I see further in the patch that the return code is not really used on the > top most level maybe it's a hint that return NULL here would be better. That was my second approach but I thought returning errno might be used by the caller. Returning NULL maybe is the way to go then. > > > + return ret; > > + } > > + aio_set_event_notifier(ctx, &ctx->notifier, > > + (EventNotifierHandler *) > > + event_notifier_test_and_clear); > > iothread->stopping = false; > > - iothread->ctx = aio_context_new(); > > + ret = aio_context_new(&iothread->ctx, &local_error); > > + if (ret < 0) { > > > + errno = -ret; > You don't seem to reuse errno further down in the code. > > > gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > > - qemu_aio_context = aio_context_new(); > > + ret = aio_context_new(&qemu_aio_context, &local_error); > > + if (ret < 0) { > > > + errno = -ret; > > Same here errno does not seems used by error propagate. > > Also you seems to leak gpollfds but probably you count > on the fact that the kernel will collect it for you > because QEMU will die. > > I think you could move down the gpollfds = g_array_new > after the end of the test. I am leaking it for sure, I'll move it after the test. Thanks! If anyone agrees I'll resend the patch with the proposed changes. Regards, Chrysostomos. > > > + error_propagate(errp, local_error); > > + return ret; > > + } > > + > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_error); > > + if (ret < 0) { > > + errno = -ret; > > + error_report("%s", error_get_pretty(local_error)); > > + error_free(local_error); > > + error_exit("qemu_init_main_loop failed"); > > + } > > + > > bdrv_init(); > > if (argc < 2) { > > error_exit("Not enough arguments"); > > diff --git a/qemu-io.c b/qemu-io.c > > index 33c96c4..f10dab5 100644 > > --- a/qemu-io.c > > +++ b/qemu-io.c > > @@ -387,8 +387,10 @@ int main(int argc, char **argv) > > { NULL, 0, NULL, 0 } > > }; > > int c; > > + int ret; > > int opt_index = 0; > > int flags = BDRV_O_UNMAP; > > + Error *local_error = NULL; > > > > #ifdef CONFIG_POSIX > > signal(SIGPIPE, SIG_IGN); > > @@ -454,7 +456,13 @@ int main(int argc, char **argv) > > exit(1); > > } > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_error); > > + if (ret < 0) { > > + error_report("%s", error_get_pretty(local_error)); > > + error_report("qemu_init_main_loop failed"); > > + error_free(local_error); > > + return ret; > > + } > > bdrv_init(); > > > > /* initialize commands */ > > diff --git a/qemu-nbd.c b/qemu-nbd.c > > index 9bc152e..4ba9635 100644 > > --- a/qemu-nbd.c > > +++ b/qemu-nbd.c > > @@ -498,13 +498,13 @@ int main(int argc, char **argv) > > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, > > &local_err); > > if (local_err) { > > - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > error_get_pretty(local_err)); > > } > > if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > > !(flags & BDRV_O_UNMAP)) { > > errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " > > - "without setting discard operation to unmap"); > > + "without setting discard operation to unmap"); > > } > > break; > > case 'b': > > @@ -674,7 +674,13 @@ int main(int argc, char **argv) > > snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > > } > > > > - qemu_init_main_loop(); > > + ret = qemu_init_main_loop(&local_err); > > + if (ret < 0) { > > + errno = -ret; > > + error_report("%s", error_get_pretty(local_err)); > > + error_free(local_err); > > + err(EXIT_FAILURE, "qemu_init_main_loop failed"); > > + } > > bdrv_init(); > > atexit(bdrv_close_all); > > > > diff --git a/tests/test-aio.c b/tests/test-aio.c > > index c6a8713..a9c9073 100644
On Mon, Sep 15, 2014 at 09:59:00PM +0200, Benoît Canet wrote: > The Monday 15 Sep 2014 à 21:03:18 (+0200), Benoît Canet wrote : > > The Sunday 14 Sep 2014 à 13:23:13 (+0300), Chrysostomos Nanakos wrote : > > > If event_notifier_init fails QEMU exits without printing > > > any error information to the user. This commit adds an error > > > message on failure: > > > > > > # qemu [...] > > > qemu: event_notifier_init failed: Too many open files in system > > > qemu: qemu_init_main_loop failed > > > > > > Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> > > > --- > > > async.c | 19 +++++++++++++------ > > > include/block/aio.h | 2 +- > > > include/qemu/main-loop.h | 2 +- > > > iothread.c | 12 +++++++++++- > > > main-loop.c | 11 +++++++++-- > > > qemu-img.c | 11 ++++++++++- > > > qemu-io.c | 10 +++++++++- > > > qemu-nbd.c | 12 +++++++++--- > > > tests/test-aio.c | 12 +++++++++++- > > > tests/test-thread-pool.c | 10 +++++++++- > > > tests/test-throttle.c | 12 +++++++++++- > > > vl.c | 7 +++++-- > > > 12 files changed, 99 insertions(+), 21 deletions(-) > > > > > > diff --git a/async.c b/async.c > > > index a99e7f6..d4b6687 100644 > > > --- a/async.c > > > +++ b/async.c > > > @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) > > > aio_notify(opaque); > > > } > > > > > > -AioContext *aio_context_new(void) > > > +int aio_context_new(AioContext **context, Error **errp) > > > { > > > + int ret; > > > AioContext *ctx; > > > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > > > + ret = event_notifier_init(&ctx->notifier, false); > > > + if (ret < 0) { > > > + g_source_destroy(&ctx->source); > > > + error_setg_errno(errp, -ret, "event_notifier_init failed"); > > > > aio_context_new does not seems to be guest triggered so it may be actually correct > > to bake an error message in it. I don't have enough AIO knowledge to juge this. > > > > However your current error message: "event_notifier_init failed" look more like > > a trace than an actual QEMU error message. > > > > Please grep the code for example error messages: they are usually more plaintext > > than this one > > > > Also switching to returning a -errno make the caller's code convoluted. > > Maybe returning NULL would be enough. > > > > I see further in the patch that the return code is not really used on the > > top most level maybe it's a hint that return NULL here would be better. > > I though a bit more about this. > > You could just do > > if (ret < 0) { > g_source_destroy(&ctx->source); > error_setg_errno(errp, -ret, "event_notifier_init failed"); > errno = -ret; > return NULL; > } > > when the test fail. > > This way you have both a straightforward return path _and_ > a regular errno usage to propagate it to the callers if you need so. > I agree, it seems to be much cleaner and straightforward. Thanks again. > > > > > + return ret; > > > + } > > > + aio_set_event_notifier(ctx, &ctx->notifier, > > > + (EventNotifierHandler *) > > > + event_notifier_test_and_clear); > > > iothread->stopping = false; > > > - iothread->ctx = aio_context_new(); > > > + ret = aio_context_new(&iothread->ctx, &local_error); > > > + if (ret < 0) { > > > > > + errno = -ret; > > You don't seem to reuse errno further down in the code. > > > > > gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > > > - qemu_aio_context = aio_context_new(); > > > + ret = aio_context_new(&qemu_aio_context, &local_error); > > > + if (ret < 0) { > > > > > + errno = -ret; > > > > Same here errno does not seems used by error propagate. > > > > Also you seems to leak gpollfds but probably you count > > on the fact that the kernel will collect it for you > > because QEMU will die. > > > > I think you could move down the gpollfds = g_array_new > > after the end of the test. > > > > > + error_propagate(errp, local_error); > > > + return ret; > > > + } > > > + > > > > > > - qemu_init_main_loop(); > > > + ret = qemu_init_main_loop(&local_error); > > > + if (ret < 0) { > > > + errno = -ret; > > > + error_report("%s", error_get_pretty(local_error)); > > > + error_free(local_error); > > > + error_exit("qemu_init_main_loop failed"); > > > + } > > > + > > > bdrv_init(); > > > if (argc < 2) { > > > error_exit("Not enough arguments"); > > > diff --git a/qemu-io.c b/qemu-io.c > > > index 33c96c4..f10dab5 100644 > > > --- a/qemu-io.c > > > +++ b/qemu-io.c > > > @@ -387,8 +387,10 @@ int main(int argc, char **argv) > > > { NULL, 0, NULL, 0 } > > > }; > > > int c; > > > + int ret; > > > int opt_index = 0; > > > int flags = BDRV_O_UNMAP; > > > + Error *local_error = NULL; > > > > > > #ifdef CONFIG_POSIX > > > signal(SIGPIPE, SIG_IGN); > > > @@ -454,7 +456,13 @@ int main(int argc, char **argv) > > > exit(1); > > > } > > > > > > - qemu_init_main_loop(); > > > + ret = qemu_init_main_loop(&local_error); > > > + if (ret < 0) { > > > + error_report("%s", error_get_pretty(local_error)); > > > + error_report("qemu_init_main_loop failed"); > > > + error_free(local_error); > > > + return ret; > > > + } > > > bdrv_init(); > > > > > > /* initialize commands */ > > > diff --git a/qemu-nbd.c b/qemu-nbd.c > > > index 9bc152e..4ba9635 100644 > > > --- a/qemu-nbd.c > > > +++ b/qemu-nbd.c > > > @@ -498,13 +498,13 @@ int main(int argc, char **argv) > > > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, > > > &local_err); > > > if (local_err) { > > > - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > > > error_get_pretty(local_err)); > > > } > > > if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > > > !(flags & BDRV_O_UNMAP)) { > > > errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " > > > - "without setting discard operation to unmap"); > > > + "without setting discard operation to unmap"); > > > } > > > break; > > > case 'b': > > > @@ -674,7 +674,13 @@ int main(int argc, char **argv) > > > snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > > > } > > > > > > - qemu_init_main_loop(); > > > + ret = qemu_init_main_loop(&local_err); > > > + if (ret < 0) { > > > + errno = -ret; > > > + error_report("%s", error_get_pretty(local_err)); > > > + error_free(local_err); > > > + err(EXIT_FAILURE, "qemu_init_main_loop failed"); > > > + } > > > bdrv_init(); > > > atexit(bdrv_close_all); > > > > > > diff --git a/tests/test-aio.c b/tests/test-aio.c > > > index c6a8713..a9c9073 100644 > >
On Mon, Sep 15, 2014 at 11:44:03PM +0300, Chrysostomos Nanakos wrote: > Hi Benoit, > thanks for reviewing and replying. > > On Mon, Sep 15, 2014 at 09:03:18PM +0200, Benoît Canet wrote: > > The Sunday 14 Sep 2014 à 13:23:13 (+0300), Chrysostomos Nanakos wrote : > > > If event_notifier_init fails QEMU exits without printing > > > any error information to the user. This commit adds an error > > > message on failure: > > > > > > # qemu [...] > > > qemu: event_notifier_init failed: Too many open files in system > > > qemu: qemu_init_main_loop failed > > > > > > Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> > > > --- > > > async.c | 19 +++++++++++++------ > > > include/block/aio.h | 2 +- > > > include/qemu/main-loop.h | 2 +- > > > iothread.c | 12 +++++++++++- > > > main-loop.c | 11 +++++++++-- > > > qemu-img.c | 11 ++++++++++- > > > qemu-io.c | 10 +++++++++- > > > qemu-nbd.c | 12 +++++++++--- > > > tests/test-aio.c | 12 +++++++++++- > > > tests/test-thread-pool.c | 10 +++++++++- > > > tests/test-throttle.c | 12 +++++++++++- > > > vl.c | 7 +++++-- > > > 12 files changed, 99 insertions(+), 21 deletions(-) > > > > > > diff --git a/async.c b/async.c > > > index a99e7f6..d4b6687 100644 > > > --- a/async.c > > > +++ b/async.c > > > @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) > > > aio_notify(opaque); > > > } > > > > > > -AioContext *aio_context_new(void) > > > +int aio_context_new(AioContext **context, Error **errp) > > > { > > > + int ret; > > > AioContext *ctx; > > > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > > > + ret = event_notifier_init(&ctx->notifier, false); > > > + if (ret < 0) { > > > + g_source_destroy(&ctx->source); > > > + error_setg_errno(errp, -ret, "event_notifier_init failed"); > > > > aio_context_new does not seems to be guest triggered so it may be actually correct > > to bake an error message in it. I don't have enough AIO knowledge to juge this. > > Mean either but I am pretty sure that aio_context_new is not guest triggered. It is not guest-triggerable. > > > > Also switching to returning a -errno make the caller's code convoluted. > > Maybe returning NULL would be enough. > > > > I see further in the patch that the return code is not really used on the > > top most level maybe it's a hint that return NULL here would be better. > > That was my second approach but I thought returning errno might be used by the > caller. Returning NULL maybe is the way to go then. I agree, AioContext *aio_context_new(Error **errp) is simpler. The callers don't need to know the errno because they don't act on its value, just use error_setg_errno() so the error message captures it. > > > > > + return ret; > > > + } > > > + aio_set_event_notifier(ctx, &ctx->notifier, > > > + (EventNotifierHandler *) > > > + event_notifier_test_and_clear); > > > iothread->stopping = false; > > > - iothread->ctx = aio_context_new(); > > > + ret = aio_context_new(&iothread->ctx, &local_error); > > > + if (ret < 0) { > > > > > + errno = -ret; > > You don't seem to reuse errno further down in the code. Please don't use errno unless the function already sets it. QEMU almost never uses errno. > > > gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > > > - qemu_aio_context = aio_context_new(); > > > + ret = aio_context_new(&qemu_aio_context, &local_error); > > > + if (ret < 0) { > > > > > + errno = -ret; > > > > Same here errno does not seems used by error propagate. > > > > Also you seems to leak gpollfds but probably you count > > on the fact that the kernel will collect it for you > > because QEMU will die. > > > > I think you could move down the gpollfds = g_array_new > > after the end of the test. > > I am leaking it for sure, I'll move it after the test. Thanks! > > If anyone agrees I'll resend the patch with the proposed changes. Yes, please. Stefan
On Sun, Sep 14, 2014 at 01:23:13PM +0300, Chrysostomos Nanakos wrote: > @@ -63,10 +64,19 @@ static void iothread_instance_finalize(Object *obj) > > static void iothread_complete(UserCreatable *obj, Error **errp) > { > + int ret; > + Error *local_error = NULL; > IOThread *iothread = IOTHREAD(obj); > > iothread->stopping = false; > - iothread->ctx = aio_context_new(); > + ret = aio_context_new(&iothread->ctx, &local_error); > + if (ret < 0) { > + errno = -ret; > + error_report("%s", error_get_pretty(local_error)); > + error_report("iothread_complete failed"); > + error_free(local_error); > + exit(1); > + } Why use error_report()? The function takes an errp argument so local_error should be propagated. > @@ -2893,7 +2895,14 @@ int main(int argc, char **argv) > error_set_progname(argv[0]); > qemu_init_exec_dir(argv[0]); > > - qemu_init_main_loop(); > + ret = qemu_init_main_loop(&local_error); > + if (ret < 0) { > + errno = -ret; > + error_report("%s", error_get_pretty(local_error)); > + error_free(local_error); > + error_exit("qemu_init_main_loop failed"); > + } IMO it's okay to leak local_error here: error_exit("%s", error_get_pretty(local_error)); qemu_init_main_loop failed is not a meaningful error message, please drop it and keep just the detailed message.
diff --git a/async.c b/async.c index a99e7f6..d4b6687 100644 --- a/async.c +++ b/async.c @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque) aio_notify(opaque); } -AioContext *aio_context_new(void) +int aio_context_new(AioContext **context, Error **errp) { + int ret; AioContext *ctx; ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); + ret = event_notifier_init(&ctx->notifier, false); + if (ret < 0) { + g_source_destroy(&ctx->source); + error_setg_errno(errp, -ret, "event_notifier_init failed"); + return ret; + } + aio_set_event_notifier(ctx, &ctx->notifier, + (EventNotifierHandler *) + event_notifier_test_and_clear); ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); ctx->thread_pool = NULL; qemu_mutex_init(&ctx->bh_lock); rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx); - event_notifier_init(&ctx->notifier, false); - aio_set_event_notifier(ctx, &ctx->notifier, - (EventNotifierHandler *) - event_notifier_test_and_clear); timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx); - return ctx; + *context = ctx; + return 0; } void aio_context_ref(AioContext *ctx) diff --git a/include/block/aio.h b/include/block/aio.h index 4603c0f..fca4639 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -99,7 +99,7 @@ void aio_set_dispatching(AioContext *ctx, bool dispatching); * They also provide bottom halves, a service to execute a piece of code * as soon as possible. */ -AioContext *aio_context_new(void); +int aio_context_new(AioContext **context, Error **errp); /** * aio_context_ref: diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 6f0200a..62c68c0 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -42,7 +42,7 @@ * * In the case of QEMU tools, this will also start/initialize timers. */ -int qemu_init_main_loop(void); +int qemu_init_main_loop(Error **errp); /** * main_loop_wait: Run one iteration of the main loop. diff --git a/iothread.c b/iothread.c index d9403cf..a18c455 100644 --- a/iothread.c +++ b/iothread.c @@ -17,6 +17,7 @@ #include "block/aio.h" #include "sysemu/iothread.h" #include "qmp-commands.h" +#include "qemu/error-report.h" #define IOTHREADS_PATH "/objects" @@ -63,10 +64,19 @@ static void iothread_instance_finalize(Object *obj) static void iothread_complete(UserCreatable *obj, Error **errp) { + int ret; + Error *local_error = NULL; IOThread *iothread = IOTHREAD(obj); iothread->stopping = false; - iothread->ctx = aio_context_new(); + ret = aio_context_new(&iothread->ctx, &local_error); + if (ret < 0) { + errno = -ret; + error_report("%s", error_get_pretty(local_error)); + error_report("iothread_complete failed"); + error_free(local_error); + exit(1); + } iothread->thread_id = -1; qemu_mutex_init(&iothread->init_done_lock); diff --git a/main-loop.c b/main-loop.c index 3cc79f8..b2ef5fc 100644 --- a/main-loop.c +++ b/main-loop.c @@ -126,10 +126,11 @@ void qemu_notify_event(void) static GArray *gpollfds; -int qemu_init_main_loop(void) +int qemu_init_main_loop(Error **errp) { int ret; GSource *src; + Error *local_error = NULL; init_clocks(); @@ -139,7 +140,13 @@ int qemu_init_main_loop(void) } gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); - qemu_aio_context = aio_context_new(); + ret = aio_context_new(&qemu_aio_context, &local_error); + if (ret < 0) { + errno = -ret; + error_propagate(errp, local_error); + return ret; + } + src = aio_get_g_source(qemu_aio_context); g_source_attach(src, NULL); g_source_unref(src); diff --git a/qemu-img.c b/qemu-img.c index 91d1ac3..789c512 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2879,7 +2879,9 @@ int main(int argc, char **argv) { const img_cmd_t *cmd; const char *cmdname; + Error *local_error = NULL; int c; + int ret; static const struct option long_options[] = { {"help", no_argument, 0, 'h'}, {"version", no_argument, 0, 'v'}, @@ -2893,7 +2895,14 @@ int main(int argc, char **argv) error_set_progname(argv[0]); qemu_init_exec_dir(argv[0]); - qemu_init_main_loop(); + ret = qemu_init_main_loop(&local_error); + if (ret < 0) { + errno = -ret; + error_report("%s", error_get_pretty(local_error)); + error_free(local_error); + error_exit("qemu_init_main_loop failed"); + } + bdrv_init(); if (argc < 2) { error_exit("Not enough arguments"); diff --git a/qemu-io.c b/qemu-io.c index 33c96c4..f10dab5 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -387,8 +387,10 @@ int main(int argc, char **argv) { NULL, 0, NULL, 0 } }; int c; + int ret; int opt_index = 0; int flags = BDRV_O_UNMAP; + Error *local_error = NULL; #ifdef CONFIG_POSIX signal(SIGPIPE, SIG_IGN); @@ -454,7 +456,13 @@ int main(int argc, char **argv) exit(1); } - qemu_init_main_loop(); + ret = qemu_init_main_loop(&local_error); + if (ret < 0) { + error_report("%s", error_get_pretty(local_error)); + error_report("qemu_init_main_loop failed"); + error_free(local_error); + return ret; + } bdrv_init(); /* initialize commands */ diff --git a/qemu-nbd.c b/qemu-nbd.c index 9bc152e..4ba9635 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -498,13 +498,13 @@ int main(int argc, char **argv) BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, &local_err); if (local_err) { - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", error_get_pretty(local_err)); } if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && !(flags & BDRV_O_UNMAP)) { errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " - "without setting discard operation to unmap"); + "without setting discard operation to unmap"); } break; case 'b': @@ -674,7 +674,13 @@ int main(int argc, char **argv) snprintf(sockpath, 128, SOCKET_PATH, basename(device)); } - qemu_init_main_loop(); + ret = qemu_init_main_loop(&local_err); + if (ret < 0) { + errno = -ret; + error_report("%s", error_get_pretty(local_err)); + error_free(local_err); + err(EXIT_FAILURE, "qemu_init_main_loop failed"); + } bdrv_init(); atexit(bdrv_close_all); diff --git a/tests/test-aio.c b/tests/test-aio.c index c6a8713..a9c9073 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -14,6 +14,7 @@ #include "block/aio.h" #include "qemu/timer.h" #include "qemu/sockets.h" +#include "qemu/error-report.h" static AioContext *ctx; @@ -810,11 +811,20 @@ static void test_source_timer_schedule(void) int main(int argc, char **argv) { + int ret; + Error *local_error; GSource *src; init_clocks(); - ctx = aio_context_new(); + ret = aio_context_new(&ctx, &local_error); + if (ret < 0) { + errno = -ret; + error_report("%s", error_get_pretty(local_error)); + error_report("aio_context_new failed"); + error_free(local_error); + exit(1); + } src = aio_get_g_source(ctx); g_source_attach(src, NULL); g_source_unref(src); diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c index f40b7fc..b9114f0 100644 --- a/tests/test-thread-pool.c +++ b/tests/test-thread-pool.c @@ -4,6 +4,7 @@ #include "block/thread-pool.h" #include "block/block.h" #include "qemu/timer.h" +#include "qemu/error-report.h" static AioContext *ctx; static ThreadPool *pool; @@ -205,10 +206,17 @@ static void test_cancel(void) int main(int argc, char **argv) { int ret; + Error *local_error; init_clocks(); - ctx = aio_context_new(); + ret = aio_context_new(&ctx, &local_error); + if (ret < 0) { + error_report("%s", error_get_pretty(local_error)); + error_report("aio_context_new failed"); + error_free(local_error); + exit(1); + } pool = aio_get_thread_pool(ctx); g_test_init(&argc, &argv, NULL); diff --git a/tests/test-throttle.c b/tests/test-throttle.c index 000ae31..4f4bff3 100644 --- a/tests/test-throttle.c +++ b/tests/test-throttle.c @@ -14,6 +14,7 @@ #include <math.h> #include "block/aio.h" #include "qemu/throttle.h" +#include "qemu/error-report.h" static AioContext *ctx; static LeakyBucket bkt; @@ -491,11 +492,20 @@ static void test_accounting(void) int main(int argc, char **argv) { + int ret; GSource *src; + Error *local_error; init_clocks(); - ctx = aio_context_new(); + ret = aio_context_new(&ctx, &local_error); + if (ret < 0) { + errno = -ret; + error_report("%s", error_get_pretty(local_error)); + error_report("aio_context_new failed"); + error_free(local_error); + exit(1); + } src = aio_get_g_source(ctx); g_source_attach(src, NULL); g_source_unref(src); diff --git a/vl.c b/vl.c index 9c9acf5..e6e9ae7 100644 --- a/vl.c +++ b/vl.c @@ -2968,6 +2968,7 @@ int main(int argc, char **argv, char **envp) ram_addr_t maxram_size = default_ram_size; uint64_t ram_slots = 0; FILE *vmstate_dump_file = NULL; + Error *main_loop_err = NULL; atexit(qemu_run_exit_notifiers); error_set_progname(argv[0]); @@ -3998,8 +3999,10 @@ int main(int argc, char **argv, char **envp) os_daemonize(); - if (qemu_init_main_loop()) { - fprintf(stderr, "qemu_init_main_loop failed\n"); + if (qemu_init_main_loop(&main_loop_err) < 0) { + error_report("%s", error_get_pretty(main_loop_err)); + error_report("qemu_init_main_loop failed"); + error_free(main_loop_err); exit(1); }
If event_notifier_init fails QEMU exits without printing any error information to the user. This commit adds an error message on failure: # qemu [...] qemu: event_notifier_init failed: Too many open files in system qemu: qemu_init_main_loop failed Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> --- async.c | 19 +++++++++++++------ include/block/aio.h | 2 +- include/qemu/main-loop.h | 2 +- iothread.c | 12 +++++++++++- main-loop.c | 11 +++++++++-- qemu-img.c | 11 ++++++++++- qemu-io.c | 10 +++++++++- qemu-nbd.c | 12 +++++++++--- tests/test-aio.c | 12 +++++++++++- tests/test-thread-pool.c | 10 +++++++++- tests/test-throttle.c | 12 +++++++++++- vl.c | 7 +++++-- 12 files changed, 99 insertions(+), 21 deletions(-)