[asterisk-dev] [Code Review] If 'faxdetect=yes' in sip.conf, switch to a 'fax' extension after T38 is negotiated

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/69/#review169
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/69/#comment296>

    The channel needs to be locked before reading this value.  Otherwise, it can change out from under you.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/69/#comment297>

    The channel needs to be locked before reading these values, as well.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/69/#comment298>

    You should be using ast_verb() for this in trunk.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/69/#comment299>

    This code looks fine.  However, I don't see code that explicitly sets the default value for the case where the option is not present at all.  This may already work since the default is off, but just be sure to check on it.


- Russell


On 2008-12-02 17:27:46, Dwayne Hubbard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/69/
> -----------------------------------------------------------
> 
> (Updated 2008-12-02 17:27:46)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, Mark Michelson, Kevin Fleming, and Brian Degenhardt.
> 
> 
> Summary
> -------
> 
> If 'faxdetect=yes' in sip.conf, switch to a 'fax' extension after T38 is negotiated.  Terry Wilson created the original patch for this functionality, which I slightly modified and added the faxdetect=yes|no configuration option.  This patch is only for T38 fax detection and does not do anything for G711 over SIP fax detection.  This is for issue AST-140.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 160384 
> 
> Diff: http://reviewboard.digium.com/r/69/diff
> 
> 
> Testing
> -------
> 
> 1.  Applied patch to asterisk/trunk revision 160384 and built Asterisk.
> 2.  Set faxdetect=no in /etc/asterisk/sip.conf and make a T38 fax call into Asterisk.  Verified the channel did not switch to the 'fax' extension after T38 was negotiated.
> 3.  Set faxdetect=yes in /etc/asterisk/sip.conf and make a T38 fax call into Asterisk.  Verified the channel switched to the fax extension after T38 was negotiated.
> 4.  Set faxdetect=no in /etc/asterisk/sip.conf and make a T38 fax call into Asterisk.  Verified the channel did not switch to the 'fax' extension after T38 was negotiated.
> 
> 
> Thanks,
> 
> Dwayne
> 
>