Re: [Qemu-devel] [PATCH V10 09/13] quorum: Add quorum_co_get_block_status.

[Max Reitz]

On 28.01.2014 17:52, Benoît Canet wrote:
> From: Benoît Canet <address@hidden>
>
> Signed-off-by: Benoit Canet <address@hidden>
> ---
>   block/quorum.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index a47cd33..9b0718b 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -171,6 +171,22 @@ static int quorum_sha256_compare(QuorumVoteValue *a, 
> QuorumVoteValue *b)
>       return memcmp(a->h, b->h, HASH_LENGTH);
>   }
>   
> +static int quorum_64bits_compare(QuorumVoteValue *a, QuorumVoteValue *b)
> +{
> +    int64_t i = a->l;
> +    int64_t j = b->l;
> +
> +    if (i < j) {
> +        return -1;
> +    }
> +
> +    if (i > j) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
>   static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s,
>                                      BlockDriverState *bs,
>                                      QEMUIOVector *qiov,
> @@ -587,6 +603,56 @@ static void quorum_invalidate_cache(BlockDriverState *bs)
>       }
>   }
>   
> +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> +                                                       int64_t sector_num,
> +                                                       int nb_sectors,
> +                                                       int *pnum)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    QuorumVoteVersion *winner = NULL;
> +    QuorumVotes result_votes, num_votes;
> +    QuorumVoteValue result_value, num_value;
> +    int i, num;
> +    int64_t result = 0;
> +
> +    QLIST_INIT(&result_votes.vote_list);
> +    QLIST_INIT(&num_votes.vote_list);
> +    result_votes.compare = quorum_64bits_compare;
> +    num_votes.compare = quorum_64bits_compare;
> +
> +    for (i = 0; i < s->total; i++) {
> +        result = bdrv_get_block_status(s->bs[i], sector_num, nb_sectors, &num);
> +        /* skip failed requests */
> +        if (result < 0) {
> +            continue;
> +        }
> +        result_value.l = result & BDRV_BLOCK_DATA;
> +        num_value.l = num;
> +        quorum_count_vote(&result_votes, &result_value, i);
> +        quorum_count_vote(&num_votes, &num_value, i);
> +    }
> +
> +    winner = quorum_get_vote_winner(&result_votes);
> +    result = winner->value.l;

Below, you're reading the winning value after checking whether it's 
corresponding votes exceeded the threshold. It doesn't matter in the 
end, but for the sake of uniformity, I'd do it the same way here (i.e., 
move this statement below the if block).

> +    if (winner->vote_count < s->threshold) {
> +        result = -ERANGE;

Is there any specific reason why you're returning -ERANGE here and -EIO 
everywhere else (even in quorum_getlength())?

Max

> +        goto free_exit;
> +    }
> +
> +    winner = quorum_get_vote_winner(&num_votes);
> +    if (winner->vote_count < s->threshold) {
> +        result = -ERANGE;
> +        goto free_exit;
> +    }
> +    *pnum = winner->value.l;
> +
> +free_exit:
> +    quorum_free_vote_list(&result_votes);
> +    quorum_free_vote_list(&num_votes);
> +
> +    return result;
> +}
> +
>   static BlockDriver bdrv_quorum = {
>       .format_name        = "quorum",
>       .protocol_name      = "quorum",
> @@ -598,6 +664,7 @@ static BlockDriver bdrv_quorum = {
>       .bdrv_aio_readv     = quorum_aio_readv,
>       .bdrv_aio_writev    = quorum_aio_writev,
>       .bdrv_invalidate_cache = quorum_invalidate_cache,
> +    .bdrv_co_get_block_status = quorum_co_get_block_status,
>   };
>   
>   static void bdrv_quorum_init(void)