> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
