[asterisk-dev] [Code Review] Make hints for non-existant SIP devices show up as Unavailable instead of Idle

[Terry Wilson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1808/
-----------------------------------------------------------

(Updated March 12, 2012, 4:37 p.m.)


Review request for Asterisk Developers and schmidts.


Changes
-------

Ok, the devicestate2extenstate test found legitimate issues with the last version of the patch. This patch goes through and simplifies device state aggregation. For the most part, device states can be considered ordered, with higher ordered device states taking preference over those with lower orders. The exception is RINGINUSE as it is a combination of in-use types and RINGING. The original enum cannot be re-ordered since those values are passed over the network via IAX2, so I added a static ordred array to the aggregate function.

The changes to the test are for the (Invalid, Invalid) case which was showing up incorrectly as Unknown/Idle instead of Invalid/Unavailable, before. The test passes.


Summary
-------

First, a big thanks to Elazar Broad and Stefan Schmidt for the detective work done in ASTERISK-16702 tracking down this issue. The solution presented here is almost directly from their discussion in the issue.

Several issues result in non-existant SIP devices showing up as Idle when doing 'core show hints'.

First, in chan_sip, there used to be a section of code in sip_devicestate() that tried to resolve a hostname, and if it was unresolvable return AST_DEVICE_UNKNOWN instead of the default AST_DEVICE_INVALID. When this resolving code was removed, AST_DEVICE_INVALID would never be returned--only AST_DEVICE_UNKOWN. AST_DEVICE_UNKNOWN means the device is valid, but we don't know it's state and is the wrong result for a non-existant device.

Second, ast_devstate_aggregate_result had no way of actually returning AST_DEVICE_INVALID even if it was the result.

Third, ast_destate_aggregate_add would return the same results for both AST_DEVICE_UNKNOWN and AST_DEVICE_INVALID, making it impossible to differentiate between the two in ast_devstate_aggregate_result.

This patch fixes the above issues. The patch is longer than it technically needs to be. The agg->all_invalid state can be removed and instead just return AST_DEVICE_INVALID for the agg->all_unavail && agg->all_unknown case in ast_devstate_aggregate_result (indeed, this would be exactly what Elazar suggests in the issue). But, I thought that more explicit behavior might be less prone to future bugs. Either method would work for me.


This addresses bug ASTERISK-16702.
    https://issues.asterisk.org/jira/browse/ASTERISK-16702


Diffs (updated)
-----

  /branches/1.8/channels/chan_sip.c 358806 
  /branches/1.8/include/asterisk/devicestate.h 358806 
  /branches/1.8/main/devicestate.c 358806 
  /branches/1.8/tests/test_devicestate.c 358806 

Diff: https://reviewboard.asterisk.org/r/1808/diff


Testing
-------

Tested with unregistered, registered, and defined peers and combination of the three. Results look sane.


Thanks,

Terry

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120312/6421232d/attachment.htm>