[asterisk-dev] [Code Review] 2660: Add IPv6 Support To chan_iax2

Fri Aug 30 11:39:00 CDT 2013

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


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18726>

    The ast_strdupa() isn't necessary and could potentially cause stack consumption issues since it is being used in the body of a loop.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18727>

    Rather than ast_gethostbyname, use ast_sockaddr_resolve().


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18728>

    Use ast_sockaddr_resolve() instead of ast_gethostbyname()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18729>

    This can be simplified. You can get rid of str_taddr and str_tport altogether. In the debug message, just use ast_sockaddr_stringify() and it will print the address as "addr:port" properly.
    
    The way you have it may also output IPv6 addresses without square brackets, but I'm not sure about that.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18730>

    Another case where str_taddr and str_tport are not needed based on the way the address is being printed.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18731>

    Another case where ast_strdupa() is unnecessary and potentially harmful since it is in a loop.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18732>

    Just use ast_sockaddr_stringify() and get rid of str_addr, str_port, IPV4FORMAT and IPV6FORMAT.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18733>

    Get rid of str_addr and str_port and just use ast_sockaddr_stringify()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18734>

    Don't use ast_strdupa() here.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18735>

    No need for str_addr and str_port. Just use ast_sockaddr_stringify()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18736>

    Same comment as has been made several other times.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18737>

    In this case, you can't get rid of str_addr (because of the ast_json_pack() function call), but you can get rid of str_port. For the places where you're just printing the address and port, use ast_sockaddr_stringify() on the addr. IPV4FORMAT and IPV6FORMAT are unnecessary.
    
    The ast_json_pack() string is currently formatted incorrectly. You're passing a string for the port but have specified that you will pass an integer in. Instead of using str_port, use ast_sockaddr_port() to use the proper type.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18738>

    While there isn't the same potential to blow the stack here, ast_strdupa() just isn't necessary.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18739>

    Another case where str_addr and str_port are not necessary.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18740>

    And again here.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18741>

    And here, too.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18742>

    And again.
    
    Later in the function you print out str_addr, but it wouldn't hurt in those cases to include the port as well, so my comment stands.
    
    Also, instead of copying the value of ast_sockaddr_stringify() here at the top of this ultra-way-too-big function, I recommend calling ast_sockaddr_stringify() each time you plan to print out the address. This is because there are cases in here that change the address, so you may end up printing stale values later if you cache the address string.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18743>

    The old code used inaddrcmp(), which compares both address and port. Unless this is fixing a bug I'm unaware of, you should use ast_sockaddr_cmp() instead of ast_sockaddr_cmp_addr()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18744>

    This change feels backwards. The old comment was split between two lines since it is long, but now it takes up only one line. Revert this to take up multiple lines.


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18745>

    Use ast_sockaddr_cmp() instead of ast_sockaddr_cmp_addr()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18746>

    Use ast_sockaddr_cmp() instead of ast_sockaddr_cmp_addr()


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18747>

    Would ast_sockaddr_isnull() be a suitable replacement for the individual address family checks?


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/2660/#comment18748>

    Is this change related to adding IPv6 support?


/trunk/channels/iax2/parser.c
<https://reviewboard.asterisk.org/r/2660/#comment18759>

    Avoid using inet_ntop() in favor of ast_sockaddr_stringify() if possible. In this case, you could probably do something like:
    
    struct ast_sockaddr addr;
    char *straddr;
    
    if (len == (int)sizeof(struct sockaddr_in)) {
        addr.ss.ss_family = AF_INET;
    } else if (len == (int)sizeof(struct sockaddr_in6) {
        addr.ss.ss_family = AF_INET6;
    } else {
        /* Error and return */
    }
    
    memcpy(&addr, value, len);
    addr.len = len;
    
    straddr = ast_sockaddr_stringify(&addr);
    ast_copy_string(output, straddr, maxlen);


/trunk/channels/iax2/parser.c
<https://reviewboard.asterisk.org/r/2660/#comment18750>

    Use ast_sockaddr_stringify() instead of inet_ntop() in these cases.


/trunk/channels/iax2/parser.c
<https://reviewboard.asterisk.org/r/2660/#comment18751>

    No need for separate tmp_addr and tmp_port. Just use ast_sockaddr_stringify(). You also won't need separate IPV4FORMAT and IPV6FORMAT specifiers. You can combine them into a single specifier.


/trunk/channels/iax2/parser.c
<https://reviewboard.asterisk.org/r/2660/#comment18760>

    In this case, stringifying the address is a roundabout way of accomplishing what you need. You should just be able to do a memcpy in order to take the data at data + 2 and put it in ies->apparent_addr.
    

- Mark Michelson


On Aug. 29, 2013, 4:29 a.m., Michael Young wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2660/
> -----------------------------------------------------------
> 
> (Updated Aug. 29, 2013, 4:29 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22025
>     https://issues.asterisk.org/jira/browse/ASTERISK-22025
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch adds the ability to bind and communicate over IPv6 addresses using chan_iax2.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/netsock.c 397908 
>   /trunk/main/acl.c 397908 
>   /trunk/include/asterisk/acl.h 397908 
>   /trunk/channels/iax2/parser.c 397908 
>   /trunk/channels/iax2/include/parser.h 397908 
>   /trunk/channels/iax2/include/iax2.h 397908 
>   /trunk/CHANGES 397908 
>   /trunk/channels/chan_iax2.c 397908 
> 
> Diff: https://reviewboard.asterisk.org/r/2660/diff/
> 
> 
> Testing
> -------
> 
> Successfully made calls using iax2 over IPv6 between two dev machines.  SIP Softphone -> dev vm (iax2 ipv6) -> (iax2 ipv6) dev box -> SIP ITSP -> mobile phone
> 
> 
> Thanks,
> 
> Michael Young
> 
>

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