[asterisk-dev] [Code Review]: SIP INFO DTMF non-numeric codes treated as '1'

[Matt Jordan]

> On Feb. 9, 2012, 10:45 a.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, lines 19434-19439
> > <https://reviewboard.asterisk.org/r/1722/diff/2/?file=24011#file24011line19434>
> >
> >     This should just be an else case.  Otherwise there is a theoretical code path that can leave f.subclass.integer unset.  Static checkers would have an opportunity to complain.

Actually, before this we check if event > 16 and bail out.  That being said, it doesn't hurt to change this to an else.


> On Feb. 9, 2012, 10:45 a.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, line 19368
> > <https://reviewboard.asterisk.org/r/1722/diff/2/?file=24011#file24011line19368>
> >
> >     Should wrap the long line at 90.

Done


> On Feb. 9, 2012, 10:45 a.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, lines 19335-19336
> > <https://reviewboard.asterisk.org/r/1722/diff/2/?file=24011#file24011line19335>
> >
> >     This does not need to be an else clause since the then clause returns.

Done


> On Feb. 9, 2012, 10:45 a.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, lines 19331-19332
> > <https://reviewboard.asterisk.org/r/1722/diff/2/?file=24011#file24011line19331>
> >
> >     Wrap long lines at 90.

Done


- Matt


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


On Feb. 9, 2012, 9:04 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1722/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2012, 9:04 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Summary
> -------
> 
> In ASTERISK-18924, SIP INFO DTMF handling was changed to account for both lowercase alphabetic DTMF events, as well as uppercase alphabetic DTMF events.  When this occurred, the comparison of the character buffer containing the event code was changed such that the buffer was first compared against '0' and '9' to determine if it was numeric.  Unfortunately, since the first character in the buffer will typically be '1' in the case of event codes 10, 11, 16, and (if non-alphabetic) 12-15 (for 'A' - 'D'), this caused those codes to be converted to a DTMF event of '1'.
> 
> Pressing '#' in voicemail resulted in hilarity.
> 
> This patch changes this to attempt to parse the DTMF codes first as '*', '#', '!', and 'A'/'a' - 'D'/'d'.  If those fail, it then converts the character buffer to an integer value; if this is invalid a 200 OK is sent and no DTMF tone is generated.
> 
> I didn't change the sending of a 200 OK for an invalid DTMF event sent from a UA, as that was the previous behavior.
> 
> 
> This addresses bug ASTERISK-19290.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19290
> 
> 
> Diffs
> -----
> 
>   /branches/10/channels/chan_sip.c 354428 
> 
> Diff: https://reviewboard.asterisk.org/r/1722/diff
> 
> 
> Testing
> -------
> 
> Test written for the Asterisk Test Suite (see review https://reviewboard.asterisk.org/r/1723/).  Without this patch the test fails, with it - great success.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120209/29496a43/attachment-0001.htm>