On Tue 30 Jun 2015 05:34:30 AM CEST, Wen Congyang wrote:
> +static void quorum_add_child(BlockDriverState *bs, QDict *options, Error
> **errp)
> +{
> + BDRVQuorumState *s = bs->opaque;
> + int ret;
> + Error *local_err = NULL;
> +
> + bdrv_drain(bs);
> +
> + if (s->num_children == s->max_children) {
> + if (s->max_children >= INT_MAX / 2) {
> + error_setg(errp, "Too many children");
> + return;
> + }
> +
> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children * 2);
> + memset(&s->bs[s->max_children], 0, s->max_children * sizeof(void *));
> + s->max_children *= 2;
> + }
> +
> + ret = bdrv_open_image(&s->bs[s->num_children], NULL, options, "child",
> bs,
> + &child_format, false, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + s->num_children++;
> +
> + /* TODO: Update vote_threshold */
> +}
A few comments:
1) Is there any reason why you grow the array exponentially instead of
adding a fixed number of elements when the limit is reached? I guess
in practice the number of children is never going to be too high
anyway...
2) Did you think of any API to update vote_threshold? Currently it
cannot be higher than num_children, so it's effectively limited by
the number of children that are attached when the quorum device is
created.
3) I don't think it's necessary to set to NULL the pointers in s->bs[i]
when i >= num_children. There's no way to access those pointers
anyway. Same for the ' s->bs[s->num_children] = NULL; ' bit in
quorum_del_child(). I also think that using memset() for setting NULL
pointers is not portable, although QEMU is already doing this in a
few places.
Otherwise the code looks good to me. Thanks!
Berto
