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

Thu Jul 1 16:15:12 CDT 2010

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

I have now reviewed the entire diff. I'll give another run-through after the points below have been addressed.

/trunk/include/asterisk/netsock2.h
<https://reviewboard.asterisk.org/r/743/#comment5050>

    Can you explain this comment more?

/trunk/include/asterisk/netsock2.h
<https://reviewboard.asterisk.org/r/743/#comment5071>

    I have now looked at the implementations for ast_sockaddr_to_sin() and ast_sockaddr_from_sin(). Why does one take a pointer to an output parameter and one return a copy of the struct created? I'm assuming the reason is that ast_sockaddr_from_sin() cannot encounter errors and ast_sockaddr_to_sin() can. I dislike the inconsistent feel of these functions and would prefer that they both were formatted the way ast_sockaddr_to_sin() is.

/trunk/include/asterisk/rtp_engine.h
<https://reviewboard.asterisk.org/r/743/#comment5051>

    This should say:

    \param sa Address we want to bind to

    You are currently missing the parameter name "sa"

/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/743/#comment5052>

    Please either remove this warning or downgrade it to a debug message. I can see this being quite an annoyance.

/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/743/#comment5053>

    I think this would be a good place to add an ast_assert() statement to ensure that the port is not zero.

/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/743/#comment5054>

    This if statement needs to use ast_sockaddr_is_any() instead of ast_sockaddr_isnull(). We should never return an unspecified address from this function. 

/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/743/#comment5055>

    I've noticed this construct used in a few places. In chan_sip.c, there is a function called ast_sockaddr_resolve_first(). Perhaps this function should be moved to netsock2.[ch] since it seems to have a use outside of chan_sip.c

/trunk/main/app.c
<https://reviewboard.asterisk.org/r/743/#comment5056>

    Why was this code added? Also, the second if statement should be comparing *scan to ']' instead of '['

/trunk/main/dnsmgr.c
<https://reviewboard.asterisk.org/r/743/#comment5058>

    If ast_get_ip_or_srv() does an SRV lookup, it may set the port on tmp. The ast_sockaddr_set_port() operation has the potential to overwrite the port we set during ast_get_ip_or_srv() with the old port set on the dnsmgr entry.

    The old code avoided this issue by setting the port on tmp prior to the call to ast_get_ip_or_srv()

/trunk/main/dnsmgr.c
<https://reviewboard.asterisk.org/r/743/#comment5057>

    Space after if

/trunk/main/http.c
<https://reviewboard.asterisk.org/r/743/#comment5059>

    Is there a reason you can't just use ast_sockaddr_set_port() here?

/trunk/main/http.c
<https://reviewboard.asterisk.org/r/743/#comment5060>

    Is there a reason you can't just use ast_sockaddr_parse() here?

    Edit: After looking further, I'm guessing this is to prevent a user from trying to configure an IPv6 address in http.conf. Is that correct?

/trunk/main/http.c
<https://reviewboard.asterisk.org/r/743/#comment5061>

    This if statement should be testing tmp2 instead of tmp.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/743/#comment5063>

    These memset() calls are unnecessary since you initialize the struct to be all zeros at the top of the function.

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5064>

    ast_copy_string() should not be used if the target is an ast_str structure. Instead, please use ast_str_set().

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5065>

    Does setting hints.ai_socktype to SOCK_DGRAM cause problems if you are trying to get address information pertaining to a TCP port? My manpage just says that the ai_socktype field specifies the "preferred socket type" but does not say anything useful beyond that.

    The same comment also applies to ast_sockaddr_resolve()

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5066>

    It is possible for port to be NULL here. We have found that there are some platforms that do not like to be given NULL arguments for printf-like functions. Use S_OR(port, "") instead.

    The same thing is done in ast_sockaddr_resolve()

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5067>

    Be prepared to handle a NULL return from ast_malloc()

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5068>

    I noticed that this function's logic is copied almost verbatim in chan_sip.c's peer_iphash_cb() function. I think the function in chan_sip.c should be calling ast_sockaddr_hash() instead.

    I would have put this comment into chan_sip.c instead of here, but that's on a different page of the diff and I didn't feel like losing my spot :)

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5069>

    In both of these failure cases, you can call strerror(errno) to give more of a hint what went wrong.

/trunk/main/netsock2.c
<https://reviewboard.asterisk.org/r/743/#comment5070>

    It's up to you on this one, but this if is equivalent to calling if (ast_sockaddr_isnull(addr)), so you could change it to that if you wanted to try to abstract things more.

/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/743/#comment5072>

    All of these .len checks here can be replaced with ast_sockaddr_isnull() if you wish to keep things more abstract.

- Mark

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