Summary:	ASTERISK-07541: Devicestate via manager interface incorrectly thinks an endpoint is on hold.
Reporter:	John Martin (jfp_martin)	Labels:
Date Opened:	2006-08-16 05:21:23	Date Closed:	2006-08-16 10:37:50
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) devicestate.txt
Description:	When using dialparties.agi to determine if a SIP endpoint is available before calling it (dialparties use extension status manager command) the second call to an endpoint reports that the endpoint is busy. ****** STEPS TO REPRODUCE ****** Make two calls between two endpoints and use manager command extension status to see if target endpoint is busy. ****** ADDITIONAL INFORMATION ****** So, having looked at the code. Chan_sip.c uses the function sip_peer_hold(peer, hold) to increment and decrement the peer->onHold integer. sip_devicestate() is used by the manager interface to determine the state of a device. Now, there seems to be a couple of things going on here: Firstly, process_sdp() will pretty much always call sip_peer_hold(p,0) when a device is registered and there is two way media. This will decrement peer->onHold and take it negative if peer->onHold is zero. [there should probably be a check in sip_peer_hold() to make sure that onHold can't go negative... does it make sense to have a negative number of calls on hold?] Secondly, sip_devicestate() checks to see if peer->onHold is non-zero. If onHold is negative then this test succeeds and sip_devicestate() returns with the device on hold, even though there's a negative number of devices on hold [to safeguard against this happening again, the check in sip_devicestate could be if(p->onHold > 0). Thirdly, the use of sip_peer_hold() in process_sdp() to decrement peer->onHold should probably be conditional on the call actually being on hold - like the issuing of the manager event a few lines of code below the uses of sip_peer_hold() in process_sdp() [approx line 4950]. Is there any point in decrementing peer->onHold if the sip flags are showing no calls on hold? Isn't it a bit scary that the code tries to do a re-invite (though I don't think it follows through with it) even if there are no calls on hold? Another suggestion might be to remove the peer->onHold variable all together and just use the sip_flags[1].SIP_CALL_ONHOLD_XXX, though I guess the onHold var is there in case you have more than one call on hold and you want to track when that returns to zero. I'd need to dig deeper to see how sip_devicestate() is used in the setting and clearing of the sip_flags[1] to know how they track the number of calls on hold. I don't think this affects calls unless you are checking the device state prior to making a call. Couldn't see any other mantis tickets on the issue and the three changes to chan_sip since this revision don't appear to be related to (or fix) this problem. Comments anyone? I can generate a patch with all of this in quite simply but I'd like to know if I'm on the right track first. (I've included a trace (set verbose 4, set debug 4, sip debug) of two calls that display the problem but I don't think there's enough being printed out to show the issue... I added more output to "sip show peer" to show onHold going negative but that wouldn't be a trace against trunck ;-) ) John
Comments:	By: Joshua C. Colp (jcolp) 2006-08-16 10:37:50 I've put some of the changes you've outlined into trunk as of revision 40010, so please do give it a try and if it isn't reopen this and we can talk.