[asterisk-dev] [Code Review] 3982: res_rtp_asterisk: Fix a slew of TURN issues.

[rmudgett]

> On Sept. 8, 2014, 1:35 p.m., Matt Jordan wrote:
> > /branches/13/res/res_rtp_asterisk.c, line 999
> > <https://reviewboard.asterisk.org/r/3982/diff/1/?file=67291#file67291line999>
> >
> >     Naming nitpick: thread is a bit too close to a keyword (it certainly gets bolded in reviewboard). I'd rename it to something that won't be quite as confusing in review tools/IDEs/things that like markup.

I think reviewboard is bolding it because it is some kind of library identifer just like it bolds NULL.  Kind of hard to predict those kinds of naming collisions when the usage has nothing to do with the library identifier.


> On Sept. 8, 2014, 1:35 p.m., Matt Jordan wrote:
> > /branches/13/res/res_rtp_asterisk.c, line 850
> > <https://reviewboard.asterisk.org/r/3982/diff/1/?file=67291#file67291line850>
> >
> >     Nitpick: no space between (int) and status.

I prefer
(int) status,
over
(int)status,


> On Sept. 8, 2014, 1:35 p.m., Matt Jordan wrote:
> > /branches/13/res/res_rtp_asterisk.c, lines 470-479
> > <https://reviewboard.asterisk.org/r/3982/diff/1/?file=67291#file67291line470>
> >
> >     This was clearly in this code prior to this patch, but it'd be nice if component were typed to ast_rtp_ice_component_type. That would make it less likely that an arbitrary integer is passed to the function and going beyond the bounds of the array.

I don't expect the C compiler to enforce that kind of typing since conversion between an int and an enum is silent (and vice versa).
However, using enum would help when using gdb since it can convert the enum values to their symbolic names.


- rmudgett


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


On Sept. 7, 2014, 10:07 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3982/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2014, 10:07 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23577 and ASTERISK-23634
>     https://issues.asterisk.org/jira/browse/ASTERISK-23577
>     https://issues.asterisk.org/jira/browse/ASTERISK-23634
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The TURN support in res_rtp_asterisk has been exercised by only a few, which has uncovered a slew of issues.
> 
> 1. The number of file descriptors that an ioqueue instance in pjlib can support is a fixed number. Exceeding this causes an assertion. The code will now dynamically create/terminate threads as needed to ensure that this limit is not exceeded on ioqueue instances.
> 2. The API did not allow users of the TURN code to explicitly request a TURN session with details. This has been added.
> 3. The ICE code has a fixed size array of 4 for transports. As our transport identifiers started at 1 we were exceeding this causing an assertion. Our identifiers now start at 0.
> 4. The TURN client did not set up client binding causing needless bandwidth usage. Upon ICE completion if TURN is used channel binding is now established.
> 5. The code will no longer update address information on every sent packet. The remote address is now updated only once upon ICE completion to the target address.
> 6. When relaying was in use STUN traffic was getting looped back to res_rtp_asterisk due to it being handled on the normal socket. It is now handled in the TURN session callback instead.
> 7. Logging when a TURN relay is in use now uses the IP address that the TURN relay is sending/receiving to/from on our behalf.
> 8. Synchronization between the TURN session and the RTP instance now ensures that the session has been fully created or fully destroyed before continuing.
> 9. Some cleanup has occurred when tearing down the pj environment.
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/res_rtp_asterisk.c 422835 
>   /branches/13/include/asterisk/rtp_engine.h 422835 
> 
> Diff: https://reviewboard.asterisk.org/r/3982/diff/
> 
> 
> Testing
> -------
> 
> Sabotaged the code so the only candidates I sent were of the relay type. Confirmed bidirectional media flow using the TURN relay.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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