[Asterisk-code-review] chan iax2: Prevent deadlock between hangup and sending lagrq... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: chan_iax2: Prevent deadlock between hangup and sending lagrq/ping
......................................................................


Patch Set 9: Code-Review-1

(10 comments)

https://gerrit.asterisk.org/#/c/169/9//COMMIT_MSG
Commit Message:

Line 9: channels/chan_iax2.c: Prevent the deadlock between iax2_hangup and
The channels/chan_iax2.c prefix is not needed since this is the only thing in the patch now.


Line 18: ASTERISK-24983
Change to below since the patch fixes the issue.
ASTERISK-24983 #close


https://gerrit.asterisk.org/#/c/169/9/channels/chan_iax2.c
File channels/chan_iax2.c:

Line 1698: 
         : 
         : 
         : 
         : 
         : 
Setting pingid to -1 is still needed here.  This is called in the sched context thread so the pingid is not valid after this function returns.

schedule_action() pushes the ping sending action onto another thread.


Line 1765: 
         : 
         : 
         : 
         : 
         : 
Setting lagid to -1 is still needed here.  This is called in the sched context thread so the lagid is not valid after this function returns.

schedule_action() pushes the lagrq sending action onto another thread.


Line 1678:  *
         :  * \ref See ASTERISK-24983
         :  
Please remove the issue reference.  It is not needed.  This is what version control is for.


Line 1687: 	if (!iaxs[callno] || (iaxs[callno] &&  iaxs[callno]->destroy_initiated)) {
The non-null check of iaxs[] is redundant with !iaxs[].

if (!iaxs[] || iaxs[]->destroy_initiated) {
}


Line 2067: static int iax2_delete_from_sched(const void* data) {
guidelines: function curly on its own line.

This function should just do
AST_SCHED_DEL()
return 0

Rescheduling by returning non-zero does nothing useful.  If it couldn't get deleted then rescheduling isn't going to help.


Line 2071: 	if ( ast_sched_find_data(sched, sched_id) ) {
Guidelines: No space on if parentheses
if (x) {
}


Line 2103: 	 * Don't call AST_SCHED_DEL from this thread because it leads to a deadlock
         : 	 * between scheduler thread (all of them lock the callno mutex) and this
         : 	 * thread. It is better to call them asynchronously (from other thread which
         : 	 * don't lock the callno mutex).
Update comment:
Don't call AST_SCHED_DEL from this thread for pingid and lagid because it leads to a deadlock between the scheduler thread callback locking the callno mutex and this thread which holds the callno mutex one or more times.  It is better to have another thread delete the scheduled callbacks which doesn't lock the callno mutex.


Line 2108: 	iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)((long)pvt->pingid));
         : 	iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)((long)pvt->lagid));
You have unnecessary parentheses with the casts:
(void *)(long) pvt->pingid
(void *)(long) pvt->lagid


-- 
To view, visit https://gerrit.asterisk.org/169
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I03bec1fc8faacb89630269e935fa667c6d6c080c
Gerrit-PatchSet: 9
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Y Ateya <y.ateya at starkbits.com>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Y Ateya <y.ateya at starkbits.com>
Gerrit-HasComments: Yes