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

Mon Jul 5 09:41:41 CDT 2010

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/include/asterisk/netsock2.h, lines 290-291
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11261#file11261line290>
> >
> >     Can you explain this comment more?

An address should be considered an atomic entity. The port number should not be extracted and carried around separately. We needed to use this function in a few places because we didn't want to change the whole design of existing code. But new code should not use this function.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/include/asterisk/netsock2.h, lines 492-515
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11261#file11261line492>
> >
> >     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.

Yes, that was the idea. Fixed in r273881.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/include/asterisk/rtp_engine.h, line 522
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11262#file11262line522>
> >
> >     This should say:
> >     
> >     \param sa Address we want to bind to
> >     
> >     You are currently missing the parameter name "sa"

Fixed in 273882.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/acl.c, line 135
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11264#file11264line135>
> >
> >     Please either remove this warning or downgrade it to a debug message. I can see this being quite an annoyance.

Fixed in 273883.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/acl.c, lines 497-505
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11264#file11264line497>
> >
> >     I think this would be a good place to add an ast_assert() statement to ensure that the port is not zero.

No. The only use we have for the port number in this function is to store it temporarily in a separate variable and restore it at the end. This ensures that it does not get modified, preserving the original behaviour. So we don't care if it is zero or not.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/acl.c, lines 542-547
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11264#file11264line542>
> >
> >     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.

You're right. Fixed in 273885.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/acl.c, lines 554-561
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11264#file11264line554>
> >
> >     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

The reason for not doing to was that ast_sockaddr_resolve_first() is so evil that we want to restrain its availability. I mean, ignoring all records except the first one (which may be IPv4 or IPv6 and which may not even be reachable from the address family you're bound to) is bound to create bugs. In chan_sip everything is architected around a single socket and ignoring all records but the first one and we didn't want to do a complete rearchitecturing, which isn't related to IPv6.

We factored this code in acl.c into a single function in r273893.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/app.c, lines 1216-1221
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11265#file11265line1216>
> >
> >     Why was this code added? Also, the second if statement should be comparing *scan to ']' instead of '['

It is necessary for parsing register lines that contain IPv6 address literals.

Fixed in r273894.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/dnsmgr.c, lines 171-172
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11267#file11267line171>
> >
> >     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()

The problem here is that we can't set the port before knowing the address family of tmp.

Fixed in r273895. We only set the port if the SRV lookup did not set it first.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/dnsmgr.c, line 173
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11267#file11267line173>
> >
> >     Space after if

Fixed.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/http.c, lines 1049-1054
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11268#file11268line1049>
> >
> >     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?

Right. The HTTP subsystem is still not IPv6-ready.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/http.c, lines 1075-1080
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11268#file11268line1075>
> >
> >     This if statement should be testing tmp2 instead of tmp.

Right. Fixed in r273910.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/http.c, lines 1043-1047
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11268#file11268line1043>
> >
> >     Is there a reason you can't just use ast_sockaddr_set_port() here?

No. This is just the result of adding a lot of casts rather mechanically. ;)

Fixed in r273901.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/manager.c, lines 6183-6184
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11269#file11269line6183>
> >
> >     These memset() calls are unnecessary since you initialize the struct to be all zeros at the top of the function.

Right. Fixed in r273916.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 98-116
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line98>
> >
> >     ast_copy_string() should not be used if the target is an ast_str structure. Instead, please use ast_str_set().

Fixed in r273928.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 192-197
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line192>
> >
> >     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()

No, it doesn't. Ideally when we call getaddrinfo() we would already know the type of socket that we want and use that, and then we could just pass the results from getaddrinfo() to generic code. But at this point we don't already know it so we use UDP, and we ignore the socktype in the results returned by getaddrinfo().

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 198-199
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line198>
> >
> >     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()

Fixed in r273947.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, line 247
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line247>
> >
> >     Be prepared to handle a NULL return from ast_malloc()

Fixed in r273956.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 385-401
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line385>
> >
> >     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 :)

Fixed in r273968.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, lines 442-456
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line442>
> >
> >     In both of these failure cases, you can call strerror(errno) to give more of a hint what went wrong.

Sure. This code was copied verbatim from netsock.c.

Fixed in r273973.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/netsock2.c, line 464
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11270#file11270line464>
> >
> >     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.

Fixed in r273974.

> On 2010-07-01 16:15:13, Mark Michelson wrote:
> > /trunk/main/rtp_engine.c, lines 1068-1070
> > <https://reviewboard.asterisk.org/r/743/diff/4/?file=11271#file11271line1068>
> >
> >     All of these .len checks here can be replaced with ast_sockaddr_isnull() if you wish to keep things more abstract.

Fixed in r273975.

- Simon

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

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