[asterisk-dev] [Code Review] SRTP support for Asterisk

[Terry Wilson]

> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 5194-5198
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9769#file9769line5194>
> >
> >     This iax_pvt never gets unlocked.

fixed


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, line 5023
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9770#file9770line5023>
> >
> >     I recommend changing the wording here from "reinviting" to "direct media." The reason is that connected line-related reinvites are still perfectly possible, just not direct media reinvites.

fixed


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 7801-7804
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9770#file9770line7801>
> >
> >     I think this is impossible since both of the conditions of the logical OR in the if statement require len to be greater than 0.

fixed


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 7832-7835
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9770#file9770line7832>
> >
> >     Another impossible to reach block.

fixed


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 7930-7936
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9770#file9770line7930>
> >
> >     It seems a bit odd that the various process_* functions have different meanings for their return values.

inverted the logic on process_crypto to match other functions


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 7987-8012
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9770#file9770line7987>
> >
> >     Could you document what these new return values mean?

I don't think -1, -2, or -3 is documented anywhere either--and nothing actually cares about what the value of process_sdp is other than 0 || !0. I may hold off and do a separate change with documentation for the others since that is non-srtp related stuff.


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/channels/sip/sdp_crypto.c, lines 65-67
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9775#file9775line65>
> >
> >     This if is unnecessary. You can free NULL pointers with no ill effect.

fixed.


> On 2010-04-28 16:29:55, Mark Michelson wrote:
> > /trunk/funcs/func_secure.c, line 20
> > <https://reviewboard.asterisk.org/r/191/diff/2/?file=9780#file9780line20>
> >
> >     Red splotch

I went ahead and removed func_secure.c since duplicate functionality exists with CHANNEL(secure_signaling) and CHANNEL(secure_media).


- Terry


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


On 2010-04-28 21:01:02, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/191/
> -----------------------------------------------------------
> 
> (Updated 2010-04-28 21:01:02)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> SRTP support for Asterisk using Sdescriptions. This has been sitting around for a while, so I figured that it should at least get some review.  Full description of setup at http://lists.digium.com/pipermail/asterisk-dev/2009-January/036029.html
> 
> 
> This addresses bug 5413.
>     https://issues.asterisk.org/view.php?id=5413
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 259665 
>   /trunk/build_tools/menuselect-deps.in 259665 
>   /trunk/channels/chan_iax2.c 259665 
>   /trunk/channels/chan_sip.c 259665 
>   /trunk/channels/sip/dialplan_functions.c 259665 
>   /trunk/channels/sip/include/sdp_crypto.h PRE-CREATION 
>   /trunk/channels/sip/include/sip.h 259665 
>   /trunk/channels/sip/include/srtp.h PRE-CREATION 
>   /trunk/channels/sip/sdp_crypto.c PRE-CREATION 
>   /trunk/channels/sip/srtp.c PRE-CREATION 
>   /trunk/configure UNKNOWN 
>   /trunk/configure.ac 259665 
>   /trunk/funcs/func_channel.c 259665 
>   /trunk/include/asterisk/autoconfig.h.in 259665 
>   /trunk/include/asterisk/frame.h 259665 
>   /trunk/include/asterisk/global_datastores.h 259665 
>   /trunk/include/asterisk/res_srtp.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 259665 
>   /trunk/main/asterisk.exports.in 259665 
>   /trunk/main/channel.c 259665 
>   /trunk/main/global_datastores.c 259665 
>   /trunk/main/rtp_engine.c 259665 
>   /trunk/makeopts.in 259665 
>   /trunk/res/res_rtp_asterisk.c 259665 
>   /trunk/res/res_srtp.c PRE-CREATION 
>   /trunk/res/res_srtp.exports.in PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/191/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Terry
> 
>