[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] linux-aio: properly bubble up errors fro
From: |
Nishanth Aravamudan |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] linux-aio: properly bubble up errors from initialization |
Date: |
Fri, 22 Jun 2018 10:12:07 -0700 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
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