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

[Simon Perreault]

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


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.


/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6033>

    Can we stop including asterisk/network.h? It doesn't seem needed anymore, at least on Linux...



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6035>

    Shouldn't this be initialized to DEFAULT_PORT?



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6040>

    %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);



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6036>

    Red.



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6039>

    ARRAY_LEN() can only be used with statically-sized arrays.
    
    Use the return value of ast_sockaddr_resolve() instead.



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6037>

    Simpler: initialize bindport to DEFAULT_PORT. Then:
    
    if (!ast_sockaddr_port(...)) {
        ast_sockaddr_set_port(...., bindport);
    }



/trunk/main/http.c
<https://reviewboard.asterisk.org/r/986/#comment6038>

    Should success be logged?


- Simon


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