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

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

On 07/02/2015 11:21 PM, Alberto Garcia wrote:
> On Thu 02 Jul 2015 04:30:50 PM CEST, Wen Congyang wrote:
> 
>>> 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?
> 
> Adding/removing a child and changing vote_threshold are in principle two
> unrelated operations. One user could want to modify one but not the
> other, so linking them together does not seem like a good idea. A
> specific API to change vote_threshold could be added when the use case
> appears (currently no one seems to have missed it).
> 
> So I think I would keep these things separate, I would also remove the
> "TODO" comments that mention it.
> 
> With the current patches (that do not touch vote_threshold), you can
> remove as many children as you want as long as there's at least one
> left. However if you end up with num_chilren < vote_threshold then you
> will get read errors. I see two alternatives:
> 
>   - Allow that and expect that the user will add the necessary children
>     afterwards.
> 
>   - Forbid that completely and return an error.
> 
> I think I prefer the second option.

OK, I will do it.

> 
>>> 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?
> 
> The standard allows for null pointers to be internally represented by
> nonzero bit patterns. However I'm not aware of any system that we
> support that does that.
> 
>    http://c-faq.com/null/confusion4.html
>    http://c-faq.com/null/machexamp.html

Thanks for your explantion.

Wen Congyang

> 
> Berto
> .
>