Re: [Qemu-block] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Tue 22 Sep 2015 09:44:20 AM CEST, Wen Congyang wrote:

> +++ b/block/quorum.c
> @@ -66,6 +66,9 @@ typedef struct QuorumVotes {
>  typedef struct BDRVQuorumState {
>      BlockDriverState **bs; /* children BlockDriverStates */
>      int num_children;      /* children count */
> +    int max_children;      /* The maximum children count, we need to 
> reallocate
> +                            * bs if num_children grows larger than maximum.
> +                            */
>      int threshold;         /* if less than threshold children reads gave the
>                              * same result a quorum error occurs.
>                              */

As you announce in the cover letter of this series, your code depends on
the parents list patch written by Kevin here:

http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04579.html

As you might be aware, and as part of the same series by Kevin,
BDRVQuorumState will no longer hold a list of BlockDriverState but a
list of BdrvChild instead:

https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04571.html

> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +
> +    bdrv_drain(bs);
> +
> +    if (s->num_children == s->max_children) {
> +        if (s->max_children >= INT_MAX) {
> +            error_setg(errp, "Too many children");
> +            return;
> +        }

max_children can never be greater than INT_MAX. Use == instead.

> +        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
> +        s->bs[s->num_children] = NULL;

No need to set the pointer to NULL here, and you are anyway setting the
pointer to the new child a few lines afterwards.

> +        s->max_children++;
> +    }
> +
> +    bdrv_ref(child_bs);
> +    bdrv_attach_child(bs, child_bs, &child_format);
> +    s->bs[s->num_children++] = child_bs;
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    int i;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (s->bs[i] == child_bs) {
> +            break;
> +        }
> +    }
> +
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        if (child->bs == child_bs) {
> +            break;
> +        }
> +    }
> +
> +    /* we have checked it in bdrv_del_child() */
> +    assert(i < s->num_children && child);
> +
> +    if (s->num_children <= s->threshold) {
> +        error_setg(errp,
> +            "The number of children cannot be lower than the vote threshold 
> %d",
> +            s->threshold);
> +        return;
> +    }
> +
> +    bdrv_drain(bs);
> +    /* We can safely remove this child now */
> +    memmove(&s->bs[i], &s->bs[i + 1],
> +            (s->num_children - i - 1) * sizeof(void *));
> +    s->num_children--;
> +    s->bs[s->num_children] = NULL;

Same here, no one will check or use s->bs[s->num_children] so there's no
need to make it NULL.

Apart from the issue of using only part of Kevin's series, the rest are
minor things.

Thanks and sorry for the late review!

Berto