Re: [Qemu-devel] [PATCH COLO-BLOCK v7 02/17] quorum: implement block driver interfaces add/delete a BDS's child

[Alberto Garcia]

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