Re: [Qemu-devel] [PATCH] [RFC] aio: properly bubble up errors from initialization

2018-06-14 Thread no-reply
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180614232119.31669-1-naravamu...@digitalocean.com
Subject: [Qemu-devel] [PATCH] [RFC] aio: properly bubble up errors from 
initialization

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   
patchew/20180614232119.31669-1-naravamu...@digitalocean.com -> 
patchew/20180614232119.31669-1-naravamu...@digitalocean.com
Switched to a new branch 'test'
e72bf49783 aio: properly bubble up errors from initialization

=== OUTPUT BEGIN ===
Checking PATCH 1/1: aio: properly bubble up errors from initialization...
ERROR: do not use C99 // comments
#146: FILE: block/io.c:2788:
+// XXX: do we need to undo the successful setups if we fail midway?

ERROR: do not use C99 // comments
#157: FILE: block/io.c:2799:
+// XXX: do we need to undo the successful setups if we fail midway?

total: 2 errors, 0 warnings, 258 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-devel] [PATCH] [RFC] aio: properly bubble up errors from initialization

2018-06-14 Thread Nishanth Aravamudan via Qemu-devel
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_linux_aio_setup() path which is called where
aio_get_linux_aio() is called currently, but can propogate errors up.

virtio-block and virtio-scsi call this new function before calling
blk_io_plug() (which eventually calls aio_get_linux_aio). This is
necessary because plug/unplug currently assume they do not fail.

It is trivial to make qemu segfault in my testing. Set
/proc/sys/fs/aio-max-nr to 0 and start a guest with
aio=native,cache=directsync. With this patch, the guest successfully
starts (but obviously isn't using native AIO). Setting aio-max-nr back
up to a reasonable value, AIO contexts are consumed normally.

Signed-off-by: Nishanth Aravamudan 
---
 block/block-backend.c  | 10 ++
 block/file-posix.c | 27 +++
 block/io.c | 27 +++
 block/linux-aio.c  | 15 ++-
 hw/block/virtio-blk.c  |  4 
 hw/scsi/virtio-scsi.c  |  4 
 include/block/aio.h|  3 +++
 include/block/block.h  |  1 +
 include/block/block_int.h  |  1 +
 include/block/raw-aio.h|  2 +-
 include/sysemu/block-backend.h |  1 +
 stubs/linux-aio.c  |  2 +-
 util/async.c   | 14 +++---
 13 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d55c328736..2a18bb2af4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1946,6 +1946,16 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, 
Notifier *notify)
 notifier_list_add(&blk->insert_bs_notifiers, notify);
 }
 
+int blk_io_plug_setup(BlockBackend *blk)
+{
+BlockDriverState *bs = blk_bs(blk);
+
+if (bs) {
+return bdrv_io_plug_setup(bs);
+}
+return 0;
+}
+
 void blk_io_plug(BlockBackend *blk)
 {
 BlockDriverState *bs = blk_bs(blk);
diff --git a/block/file-posix.c b/block/file-posix.c
index 07bb061fe4..adaba7c366 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1665,6 +1665,12 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, 
uint64_t offset,
 type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
 } else if (s->use_linux_aio) {
+int rc;
+rc = aio_linux_aio_setup(bdrv_get_aio_context(bs));
+if (rc != 0) {
+s->use_linux_aio = 0;
+return rc;
+}
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
 assert(qiov->size == bytes);
 return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
@@ -1690,12 +1696,28 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_WRITE);
 }
 
+static int raw_aio_plug_setup(BlockDriverState *bs)
+{
+int rc = 0;
+#ifdef CONFIG_LINUX_AIO
+BDRVRawState *s = bs->opaque;
+if (s->use_linux_aio) {
+rc = aio_linux_aio_setup(bdrv_get_aio_context(bs));
+if (rc != 0) {
+s->use_linux_aio = 0;
+}
+}
+#endif
+return rc;
+}
+
 static void raw_aio_plug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
 BDRVRawState *s = bs->opaque;
 if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
+assert(aio != NULL);
 laio_io_plug(bs, aio);
 }
 #endif
@@ -1707,6 +1729,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
 BDRVRawState *s = bs->opaque;
 if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
+assert(aio != NULL);
 laio_io_unplug(bs, aio);
 }
 #endif
@@ -2599,6 +2622,7 @@ BlockDriver bdrv_file = {
 .bdrv_co_copy_range_from = raw_co_copy_range_from,
 .bdrv_co_copy_range_to  = raw_co_copy_range_to,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3079,6 +3103,7 @@ static BlockDriver bdrv_host_device = {
 .bdrv_co_copy_range_from = raw_co_copy_range_from,
 .bdrv_co_copy_range_to  = raw_co_copy_range_to,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3201,6 +3226,7 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_co_pwritev= raw_co_pwritev,
 .bdrv_aio_flush= raw_aio_flush,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3331,6 +3357,7 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_co_pwritev= raw_co_pwritev,
 .bdrv_aio_flush= raw_aio_flush,
 .bdrv_r