[asterisk-dev] [Code Review] 3690: CEL: Fix incorrect/missing extra field information

[Matt Jordan]

> On June 30, 2014, 5:54 p.m., Matt Jordan wrote:
> > branches/12/main/causes.c, lines 19-24
> > <https://reviewboard.asterisk.org/r/3690/diff/3/?file=61383#file61383line19>
> >
> >     Since this is specific to sip, I'd place it in something that calls that out. Maybe sip_causes?
> 
> opticron wrote:
>     I figured I'd pull more cause-related functionality in from channel.c/h in trunk while leaving only this in 12. Things that would serve better here are:
>     ast_str2cause (channel.c/h)
>     ast_cause2str (channel.c/h)
>     ast_hangup_cause2sip (semi-duplicated in chan_sip and chan_pjsip as hangup_cause2sip)

I don't think that's how I'd structure this.

A generic cause API should not be exposing any channel technology specific information, nor have channel technology specific information bundled into it. Rather, it would allow channel technologies to register themselves to the cause mapping API, and provide a mapping from channel causes to core cause codes. I think putting SIP + core stuff together creates a bad precedent:

(1) It tightly couples channel technologies to the core, which is typically something we try hard to avoid (sometimes unsuccessfully, but still... we try)
(2) It sets the precedent for putting XMPP/other technology specific information in the core as well.

Ideally, in fact, none of this would be in main/. It would be in a channel driver header file that chan_sip and chan_pjsip would use (if they so felt like it).


- Matt


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


On June 30, 2014, 3:04 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3690/
> -----------------------------------------------------------
> 
> (Updated June 30, 2014, 3:04 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This corrects two issues with the extra field information in Asterisk 12+ in channel event logs.
> 
> It is possible to inject custom values into the dialstatus provided by ast_channel_dial_type() Stasis messages that fall outside the enumeration allowed for the DIALSTATUS channel variable. CEL now filters for the allowed values and ignores other values.
> 
> The "hangupsource" extra field key is always blank if the far end channel is a chan_pjsip channel. This is because the hangupsource is never set for the pjsip channel driver. This change sets the hangupsource whenever a hangup is queued for chan_pjsip channels.
> 
> This corrects an issue with the pjsip channel driver where the hangupcause information was not being set properly. This required that the hangup_sip2cause functionality be pulled out of chan_sip and chan_pjsip into main/causes.c so that it could also be utilized by res_pjsip_session.
> 
> 
> Diffs
> -----
> 
>   branches/12/tests/test_cel.c 417545 
>   branches/12/res/res_pjsip_session.c 417545 
>   branches/12/main/cel.c 417545 
>   branches/12/main/causes.c PRE-CREATION 
>   branches/12/include/asterisk/causes.h 417545 
>   branches/12/channels/chan_sip.c 417545 
>   branches/12/channels/chan_pjsip.c 417545 
> 
> Diff: https://reviewboard.asterisk.org/r/3690/diff/
> 
> 
> Testing
> -------
> 
> Tested all three portions of the patch manually and the dial status portion using the included unit test.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140701/61cd2e38/attachment.html>