Re: [Qemu-block] [PATCH v2 8/9] quorum: Inline quorum_fifo_aio_cb()

[Paolo Bonzini]

On 22/11/2016 13:24, Kevin Wolf wrote:
> Inlining the function removes some boilerplace code and replaces
> recursion by a simple loop, so the code becomes somewhat easier to
> understand.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  block/quorum.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index c0994dc..52fa806 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -252,30 +252,6 @@ static void quorum_report_bad_acb(QuorumChildRequest 
> *sacb, int ret)
>      quorum_report_bad(type, acb->offset, acb->bytes, sacb->bs->node_name, 
> ret);
>  }
>  
> -static int quorum_fifo_aio_cb(void *opaque, int ret)
> -{
> -    QuorumChildRequest *sacb = opaque;
> -    QuorumAIOCB *acb = sacb->parent;
> -    BDRVQuorumState *s = acb->bs->opaque;
> -
> -    assert(acb->is_read && s->read_pattern == QUORUM_READ_PATTERN_FIFO);
> -
> -    if (ret < 0) {
> -        quorum_report_bad_acb(sacb, ret);
> -
> -        /* We try to read next child in FIFO order if we fail to read */
> -        if (acb->children_read < s->num_children) {
> -            return read_fifo_child(acb);
> -        }
> -    }
> -
> -    acb->vote_ret = ret;
> -
> -    /* FIXME: rewrite failed children if acb->children_read > 1? */
> -
> -    return ret;
> -}
> -
>  static void quorum_report_bad_versions(BDRVQuorumState *s,
>                                         QuorumAIOCB *acb,
>                                         QuorumVoteValue *value)
> @@ -685,12 +661,20 @@ static int read_quorum_children(QuorumAIOCB *acb)
>  static int read_fifo_child(QuorumAIOCB *acb)
>  {
>      BDRVQuorumState *s = acb->bs->opaque;
> -    int n = acb->children_read++;
> -    int ret;
> +    int n, ret;
> +
> +    /* We try to read the next child in FIFO order if we failed to read */
> +    do {
> +        n = acb->children_read++;

acb->children_read is not used outside read_fifo_child, so you can get
rid of it.  I'm not sure if this also means that you can rewrite this
loop as a "for (n = 0; n < s->num_children; n++)", such as by...

> +        acb->qcrs[n].bs = s->children[n]->bs;
> +        ret = bdrv_co_preadv(s->children[n], acb->offset, acb->bytes,
> +                             acb->qiov, 0);

... break-ing out here if ret >= 0.

Paolo

> +        if (ret < 0) {
> +            quorum_report_bad_acb(&acb->qcrs[n], ret);
> +        }
> +    } while (ret < 0 && acb->children_read < s->num_children);
>  
> -    acb->qcrs[n].bs = s->children[n]->bs;
> -    ret = bdrv_co_preadv(s->children[n], acb->offset, acb->bytes, acb->qiov, 
> 0);
> -    ret = quorum_fifo_aio_cb(&acb->qcrs[n], ret);
> +    /* FIXME: rewrite failed children if acb->children_read > 1? */
>  
>      return ret;
>  }
>