Re: [Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Stefan Hajnoczi]

On Wed, Sep 17, 2014 at 04:48:34PM +0300, Chrysostomos Nanakos wrote:
> diff --git a/iothread.c b/iothread.c
> index d9403cf..6e394a0 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"
>  
> @@ -53,6 +54,9 @@ static void iothread_instance_finalize(Object *obj)
>  {
>      IOThread *iothread = IOTHREAD(obj);
>  
> +    if (!iothread->ctx) {
> +        return;
> +    }
>      iothread->stopping = true;
>      aio_notify(iothread->ctx);
>      qemu_thread_join(&iothread->thread);
> @@ -63,10 +67,15 @@ static void iothread_instance_finalize(Object *obj)
>  
>  static void iothread_complete(UserCreatable *obj, Error **errp)
>  {
> +    Error *local_error = NULL;
>      IOThread *iothread = IOTHREAD(obj);
>  
>      iothread->stopping = false;
> -    iothread->ctx = aio_context_new();
> +    iothread->ctx = aio_context_new(&local_error);
> +    if (!iothread->ctx) {
> +        error_propagate(errp, local_error);
> +        return;
> +    }
>      iothread->thread_id = -1;

Please move this under ->stopping = false so that ->thread_id is
initialized.  That way qmp_query_iothreads() will display thread_id -1
for failed IOThread objects instead of an uninitialized value.

Normally QEMU should exit or the IOThread object should be deleted
before anyone has a chance to call qmp_query_iothreads() but you never
know so let's get the lifecycle right...

> diff --git a/main-loop.c b/main-loop.c
> index 3cc79f8..2f83d80 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();
>  
> @@ -138,8 +139,12 @@ int qemu_init_main_loop(void)
>          return ret;
>      }
>  
> +    qemu_aio_context = aio_context_new(&local_error);
> +    if (!qemu_aio_context) {
> +        error_propagate(errp, local_error);
> +        return -1;

This function returns -errno so -1 would be -EPERM.  Please use an
actual errno constant like -EMFILE.

> diff --git a/tests/test-aio.c b/tests/test-aio.c
> index c6a8713..029716f 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,18 @@ static void test_source_timer_schedule(void)
>  
>  int main(int argc, char **argv)
>  {
> +    Error *local_error;

local_error = NULL

It must be NULL otherwise error_setg() will read uninitialized memory
(it checks that the error hasn't been set yet because Error objects can
only be set once).

> diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
> index f40b7fc..57c0836 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;

Same.

> diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> index 000ae31..42141a0 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;
> @@ -492,10 +493,17 @@ static void test_accounting(void)
>  int main(int argc, char **argv)
>  {
>      GSource *src;
> +    Error *local_error;

Same.

pgpLo3tTlcAwx.pgp
Description: PGP signature