Re: [Qemu-devel] [PATCH V15 03/13] quorum: Add quorum_aio_writev and its dependencies.

[Kevin Wolf <kwolf@redhat.com>]

Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> From: Benoît Canet <benoit@irqsave.net>
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/quorum.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)

Starting with writes before the driver can even open an image is a weird
order to do things in. It also doesn't make the review any easier when
you don't know how things are initialised.

> diff --git a/block/quorum.c b/block/quorum.c
> index 157efdf..81bffdd 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -64,11 +64,115 @@ struct QuorumAIOCB {
>      int vote_ret;
>  };
>  
> +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +    QuorumAIOCB *acb = container_of(blockacb, QuorumAIOCB, common);
> +    BDRVQuorumState *s = acb->bqs;
> +    int i;
> +
> +    /* cancel all callback */

"callbacks"

> +    for (i = 0; i < s->total; i++) {
> +        bdrv_aio_cancel(acb->aios[i].aiocb);
> +    }
> +}

Don't you want to free acb and similar cleanup?

> +
> +static AIOCBInfo quorum_aiocb_info = {
> +    .aiocb_size         = sizeof(QuorumAIOCB),
> +    .cancel             = quorum_aio_cancel,
> +};
> +
> +static void quorum_aio_finalize(QuorumAIOCB *acb)
> +{
> +    BDRVQuorumState *s = acb->bqs;

block/quorum.c: In function 'quorum_aio_finalize':
block/quorum.c:86:22: error: unused variable 's' [-Werror=unused-variable]

> +    int ret = 0;
> +
> +    acb->common.cb(acb->common.opaque, ret);
> +    if (acb->finished) {
> +        *acb->finished = true;
> +    }
> +    g_free(acb->aios);
> +    qemu_aio_release(acb);
> +}
> +
> +static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s,
> +                                   BlockDriverState *bs,
> +                                   QEMUIOVector *qiov,
> +                                   uint64_t sector_num,
> +                                   int nb_sectors,
> +                                   BlockDriverCompletionFunc *cb,
> +                                   void *opaque)
> +{
> +    QuorumAIOCB *acb = qemu_aio_get(&quorum_aiocb_info, bs, cb, opaque);
> +    int i;
> +
> +    acb->bqs = s;

Noticed it only here, but it's really in patch 2 (and should be in
patch 1):

What is acb->bqs good for? Isn't it always the same as
acb->common.bs->opaque?

> +    acb->sector_num = sector_num;
> +    acb->nb_sectors = nb_sectors;
> +    acb->qiov = qiov;
> +    acb->aios = g_new0(QuorumSingleAIOCB, s->total);
> +    acb->count = 0;
> +    acb->success_count = 0;
> +    acb->finished = NULL;
> +    acb->is_read = false;
> +    acb->vote_ret = 0;
> +
> +    for (i = 0; i < s->total; i++) {
> +        acb->aios[i].buf = NULL;
> +        acb->aios[i].ret = 0;
> +        acb->aios[i].parent = acb;
> +    }
>
> +
> +    return acb;
> +}

Kevin