[asterisk-dev] [Code Review] 2648: func_channel: set dtmf_features

[Matt Jordan]

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



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17884>

    The way you've structured this makes it more complex than it needs to be.
    
    This function should do one thing, and one thing only: it should set the feature flag given a character. If it can't, it should return -1.
    
    Multiple return values are usually a sign that your function is doing more than one thing. Sometimes that's justified, but I don't think this is one of those cases.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17885>

    Check for the correct case here before calling set_feature_flag_from_char. Raise the WARNING here that an invalid case was provided.
    
    It is incredibly odd that "tH" will cause a critical error and cause the dialplan function to return -1, but "ZZZZZZZZZZZZZZZZZZZZZZZZZZH" will not.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17886>

    A feature that couldn't be applied means we were given a letter we couldn't understand. This should be a WARNING - invalid input means a user has a configuration problem.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17887>

    Now that you're catching the case sensitivity of the feature when you first extract the letter, this error is not longer needed here.
    
    You can tell this is an odd place to raise this warning - we return 0 if given one class of invalid input (an upper case letter not a flag we recognize), but we return -1 if given a different kind of invalid input (a lower case letter). That kind of difference in a function's return is odd and difficult to maintain.


- Matt Jordan


On July 3, 2013, 8:55 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2648/
> -----------------------------------------------------------
> 
> (Updated July 3, 2013, 8:55 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21876
>     https://issues.asterisk.org/jira/browse/ASTERISK-21876
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Allows setting dtmf features via a channel set function (namely CHANNEL(dtmf_features)
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_channel.c 393426 
>   /trunk/include/asterisk/bridging_basic.h 393426 
>   /trunk/main/bridging_basic.c 393426 
> 
> Diff: https://reviewboard.asterisk.org/r/2648/diff/
> 
> 
> Testing
> -------
> 
> Stuck two channels in the following extension:
> 
> exten => 10,1,NoOp
> exten => 10,n,Answer()
> exten => 10,n,Set(CHANNEL(dtmf_features)=wktx)
> exten => 10,n,Wait(50)
> 
> Then bridged them together with the manager bridge command and checked to see if both channels could park, transfer, and attended transfer.
> 
> It's worth noting that the dial application overrides this datastore entirely, so these don't get applied to dial as-is. This might need to be addressed.
> 
> 
> Thanks,
> 
> jrose
> 
>

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