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

[Mark Michelson]

> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 9065-9069
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9065>
> >
> >     This code will not function correctly if the maddr parameter contains an IPv6 address.
> 
> Simon Perreault wrote:
>     It will also not work if it is a host name, which RFC 3261 allows. Changed to:
>     
>     strspn(maddr, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-.:[]")
>     
>     both here and in check_via(). These are all the characters that are allowed according to the ABNF.

That works, but man is it ugly. Since the goal of this code is to copy the maddr parameter into the hostname string, it seems like a better idea to isolate the maddr portion using strchr, strsep or something similar. This can be left for someone else to do later if you'd rather leave this alone.


> On 2010-06-29 17:06:44, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 3205-3212
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3205>
> >
> >     "want_remap" needs to be set no matter what the address scheme of "theirs" is.
> 
> Simon Perreault wrote:
>     This address remapping feature is used to deal with NATs and is unnecessary in IPv6. To ensure that users are notified if they try to use it, we added a warning:
>     
>     
>            if (ast_sockaddr_is_ipv6(&theirs)) {
>                     if (localaddr && !ast_sockaddr_isnull(&externip)) {
>                             ast_log(LOG_WARNING, "Address remapping activated in sip.conf "
>                                     "but we're using IPv6, which doesn't need it. Please "
>                                     "remove \"localnet\" and/or \"externip\" settings.\n");
>                     }
>             } else {
>                     ast_sockaddr_to_sin(&theirs, &theirs_sin);
>                     ast_sockaddr_to_sin(us, &us_sin);
>     
>                     want_remap = localaddr &&
>                             !(ast_sockaddr_isnull(&externip) && stunaddr.sin_addr.s_addr) &&
>                             ast_apply_ha(localaddr, &theirs_sin) == AST_SENSE_ALLOW ;
>             }

Very nice. Thanks!


- Mark


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


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
> 
>