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

[Mark Michelson]

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

Ship it!


Everything looks good. If you haven't done so already, make sure that the devicestate2extenstate_test unit test is still passing. If it's not, figure out whether the test itself needs to be altered or if there is some new flaw in devstate aggregation.

- Mark


On March 7, 2012, 7:04 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1808/
> -----------------------------------------------------------
> 
> (Updated March 7, 2012, 7:04 p.m.)
> 
> 
> Review request for Asterisk Developers and schmidts.
> 
> 
> 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
> -----
> 
>   /branches/1.8/channels/chan_sip.c 358434 
>   /branches/1.8/include/asterisk/devicestate.h 358434 
>   /branches/1.8/main/devicestate.c 358434 
> 
> 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/20120308/36e71a09/attachment.htm>