[asterisk-dev] [Code Review] 3174: chan_iax2: Block unnecessary control frames to/from the wire.

[rmudgett]

> On Feb. 5, 2014, 5:06 p.m., Matt Jordan wrote:
> > /branches/1.8/channels/chan_iax2.c, lines 11771-11772
> > <https://reviewboard.asterisk.org/r/3174/diff/1/?file=53380#file53380line11771>
> >
> >     This structure feels a little weird, since we handle both this frametype and this subclass earlier. It feels like this should be handled in a single block.

I didn't want to touch that block of code because channel opaquification will result in needless conflicts.


> On Feb. 5, 2014, 5:06 p.m., Matt Jordan wrote:
> > /branches/1.8/channels/chan_iax2.c, lines 1284-1286
> > <https://reviewboard.asterisk.org/r/3174/diff/1/?file=53380#file53380line1284>
> >
> >     I'm curious - why not define the parameter as an enum ast_control_frame_type - and have the cast be done when the function is called?
> >     
> >     An int should convert to an enum without any issues, and that would resolve the two variables with the same name and needing to do an additional assignment.

It is done this way because the control value can be -1 which is not a member of the enum.


> On Feb. 5, 2014, 5:06 p.m., Matt Jordan wrote:
> > /branches/1.8/channels/chan_iax2.c, lines 1284-1286
> > <https://reviewboard.asterisk.org/r/3174/diff/1/?file=53380#file53380line1284>
> >
> >     I can't say I'm a big fan of parameters that begin with _. While there are plenty of functions in chan_iax2 that use the '__' prefix, there are no functions that I could find that take in a parameter with a leading '_'.
> >     
> >     I'd rename this to something else - maybe frame_subtype, since that's what it is?

See ast_indicate_data() which uses the same names and for the same reason.  I'll rename.


> On Feb. 5, 2014, 5:06 p.m., Matt Jordan wrote:
> > /branches/1.8/channels/chan_iax2.c, lines 1289-1293
> > <https://reviewboard.asterisk.org/r/3174/diff/1/?file=53380#file53380line1289>
> >
> >     This shouldn't be needed if you have a 'default' label that disallows the frame.

You cannot have a default case in the switch and still have the compiler complain if an enum case is missing.  I have a comment at the switch statement to that effect.


- rmudgett


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


On Feb. 3, 2014, 5:31 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3174/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2014, 5:31 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1302
>     https://issues.asterisk.org/jira/browse/AST-1302
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Establishing an IAX2 call between Asterisk v1.4 and v1.8 (or later) results in an unexpected call disconnect.  The problem happens because newer values in the enum ast_control_frame_type are not consistent between the branch versions of Asterisk.
> 
> For example:
> 1) v1.4 calls v1.8 (or later) using IAX2
> 2) v1.8 answers and sends a connected line update control frame. (on v1.8 AST_CONTROL_CONNECTED_LINE = 22)
> 3) v1.4 receives the control frame as an end-of-q (on v1.4 AST_CONTROL_END_OF_Q = 22)
> 4) v1.4 disconnects the call once the receive queue becomes empty.
> 
> Several things are done by this patch to fix the problem and attempt to prevent it from happening again in the future:
> * Added a warning at the definition of enum ast_control_frame_type about how to add new control frame values.
> 
> * Made block sending and receiving control frames that have no reason to go over the wire.
> 
> * Extended the connectedline iax.conf parameter to also include the redirecting information updates.
> 
> * Updated the connectedline iax.conf parameter documentation to include a notice that the parameter must be "no" when the peer is an Asterisk v1.4 instance.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/frame.h 407196 
>   /branches/1.8/configs/iax.conf.sample 407196 
>   /branches/1.8/channels/chan_iax2.c 407196 
> 
> Diff: https://reviewboard.asterisk.org/r/3174/diff/
> 
> 
> Testing
> -------
> 
> Without the patch the IAX2 call gets disconnected when the call initially connects.
> With the patch the call stays up.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140205/7e2c4ed0/attachment-0001.html>