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

[Wen Congyang <wency@cn.fujitsu.com>]

On 08/05/2015 08:19 PM, Alberto Garcia wrote:
> On Tue 07 Jul 2015 10:43:00 AM CEST, Wen Congyang wrote:
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Cc: Alberto Garcia <berto@igalia.com>
> 
> Sorry for being so late with this, I was on holidays during July.
> 
>> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    int i;
>> +
>> +    for (i = 0; i < s->num_children; i++) {
>> +        if (s->bs[i] == child_bs) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (i == s->num_children) {
>> +        error_setg(errp, "Invalid child");
>> +        return;
>> +    }
>> +
>> +    if (s->num_children <= s->threshold) {
>> +        error_setg(errp, "Cannot remove any more child");
>> +        return;
>> +    }
> 
> I think that should be "Cannot remove any more children".
> 
> You could also make it a bit more explanatory by saying "The number of
> children cannot be lower than the vote threshold" or something like
> that.
> 
>> +    bdrv_drain(bs);
>> +    /* We can safe remove this child now */
>> +    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void *));
> 
> s/safe/safely/
> 
> Apart from that, the code looks good to me, so
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>

Thanks, I have send add/delete a quorum child separately. This patch is changed one line:
unref child_bs when it is removed.

I will update that patch and add your reviewed-by in that patchset.

Thanks
Wen Congyang

> 
> Berto
> .
>