Re: [Qemu-block] [PATCH v3 1/2] linux-aio: properly bubble up errors from initialization

[Nishanth Aravamudan]

On 22.06.2018 [10:21:19 +0800], Fam Zheng wrote:
> On Thu, 06/21 15:21, Nishanth Aravamudan wrote:
> > laio_init() can fail for a couple of reasons, which will lead to a NULL
> > pointer dereference in laio_attach_aio_context().
> > 
> > To solve this, add a aio_setup_linux_aio() function which is called
> > early in raw_open_common. If this fails, propagate the error up. The
> > signature of aio_get_linux_aio() was not modified, because it seems
> > preferable to return the actual errno from the possible failing
> > initialization calls.
> > 
> > Add an assert that aio_get_linux_aio() cannot return NULL.
> > 
> > Signed-off-by: Nishanth Aravamudan <address@hidden>
> > ---
> > Changes from v2 -> v3 (thanks to Eric Blake and Kevin Wolf for review):
> > 
> > Use a boolean false rather than 0 in assignment to use_linux_aio.
> > Drop ending '.' from error_report() calls.
> > Fix typo in commit message (propogates -> propagates).
> > Move aio_setup_linux_aio call to raw_open_common.
> > 
> > Changes from v1 -> v2 (thanks to Kevin Wolf for review):
> > 
> > Rather than affect virtio-scsi/blk at all, make all the changes internal
> > to file-posix.c. Thanks to Kevin Wolf for the suggested change.
> > 
> >  block/file-posix.c      | 17 ++++++++++++-----
> >  block/linux-aio.c       | 15 ++++++++++-----
> >  include/block/aio.h     |  3 +++
> >  include/block/raw-aio.h |  2 +-
> >  stubs/linux-aio.c       |  2 +-
> >  util/async.c            | 16 +++++++++++++---
> >  6 files changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 07bb061fe4..6a1714d4a8 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -545,11 +545,18 @@ static int raw_open_common(BlockDriverState *bs, 
> > QDict *options,
> >  
> >  #ifdef CONFIG_LINUX_AIO
> >       /* Currently Linux does AIO only for files opened with O_DIRECT */
> > -    if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
> > -        error_setg(errp, "aio=native was specified, but it requires "
> > -                         "cache.direct=on, which was not specified.");
> > -        ret = -EINVAL;
> > -        goto fail;
> > +    if (s->use_linux_aio) {
> > +        if (!(s->open_flags & O_DIRECT)) {
> > +            error_setg(errp, "aio=native was specified, but it requires "
> > +                             "cache.direct=on, which was not specified.");
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +        ret = aio_setup_linux_aio(bdrv_get_aio_context(bs));
> > +        if (ret != 0) {
> > +            error_setg(errp, "Unable to setup native AIO context.");
> > +            goto fail;
> > +        }
> >      }
> >  #else
> >      if (s->use_linux_aio) {
> > diff --git a/block/linux-aio.c b/block/linux-aio.c
> > index 88b8d55ec7..4d799f85fe 100644
> > --- a/block/linux-aio.c
> > +++ b/block/linux-aio.c
> > @@ -470,28 +470,33 @@ void laio_attach_aio_context(LinuxAioState *s, 
> > AioContext *new_context)
> >                             qemu_laio_poll_cb);
> >  }
> >  
> > -LinuxAioState *laio_init(void)
> > +int laio_init(LinuxAioState **linux_aio)
> >  {
> > +    int rc;
> >      LinuxAioState *s;
> >  
> >      s = g_malloc0(sizeof(*s));
> > -    if (event_notifier_init(&s->e, false) < 0) {
> > +    rc = event_notifier_init(&s->e, false);
> > +    if (rc < 0) {
> 
> It would be nice if the error message could distinguish this error...
> 
> >          goto out_free_state;
> >      }
> >  
> > -    if (io_setup(MAX_EVENTS, &s->ctx) != 0) {
> > +    rc = io_setup(MAX_EVENTS, &s->ctx);
> > +    if (rc != 0) {
> 
> ... from this one. And it also makes sense to propagate the errno back with
> error_setg_errno.
> 
> To do this, add an "Error **errp" parameter to the function and you
> can keep the return type (LinuxAioState *).

Thank you for this suggestion! I will send out a v4 with this and your
other feedback integrated.

-Nish