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

[jrose]

> On July 6, 2013, 10:07 p.m., Matt Jordan wrote:
> > /trunk/main/bridging_basic.c, lines 142-150
> > <https://reviewboard.asterisk.org/r/2648/diff/4/?file=41117#file41117line142>
> >
> >     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.

Why is that odd?  We talked about that exact issue in detail already. 'Z' might be a valid feature code here in a future version of Asterisk, so we observe it and note that it's not currently supported. We might also later enabled a way to set new feature codes to be parsed here through plugins. 't' on the other hand will never be a valid feature code in here because it's lower case and would only apply to the peer being called, and that's not something that this function was meant to support.

The addition of more 'Z's doesn't do anything since it just corresponds to a binary flag and I don't think we've ever bothered checking for that kind of redundancy in application argument parsing before.  I haven't actually checked though, so I could be wrong. If you want me to check for redundant feature requests in a dial features string and reject those, I can go ahead and do that.


- jrose


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


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/20130707/e6e9e1b8/attachment.htm>