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
