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

[Terry Wilson]

> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/apps/app_dial.c, lines 1524-1527
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3008#file3008line1524>
> >
> >     I think you have a type mismatch here which may result in res having garbage data in it.
> >     
> >     Can't you just use
> >     pthread_join(thread1, ret);
> >     without any cast?

I think I missed the mismatch, but I redid some variables to make it less odd.  The casts were necessary, taking them out prevented compilation.


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/apps/app_dial.c, line 2041
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3008#file3008line2041>
> >
> >     For CDR accuracy, you should change this ast_answer call to __ast_answer(chan, 0, 0);
> >     The second '0' argument will make sure that the CDR for the caller is not marked answered until the bridge begins.

done


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/apps/app_dial.c, line 2051
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3008#file3008line2051>
> >
> >     Same for this ast_answer as well.

done


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 5219-5222
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3012#file3012line5219>
> >
> >     Note that sip_srtp_destroy does not actually free the pointer passed in. You have a memory leak here as a result.

added free to sip_srtp_destroy


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, line 19949
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3012#file3012line19949>
> >
> >     This function isn't really necessary since you can just use get_transport(). If the case of the returned string is important, I'd suggest modifying get_transport() to take another parameter stating which case is required.

removed, thanks. :-)


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/channels/sdp_crypto.c, lines 37-38
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3014#file3014line37>
> >
> >     Please enclose all the constants in parentheses.

done.


> On 2009-03-10 17:05:00, Mark Michelson wrote:
> > /trunk/channels/sdp_crypto.c, lines 145-146
> > <http://reviewboard.digium.com/r/191/diff/1/?file=3014#file3014line145>
> >
> >     Any idea what this is about?
> 
>  wrote:
>     Hmm, it occurs to me that you are probably the one who added the second comment and thus don't know what needs fixing...

Yep, that was me.  I have no idea what he needed to fix, or if he fixed it and didn't remove the comment.  I suppose we'll see if it really is a bug sometime.


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/191/#review537
-----------------------------------------------------------


On 2009-03-10 14:39:03, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/191/
> -----------------------------------------------------------
> 
> (Updated 2009-03-10 14:39:03)
> 
> 
> 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.
>     http://bugs.digium.com/view.php?id=5413
> 
> 
> Diffs
> -----
> 
>   /trunk/CREDITS 180812 
>   /trunk/apps/app_dial.c 180812 
>   /trunk/build_tools/menuselect-deps.in 180812 
>   /trunk/channels/Makefile 180812 
>   /trunk/channels/chan_iax2.c 180812 
>   /trunk/channels/chan_sip.c 180812 
>   /trunk/channels/sdp_crypto.h PRE-CREATION 
>   /trunk/channels/sdp_crypto.c PRE-CREATION 
>   /trunk/channels/sip_srtp.h PRE-CREATION 
>   /trunk/channels/sip_srtp.c PRE-CREATION 
>   /trunk/configure.ac 180812 
>   /trunk/funcs/func_channel.c 180812 
>   /trunk/include/asterisk/aes_internal.h 180812 
>   /trunk/include/asterisk/autoconfig.h.in 180812 
>   /trunk/include/asterisk/rtp.h 180812 
>   /trunk/main/rtp.c 180812 
>   /trunk/makeopts.in 180812 
>   /trunk/res/res_srtp.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/191/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Terry
> 
>