On 6/13/2013 7:23 AM, Jan Friesse wrote:
> Fabio M. Di Nitto napsal(a):
>> On 6/12/2013 5:03 PM, Jan Friesse wrote:
>>> Signed-off-by: Jan Friesse <jfriesse@xxxxxxxxxx>
>>> ---
>>> exec/votequorum.c | 3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/exec/votequorum.c b/exec/votequorum.c
>>> index b561409..131b734 100644
>>> --- a/exec/votequorum.c
>>> +++ b/exec/votequorum.c
>>> @@ -1634,6 +1634,9 @@ static void message_handler_req_exec_votequorum_nodeinfo (
>>>
>>> if (nodeid == VOTEQUORUM_QDEVICE_NODEID) {
>>> struct cluster_node *sender_node = find_node_by_nodeid(sender_nodeid);
>>> +
>>> + assert(sender_node != NULL);
>>> +
>>> if ((!cluster_is_quorate) &&
>>> (sender_node->flags & NODE_FLAGS_QUORATE)) {
>>> node->votes = req_exec_quorum_nodeinfo->votes;
>>>
>>
>> I would prefer a proper error checking rather than assert here.
>>
>
> Agree. Actually, coverity found this problem, but I was ensure if it is
> not problem (this is what I was assuming because of dereference of
> sender_node) and then assert is proper solution OR it is problem and
> then this patch starts discussion about proper solution.
>
>> The logic is that a node always send its own information (to populate
>> node for node == sender_nodeid) and then send quorum device info from
>> that node.
>>
>> See static int votequorum_sync_process (void) that´s the one triggering
>> the population of the node entries.
>>
>> In theory, if sync queues the messages as expected, sender_node is never
>> NULL. If it is, then i´ll prefer at least to see a message that says:
>> "received qdevice info before sender node info\n" and then quit (unless
>> we can recover from that status easily).
>>
>
> Actually, because you've wrote that code you are maybe able to see how
> to recover easily, but when I was reading that code, it really didn't
> look so.
Ok so the question turns into:
static int votequorum_sync_process (void)
{
votequorum_exec_send_nodeinfo(us->node_id); <- message 1
votequorum_exec_send_nodeinfo(VOTEQUORUM_QDEVICE_NODEID); <- 2
if (strlen(qdevice_name)) {
votequorum_exec_send_qdevice_reg(VOTEQUORUM_QDEVICE_OPERATION_REGISTER,
qdevice_name);
}
return 0;
}
given this sequence of events, is there a possibility that message 1 can
be delivered after message 2?
If there is that possibility, then we need to review the whole
send_nodeinfo logic as there is the assumption (in more than one place
IIRC) that they arrive in sequence.
If there is the guarantee that they are delivered in the same order,
then we can use that assert check to verify it and error out by
reporting deeper issues.
Fabio
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
