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

[Wen Congyang <ghostwcy@gmail.com>]

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