[asterisk-dev] [Code Review] Make http.c IPv6-ready

[Mark Michelson]

> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > Excellent work, especially the iterating over addresses.
> > 
> > In a perfect world, you wouldn't stop iterating when you're successfully bound. You would continue iterating and create more sockets. For example, if I want to bind to www.example.com, and that hostname has A and AAAA records, I would expect Asterisk to bind to all the addresses it gets. But that's a separate issue which involves architectural changes.

Yep, the multiple socket thing is something I thought about, but like you said, that's a pretty large architectural change to make and would require more work. Thanks very much for the review!


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 995
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line995>
> >
> >     Shouldn't this be initialized to DEFAULT_PORT?

I may have complicated this a bit. I'll simplify it that way.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, lines 51-55
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line51>
> >
> >     Can we stop including asterisk/network.h? It doesn't seem needed anymore, at least on Linux...

I'll remove it. Bamboo will yell loudly if it doesn't work on other OSes and we can change it back if that's the case.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 1049
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line1049>
> >
> >     %d is for printing int. To print a fixed-size integer, you need to use what's in /usr/include/inttypes.h. In this case, we need PRId32.
> >     
> >     ...Using default port %"PRI32d, v->value, DEFAULT_PORT);

Will do.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 1077
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line1077>
> >
> >     Red.

Will fix.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 1096
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line1096>
> >
> >     ARRAY_LEN() can only be used with statically-sized arrays.
> >     
> >     Use the return value of ast_sockaddr_resolve() instead.

Will fix.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 1098
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line1098>
> >
> >     Simpler: initialize bindport to DEFAULT_PORT. Then:
> >     
> >     if (!ast_sockaddr_port(...)) {
> >         ast_sockaddr_set_port(...., bindport);
> >     }

Yep, much better idea.


> On 2010-10-29 13:44:14, Simon Perreault wrote:
> > /trunk/main/http.c, line 1106
> > <https://reviewboard.asterisk.org/r/986/diff/1/?file=12765#file12765line1106>
> >
> >     Should success be logged?

Sure, I can post a verbose message here.


- Mark


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


On 2010-10-29 12:48:30, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/986/
> -----------------------------------------------------------
> 
> (Updated 2010-10-29 12:48:30)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch allows for IPv6 addresses to be used when configuring the HTTP server in Asterisk.
> 
> There is a behavior change here as well. Since ast_sockaddr_resolve() returns an array of addresses, we'll iterate over the addresses returned until we're able to properly start the server. Adding logic to fail back to a cached address was not added, as the scope of that is quite different.
> 
> I rewrote a good portion of the configuration-loading function, so when reviewing, be sure that I haven't introduced any subtle behavior changes. As a side note, it would be awesome if we could just get rid of any "bindport" options, but I know that would be hard to wrangle :)
> 
> As a side note, I noticed an odd inconsistency regarding setting the TLS and non-TLS bind addresses. The non-TLS bind address can be a hostname, but the TLS one cannot be, because it uses the built-in option parsing in tcptls.c. I did not address this because I did not want the scope of my changes to be too broad.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/http.c 292864 
> 
> Diff: https://reviewboard.asterisk.org/r/986/diff
> 
> 
> Testing
> -------
> 
> None. I'm not in a position at the moment to be able to test very thoroughly.
> 
> 
> Thanks,
> 
> Mark
> 
>