[asterisk-dev] [Code Review] IPv6 in Asterisk

[Simon Perreault]

> On 2010-06-30 13:28:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 25485-25524
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11249#file11249line25485>
> >
> >     There are a few issues here.
> >     
> >     1. You must check the return value of ao2_alloc() to ensure it is non-NULL.
> >     
> >     2. Use of strtok is not threadsafe.
> >     
> >     3. If the proxyname for the peer is zero-length, you are setting the global outboundproxy's name to be zero-length, which is not correct.
> >     
> >     On the other hand, it looks like you fixed some reversed logic when parsing the "force" portion, so kudos!

This was entirely copied from the parsing of outboundproxy in the [general] section. See around line 26209. This is completely unrelated to IPv6. It's just that per-peer outboundproxy didn't work before and we needed it for some of our tests. So if you don't mind we'll just revert this chunk and keep this for a separate issue. Ok?


> On 2010-06-30 13:28:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, line 25531
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11249#file11249line25531>
> >
> >     Would ast_sockaddr_set_null() work here?

Yes, that's much better.


> On 2010-06-30 13:28:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 26421-26429
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11249#file11249line26421>
> >
> >     I realize after seeing this code here that your use of strtok in build_peer was largely based on its usage here. As such, I'd say not to worry too much about that comment since it's not something new you've introduced. Someone else can take a look at this as a separate piece of work.

Good. So I won't touch anything. Previously parsing of per-peer outboundproxy didn't work at all. Now it has issues, but at least it seems to be working. Can be improved, but still better than nothing.


> On 2010-06-30 13:28:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 26598-26604
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11249#file11249line26598>
> >
> >     I object to the removal of the bindport option, mainly with the argument that many configurations will be broken when upgrading to 1.8. If we want to discourage its use, then we can remove documentation of the feature from the sip.conf.sample file, but in the interest of backwards-compatibility, code to handle the option should remain.

Agreed. Fixed in r273272.


- Simon


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


On 2010-06-30 09:31:29, Simon Perreault wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/743/
> -----------------------------------------------------------
> 
> (Updated 2010-06-30 09:31:29)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This is the port of Asterisk to IPv6.
> 
> 
> This addresses bug 17565.
>     https://issues.asterisk.org/view.php?id=17565
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 273189 
>   /trunk/apps/app_externalivr.c 273189 
>   /trunk/channels/chan_gtalk.c 273189 
>   /trunk/channels/chan_h323.c 273189 
>   /trunk/channels/chan_iax2.c 273189 
>   /trunk/channels/chan_jingle.c 273189 
>   /trunk/channels/chan_mgcp.c 273189 
>   /trunk/channels/chan_multicast_rtp.c 273189 
>   /trunk/channels/chan_sip.c 273189 
>   /trunk/channels/chan_skinny.c 273189 
>   /trunk/channels/chan_unistim.c 273189 
>   /trunk/channels/sip/dialplan_functions.c 273189 
>   /trunk/channels/sip/include/dialog.h 273189 
>   /trunk/channels/sip/include/globals.h 273189 
>   /trunk/channels/sip/include/reqresp_parser.h 273189 
>   /trunk/channels/sip/include/sip.h 273189 
>   /trunk/channels/sip/reqresp_parser.c 273189 
>   /trunk/include/asterisk/acl.h 273189 
>   /trunk/include/asterisk/config.h 273189 
>   /trunk/include/asterisk/dnsmgr.h 273189 
>   /trunk/include/asterisk/netsock2.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 273189 
>   /trunk/include/asterisk/tcptls.h 273189 
>   /trunk/main/acl.c 273189 
>   /trunk/main/app.c 273189 
>   /trunk/main/config.c 273189 
>   /trunk/main/dnsmgr.c 273189 
>   /trunk/main/http.c 273189 
>   /trunk/main/manager.c 273189 
>   /trunk/main/netsock2.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 273189 
>   /trunk/main/tcptls.c 273189 
>   /trunk/res/res_rtp_asterisk.c 273189 
>   /trunk/res/res_rtp_multicast.c 273189 
> 
> Diff: https://reviewboard.asterisk.org/r/743/diff
> 
> 
> Testing
> -------
> 
> See test report on the mantis issue.
> 
> 
> Thanks,
> 
> Simon
> 
>