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

[Simon Perreault]

> On 2010-07-07 14:08:14, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 12223-12228
> > <https://reviewboard.asterisk.org/r/743/diff/6/?file=11374#file11374line12223>
> >
> >     You need to set the address family of peer_addr_tmp() before calling ast_dnsmgr_lookup()

Fixed in r274681.


> On 2010-07-07 14:08:14, Mark Michelson wrote:
> > /trunk/main/http.c, lines 992-1005
> > <https://reviewboard.asterisk.org/r/743/diff/6/?file=11397#file11397line992>
> >
> >     You need to set the address family of tmp and tmp2 to AF_INET or else ast_sockaddr_from_sin() will complain.

This is intentional. __ast_http_load() uses address families as a flag. It sets them only when the sockets are enabled in the configuration. We wanted to preserve the overall behaviour.


> On 2010-07-07 14:08:14, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 490-499
> > <https://reviewboard.asterisk.org/r/743/diff/6/?file=11399#file11399line490>
> >
> >     There are several pointer errors here.
> >     
> >     The memcpy should be:
> >     
> >     memcpy(&addr->ss, sin, sizeof(*sin));
> >     
> >     and the final line should be:
> >     
> >     addr->len = sizeof(*sin);

Right. These were introduced with the refactoring. Fixed in r274682.


> On 2010-07-07 14:08:14, Mark Michelson wrote:
> > /trunk/main/rtp_engine.c, lines 415-439
> > <https://reviewboard.asterisk.org/r/743/diff/6/?file=11400#file11400line415>
> >
> >     There are many places where ast_rtp_instance_get_local_address() and ast_rtp_instance_get_remote_address() are called using uninitialized ast_sockaddr structures. This means that the ast_sockaddr_cmp() call uses the uninitialized values when comparing.
> >     
> >     Personally, I don't see the point of doing the comparison. We should just unconditionally set the address.

The return value is used elsewhere to know if the address changed, so we can't change the behaviour.

Fixed in r274684.


> On 2010-07-07 14:08:14, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 4445-4450
> > <https://reviewboard.asterisk.org/r/743/diff/6/?file=11374#file11374line4445>
> >
> >     sin is an output parameter to create_addr() and is typically uninitialized at this point. Calling ast_sockaddr_from_sin() thus causes a bunch of bogus values to be copied into sin_tmp. My suggestion here is to remove the call to ast_sockaddr_from_sin() entirely. Instead, set the address family of sin_tmp and call ast_get_ip_or_srv(). Then use ast_sockaddr_to_sin() to copy sin_tmp's content into sin.

Fixed in r274685.


- Simon


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


On 2010-07-05 10:24:08, Simon Perreault wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/743/
> -----------------------------------------------------------
> 
> (Updated 2010-07-05 10:24:08)
> 
> 
> 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 273566 
>   /trunk/apps/app_externalivr.c 273566 
>   /trunk/channels/chan_gtalk.c 273566 
>   /trunk/channels/chan_h323.c 273566 
>   /trunk/channels/chan_iax2.c 273566 
>   /trunk/channels/chan_jingle.c 273566 
>   /trunk/channels/chan_mgcp.c 273566 
>   /trunk/channels/chan_multicast_rtp.c 273566 
>   /trunk/channels/chan_sip.c 273566 
>   /trunk/channels/chan_skinny.c 273566 
>   /trunk/channels/chan_unistim.c 273566 
>   /trunk/channels/sip/dialplan_functions.c 273566 
>   /trunk/channels/sip/include/dialog.h 273566 
>   /trunk/channels/sip/include/globals.h 273566 
>   /trunk/channels/sip/include/reqresp_parser.h 273566 
>   /trunk/channels/sip/include/sip.h 273566 
>   /trunk/channels/sip/reqresp_parser.c 273566 
>   /trunk/include/asterisk/acl.h 273566 
>   /trunk/include/asterisk/config.h 273566 
>   /trunk/include/asterisk/dnsmgr.h 273566 
>   /trunk/include/asterisk/netsock2.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 273566 
>   /trunk/include/asterisk/tcptls.h 273566 
>   /trunk/main/acl.c 273566 
>   /trunk/main/app.c 273566 
>   /trunk/main/config.c 273566 
>   /trunk/main/dnsmgr.c 273566 
>   /trunk/main/http.c 273566 
>   /trunk/main/manager.c 273566 
>   /trunk/main/netsock2.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 273566 
>   /trunk/main/tcptls.c 273566 
>   /trunk/res/res_rtp_asterisk.c 273566 
>   /trunk/res/res_rtp_multicast.c 273566 
> 
> Diff: https://reviewboard.asterisk.org/r/743/diff
> 
> 
> Testing
> -------
> 
> See test report on the mantis issue.
> 
> 
> Thanks,
> 
> Simon
> 
>