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

[Wen Congyang]

At 2015/7/2 22:02, Alberto Garcia Wrote:
> 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...

Yes, will do it in the next version.

> 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.

The things I think about it is: if vote_threshold-- when deleting a 
child, vote_threshold
will be less than 0. If we don't do vote_threshold-- when it is 1, the 
vote_threshold will
be changed when all children are added again. Which behavior is better?

> 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.

OK, will remove it in the next version. Just a question: why is using 
memset()
for setting NULL pointers is not prtable?

Thanks
Wen Congyang

> Otherwise the code looks good to me. Thanks!
>
> Berto