[asterisk-dev] [Code Review] Add IPv6 Support To Manager

Wed Jun 6 07:54:52 CDT 2012

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

Good job! Only minor fixes needed.

Happy IPv6 launch day!

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11941>

    struct ast_sockaddr should be treated as if it was opaque. Please pass by const pointer.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11931>

    Use ast_sockaddr_copy() instead.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11934>

    Could you just delete the addr variable? It appears useless.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11933>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11935>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11936>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11937>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11938>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11939>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11940>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11942>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11943>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11944>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11945>

    Please use ast_sockaddr_copy().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11947>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11948>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11949>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11950>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11951>

    Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11952>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11953>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11954>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11955>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11956>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11957>

    There is no need to initialize the address to zero.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11960>

    This variable's scope can be tightened, to reduce action at a distance. When parsing "port", just call ast_sockaddr_set_port(). When parsing "bindaddr", extract the port into a tightly-scoped variable, parse the address, then reset the port. Also, add a warning when "bindaddr" and "bindport" contain contradicting port numbers.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11958>

    Please use "[::]:0" as default instead. It will make the manager listen on both IPv4 and IPv6 on OSes that support IPv4-mapped IPv6 addresses (e.g., Linux).

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11959>

    What's wrong with atoi()? Why not keep using it?

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11962>

    Change the 0.0.0.0 to %s, and use ast_sockaddr_stringify().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11961>

    There's no need for this statement.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11966>

    This statement looks useless because of the copy done on line 7053.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11963>

    This will always be true.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11964>

    Just unconditionally ast_sockaddr_copy() then ast_sockaddr_set_port().

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1968/#comment11965>

    This will always be true.

- Simon

On June 5, 2012, 10 p.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1968/
> -----------------------------------------------------------
> 
> (Updated June 5, 2012, 10 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Adding support for IPv6 communication to the manager.
> 
> 
> This addresses bug ASTERISK-19965.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19965
> 
> 
> Diffs
> -----
> 
>   /trunk/main/manager.c 368584 
>   /trunk/CHANGES 368584 
> 
> Diff: https://reviewboard.asterisk.org/r/1968/diff
> 
> 
> Testing
> -------
> 
> Fedora 16 dev box
> Centos 5.8 test box
> 
> Tested only AMI and HTTP...
> 
> 
> Thanks,
> 
> elguero
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120606/adb947f2/attachment-0001.htm>