diff mbox

[1/4] quorum: Fix crash in quorum_aio_cb()

Message ID 8ad83e37ec80ac89ee7a30426c4b0f2c02e033eb.1457539274.git.berto@igalia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alberto Garcia March 9, 2016, 4:11 p.m. UTC
quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
an I/O error in a Quorum child. However sacb->aiocb must be
correctly initialized for this to happen. read_quorum_children() and
read_fifo_child() are not doing this, which results in a QEMU crash.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/quorum.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Max Reitz March 9, 2016, 4:54 p.m. UTC | #1
On 09.03.2016 17:11, Alberto Garcia wrote:
> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
> an I/O error in a Quorum child. However sacb->aiocb must be
> correctly initialized for this to happen. read_quorum_children() and
> read_fifo_child() are not doing this, which results in a QEMU crash.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/quorum.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Although I'm wondering whether we could have just used acb->common.bs
instead of sacb->aiocb->bs in quorum_aio_cb(). I guess that sacb->aiocb
is supposed to be equal to &acb->common.

Max
Alberto Garcia March 10, 2016, 10:10 a.m. UTC | #2
On Wed 09 Mar 2016 05:54:55 PM CET, Max Reitz <mreitz@redhat.com> wrote:
> On 09.03.2016 17:11, Alberto Garcia wrote:
>> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
>> an I/O error in a Quorum child. However sacb->aiocb must be
>> correctly initialized for this to happen. read_quorum_children() and
>> read_fifo_child() are not doing this, which results in a QEMU crash.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/quorum.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Although I'm wondering whether we could have just used acb->common.bs
> instead of sacb->aiocb->bs in quorum_aio_cb(). I guess that
> sacb->aiocb is supposed to be equal to &acb->common.

acb->common.bs is the Quorum BDS, sacb->aiocb->bs is the child BDS.

Berto
Max Reitz March 11, 2016, 12:35 p.m. UTC | #3
On 10.03.2016 11:10, Alberto Garcia wrote:
> On Wed 09 Mar 2016 05:54:55 PM CET, Max Reitz <mreitz@redhat.com> wrote:
>> On 09.03.2016 17:11, Alberto Garcia wrote:
>>> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
>>> an I/O error in a Quorum child. However sacb->aiocb must be
>>> correctly initialized for this to happen. read_quorum_children() and
>>> read_fifo_child() are not doing this, which results in a QEMU crash.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>  block/quorum.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> Although I'm wondering whether we could have just used acb->common.bs
>> instead of sacb->aiocb->bs in quorum_aio_cb(). I guess that
>> sacb->aiocb is supposed to be equal to &acb->common.
> 
> acb->common.bs is the Quorum BDS, sacb->aiocb->bs is the child BDS.

You're right, thanks for explaining.

Max
diff mbox

Patch

diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..73ff309 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -648,8 +648,9 @@  static BlockAIOCB *read_quorum_children(QuorumAIOCB *acb)
     }
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_aio_readv(s->children[i]->bs, acb->sector_num, &acb->qcrs[i].qiov,
-                       acb->nb_sectors, quorum_aio_cb, &acb->qcrs[i]);
+        acb->qcrs[i].aiocb = bdrv_aio_readv(s->children[i]->bs, acb->sector_num,
+                                            &acb->qcrs[i].qiov, acb->nb_sectors,
+                                            quorum_aio_cb, &acb->qcrs[i]);
     }
 
     return &acb->common;
@@ -664,9 +665,10 @@  static BlockAIOCB *read_fifo_child(QuorumAIOCB *acb)
     qemu_iovec_init(&acb->qcrs[acb->child_iter].qiov, acb->qiov->niov);
     qemu_iovec_clone(&acb->qcrs[acb->child_iter].qiov, acb->qiov,
                      acb->qcrs[acb->child_iter].buf);
-    bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
-                   &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
-                   quorum_aio_cb, &acb->qcrs[acb->child_iter]);
+    acb->qcrs[acb->child_iter].aiocb =
+        bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
+                       &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
+                       quorum_aio_cb, &acb->qcrs[acb->child_iter]);
 
     return &acb->common;
 }