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

Thu Jun 14 20:06:13 CDT 2012

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, lines 2477-2480
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line2477>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 2514
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line2514>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 3163
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line3163>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 3240
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line3240>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 3281
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line3281>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 5098
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line5098>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 5193
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line5193>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 5325
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line5325>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 5330
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line5330>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 5356
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line5356>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, lines 6212-6216
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6212>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6390
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6390>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6399
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6399>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6465
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6465>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6547
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6547>
> >
> >     Use ast_sockaddr_stringify_addr() instead of ast_sockaddr_stringify_remote().

Changed.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 2473
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line2473>
> >
> >     Could you just delete the addr variable? It appears useless.

Yep, nice catch.  With the changes, addr really wasn't needed anymore.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 1420
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line1420>
> >
> >     Use ast_sockaddr_copy() instead.

Done.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 1398
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line1398>
> >
> >     struct ast_sockaddr should be treated as if it was opaque. Please pass by const pointer.

Done.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6152
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6152>
> >
> >     Please use ast_sockaddr_copy().

Okay.

Ended up removing this line since it was duplicating what is being done in build_mansession().

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6892
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6892>
> >
> >     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.

Maybe it should not be part of these changes but I am wondering if we should get rid of the separate port and bindaddr settings in manager.conf and just have one setting (tcpbindaddr? or stick to bindaddr) that can contain an optional port number?

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6960
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6960>
> >
> >     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).

Done.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 7003
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line7003>
> >
> >     There's no need for this statement.

Woops... gone.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 7046
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line7046>
> >
> >     This statement looks useless because of the copy done on line 7053.

I reworked this area of the code.  Things just were not right... I think the variable names (ami vs amis) was messing with my brain when I was working on this and even after reviewing my changes.

I hope that I have it straight now.

> On June 6, 2012, 7:54 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 6638
> > <https://reviewboard.asterisk.org/r/1968/diff/1/?file=28535#file28535line6638>
> >
> >     There is no need to initialize the address to zero.

I might be misreading something...  from netsock2.h:

"It is important to always initialize ast_sockaddr before use -- even if they are passed to ast_sockaddr_copy() as the underlying storage could be bigger than what ends up being copied -- leaving part of the data unitialized."

Am I misunderstanding this?

- elguero

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

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/20120615/a8736bd7/attachment-0001.htm>