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

[Mark Michelson]

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


I've only made it to about line 24500 of chan_sip, so I may have more comments on it tomorrow. Unfortunately, I've run out of time for today.


/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4985>

    These additions aren't necessary. When structs are initialized in this way, any elements not specified in the declaration are automatically zeroed out.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4986>

    These are unnecessary for the same reason.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4987>

    I noticed this removal and did not see anything that seemed to replace what was formerly here. Can you explain?



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4988>

    "want_remap" needs to be set no matter what the address scheme of "theirs" is.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4989>

    Unindent the ast_debug() line by one tab. It's a bit of a nitpick, but the lines currently all look as if they are intended to belong to the else, when only the warning message does. It wouldn't hurt to add braces to the else case, too while you're here :)



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4990>

    The else case here can lead to a crash. "addrs" is not initialized to any particular value in this loop. If ast_sockaddr_resolve() returns 0, then freeing "addrs" will likely cause a crash.
    
    My suggestion is to set "addrs" to NULL at the beginning of each iteration of the loop. This way, the ast_free will always be given either a NULL pointer or a properly allocated pointer.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4991>

    This code will not function correctly if the maddr parameter contains an IPv6 address.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4992>

    Space after the comma.
    
    Also, you have changed the logic regarding the setting of udptldest.sin_port. Previously, it used udptlsin.sin_port, and now it is using p->ourip's port.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4993>

    ast_sockaddr_port() already returns the port in host byte order, so calling ntohs() on the result is erroneous.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4995>

    Is this a bug fix?



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4996>

    Similar to the other maddr comment I made, this will not work properly if the maddr parameter has an IPv6 address.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/743/#comment4997>

    Space after comma.


- Mark


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