[Home]

Summary:ASTERISK-16702: [patch] Hints for non-existent devices are in an Idle state
Reporter:Elazar Broad (ebroad)Labels:
Date Opened:2010-09-20 09:37:22Date Closed:2012-03-13 14:59:19
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Subscriptions
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 18018_v2.diff
Description:bhn-pbx01*CLI> core show hint 900
900@broadhn-internal    : SIP/foo             State:Idle            Watchers  0
1 hint matching extension 900


foo is not a valid SIP device:

bhn-pbx01*CLI> sip show peer foo
Peer foo not found.


****** ADDITIONAL INFORMATION ******

AEL to replicate:

hint(SIP/doesntexist) 900 => {NoOp(This should never be reached);};
Comments:By: Elazar Broad (ebroad) 2010-09-21 14:34:58

Picking through the code yielded this:

sip_devicestate returns AST_DEVICE_UNKNOWN for peer's it can't find, while semantically correct, ast_devstate_to_extenstate() maps AST_DEVICE_UNKNOWN to AST_EXTENSION_NOT_INUSE which is VeryBad(tm). The attached patch returns AST_DEVICE_UNAVAILABLE which is inline with the comment in sip_devicestate().
By: Stefan Schmidt (schmidts) 2010-10-20 10:02:34

what happens if you use your patch for another tech like parking slots or whatever hints could be used against (dont know and use this).

will this also show unavailable? cause i think this could be the reason for using not_inuse here instead of unavailable

best regards

stefan
By: Elazar Broad (ebroad) 2010-10-20 10:31:23

Good point, and I wouldn't be surprised if this was the case. One thing that should be noted though, in sip_devicestate() we return unavailable for a device which we have no address for, so in orders of magnitude, a device which doesn't exist, which returning AST_DEVICE_UNKNOWN is correct(Asterisk truly does not know anything about this device), mapping that to AST_EXTENSION_NOT_INUSE is by far worse. Maybe, the best solution is falling through with AST_DEVICE_INVALID which will be mapped to AST_EXTENSION_UNAVAILABLE.
By: Stefan Schmidt (schmidts) 2010-10-21 03:31:01

after checking devicestate.c again i think this would work fine, cause if there is no tech with a linked devicestate like SIP it would go over the channel list to find it. This is exactly what happens now if a hint for a non existing peer is checked and return UNKNOWN. So this would cause also a locking situation and its slow and needless ;)

the problem i see is in app_page (page_exec) cause changing form unknown to unavailable will not page this device anymore.
Also in func_devstate the return value would be changed cause of this so it could break some dialplan logic :(
By: Stefan Schmidt (schmidts) 2010-10-21 03:34:09

btw UNKNOWN is not the right state for a device * doesnt know about, it should be INVALID (have a look at devstatestring). it says UNKNOWN = Valid, but unknown state. INVALID is Invalid - not known to Asterisk which would be the right value for this.

But the problem of changes i mention above would be the same.
By: Alan Graham (zerohalo) 2010-10-21 14:58:21

think this may be the result of the patch committed in issue ASTERISK-14954 ?
By: Elazar Broad (ebroad) 2010-10-21 15:43:02

Yea, it looks that way. Nice catch!
By: Stefan Schmidt (schmidts) 2010-10-21 15:53:42

really nice catch and it looks like they run into the same trap we did ;)
Unavailable means not this device doesnt work, only the state is unavailable. Invalid would be the right state.
By: Stefan Schmidt (schmidts) 2011-04-06 03:13:14

after rereading devicestate.c again yesterday i found something which could cause this issue.

in ast_devstate_aggregate_add if a device is unavailable this state is not aggregated right, this causes in ast_devstate_aggregate_result that we see the device state not in use.

so i think the problem is in devicestate.c not in chan_sip.c
By: Elazar Broad (ebroad) 2011-04-06 11:36:47

I believe agg->all_unavail defaults to 1 in ast_devstate_aggregate_init(), so it should be handled correctly ast_devstate_aggregate_result(), additionally, I believe sip_devicestate() is called by devicestate.c, so returning the wrong mapping for a single device is going to mess with our aggregate result. The 2nd part of the problem is the device state to extension state mappings, by returning AST_DEVICE_UNKNOWN for a single invalid device or invalid aggregate list of devices, our total device state is AST_DEVICE_UNKNOWN, which according the devicestate enum, means that the device or list of devices is valid but of unknown state(http://www.asterisk.org/doxygen/trunk/devicestate_8h.html#f72f8a5c0601e4c1f8a852749cd44eee), which isn't true.
By: Elazar Broad (ebroad) 2011-04-06 12:33:34

Scratch that, this patch no longer works on trunk. I believe you are correct in that ast_devstate_aggregate_add() and ast_devstate_aggregate_result() in concert with ast_devstate_to_extenstate() are broken in more ways than one. In ast_devstate_aggregate_add(), AST_DEVICE_INVALID sets free and busy to 0, leaving us with unknown and unavailable set to 1. In ast_devstate_aggregate_result(), the first condition hit is all_unknown, which returns an aggregate result of AST_DEVICE_UNKNOWN, which maps to AST_EXTENSION_NOT_INUSE in ast_devstate_to_extenstate(). Essentially ast_devstate_aggregate_result() should handle AST_DEVICE_INVALID, something to the effect of:

if (all_unknown && all_unavail)
return AST_DEVICE_INVALID;

Additionally, in ast_devstate_aggregate_add(), AST_DEVICE_UNKNOWN should set all_unavail to 0 and we still need to let sip_devicestate() in chan_sip fall through to AST_DEVICE_INVALID for non-existent devices.

elazar