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

[Simon Perreault]

> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/addons/chan_ooh323.c, lines 485-487
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11087#file11087line485>
> >
> >     our_addr.sin_family is not set to AF_INET prior to the call to ast_sockaddr_from_sin(). This will cause a warning message to be printed in ast_sockaddr_from_sin(). I recommend setting the address family of ouraddr before the call to ast_sockaddr_from_sin.

Good catch, thanks!


> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/addons/chan_ooh323.c, lines 3939-3940
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11087#file11087line3939>
> >
> >     1. These two lines should be swapped. You are setting a remote RTP instance address using an uninitialized ast_sockaddr.
> >     
> >     2. You should be calling ast_sockaddr_from_sin() instead of ast_sockaddr_to_sin().

Yes, thanks!


> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 8316-8326
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11091#file11091line8316>
> >
> >     You don't need the ast_sockaddr_from_sin() call here. For one thing, reg has just been allocated, so reg->addr is zeroed out. For another, reg_addr_tmp will be overwritten by the subsequent call to ast_dnsmgr_lookup().

This is not entirely true. We need reg_addr_tmp to have its ss.ss_family member set to AF_INET so as to restrict the DNS lookup to AAAA records. That said, we need to set reg->addr's own sin_family member to AF_INET to prevent a warning.


> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/channels/sip/dialplan_functions.c, line 271
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11098#file11098line271>
> >
> >     This is a lot less fun than the previous version :(
> >     
> >     Heh, no need to change it though ;)
> 
> Tilghman Lesher wrote:
>     One question.  Does setting the remote address to zeroes cause the test to fail?  We're talking TEST_FRAMEWORK code, here.

The test passes:

*CLI> test execute category channels/chan_sip/ name test_sip_rtpqos 
Running all available tests matching category channels/chan_sip/ and name test_sip_rtpqos

START  channels/chan_sip/ - test_sip_rtpqos 
  == Registered RTP engine 'test'
END    channels/chan_sip/ - test_sip_rtpqos Time: 5ms Result: PASS
  == Unregistered RTP engine 'test'

1 Test(s) Executed  1 Passed  0 Failed


> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/channels/sip/reqresp_parser.c, lines 30-34
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11103#file11103line30>
> >
> >     This comment really pertains to the entire reqresp_parser.c file, and not just this one portion.
> >     
> >     It seems that there were two widespread changes made:
> >     
> >     1. Change the word "host" to "domain"
> >     2. Remove individual port settings and add them to the end of the domain.
> >     
> >     I can't see what relevance this has to supporting IPv6 or the version-independent ast_sockaddr API. Is there something I'm missing?

Splitting of host and port is done in ast_sockaddr_parse|resolve(). Because IPv6 address literals use square brackets to delimit them, there is more to it than just "find the colon". Instead of duplicating this algorithm here, we keep the host and port together (i.e. as a domain) until the call to ast_sockaddr_parse|resolve().


> On 2010-06-25 15:36:20, Mark Michelson wrote:
> > /trunk/main/netsock2.c, line 68
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11116#file11116line68>
> >
> >     You've got red on you.

Fixed.


- Simon


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


On 2010-06-25 09:02:18, Simon Perreault wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/743/
> -----------------------------------------------------------
> 
> (Updated 2010-06-25 09:02:18)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This is the port of Asterisk to IPv6.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 272524 
>   /trunk/apps/app_externalivr.c 272524 
>   /trunk/channels/chan_gtalk.c 272524 
>   /trunk/channels/chan_h323.c 272524 
>   /trunk/channels/chan_iax2.c 272524 
>   /trunk/channels/chan_jingle.c 272524 
>   /trunk/channels/chan_mgcp.c 272524 
>   /trunk/channels/chan_multicast_rtp.c 272524 
>   /trunk/channels/chan_sip.c 272524 
>   /trunk/channels/chan_skinny.c 272524 
>   /trunk/channels/chan_unistim.c 272524 
>   /trunk/channels/sip/dialplan_functions.c 272524 
>   /trunk/channels/sip/include/dialog.h 272524 
>   /trunk/channels/sip/include/globals.h 272524 
>   /trunk/channels/sip/include/reqresp_parser.h 272524 
>   /trunk/channels/sip/include/sip.h 272524 
>   /trunk/channels/sip/reqresp_parser.c 272524 
>   /trunk/include/asterisk/acl.h 272524 
>   /trunk/include/asterisk/config.h 272524 
>   /trunk/include/asterisk/dnsmgr.h 272524 
>   /trunk/include/asterisk/netsock2.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 272524 
>   /trunk/include/asterisk/tcptls.h 272524 
>   /trunk/main/acl.c 272524 
>   /trunk/main/app.c 272524 
>   /trunk/main/config.c 272524 
>   /trunk/main/dnsmgr.c 272524 
>   /trunk/main/http.c 272524 
>   /trunk/main/manager.c 272524 
>   /trunk/main/netsock2.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 272524 
>   /trunk/main/tcptls.c 272524 
>   /trunk/res/res_rtp_asterisk.c 272524 
>   /trunk/res/res_rtp_multicast.c 272524 
> 
> Diff: https://reviewboard.asterisk.org/r/743/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Simon
> 
>