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

[Y Ateya]

Y Ateya has posted comments on this change.

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


Patch Set 9:

(12 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 
Done


Line 12: starts but before it deletes the tasks in the scheduler.  The solution is
> "The solution is ..." should be in its own paragraph (with a blank line bet
Done


Line 15: Also this commit cleans up
       : the procedure of sending LAGRQ and PING.
> "Also this commit cleans up the procedure of sending LAGRQ and PING." shoul
Done


Line 18: ASTERISK-24983
> Change to below since the patch fixes the issue.
Done


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 con
Locking the mutex here requires using `iax2_lock_callno_unless_destroyed` which will be called shortly (from current thread or from scheduler thread). I will put pingid=-1 in __send_ping.


Line 1765: 
         : 
         : 
         : 
         : 
         : 
> Setting lagid to -1 is still needed here.  This is called in the sched cont
Locking the mutex here requires using `iax2_lock_callno_unless_destroyed` which will be called shortly (from current thread or from scheduler thread). I will put lagid=-1 in __send_lagrq.


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


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


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

Delete a not-found task from scheduler gives assert in development mode (saw the code, but didn't try it); That is why I check if task is still in the scheduler before deleting it.


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


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


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


-- 
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: Ashley Sanders <asanders at digium.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