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

Reply via email to