[asterisk-dev] [Code Review]: Add a unit test for ast_sockaddr_split_hostport

[Terry Wilson]

> On Nov. 9, 2011, 12:13 a.m., wdoekes wrote:
> > /branches/1.8/tests/test_netsock2.c, lines 159-160
> > <https://reviewboard.asterisk.org/r/1575/diff/2/?file=21666#file21666line159>
> >
> >     This feels awkward.
> >     
> >     Shouldn't these v6 addresses always be enclosed in brackets?
> >     
> >     If there are cases that this should be allowed, I'd like a couple of more test cases that fail below, like:
> >     "1234:5678"
> >     "1234::4567::7890"
> >     Or should they be matched, and if so, to what?
> >     
> >     Otherwise I would just say that all addresses that do not start with '[' should be split by the first or last ':'.

> Shouldn't these v6 addresses always be enclosed in brackets?

No. Only v6 addresses with ports require brackets. These have no ports.

"1234:5678" is not a valid address unless you subscribe to the theory that IPv4 addresses should be representable as 32-bit integers. Anyone who ever does this should be shot. There are other places in the code where doing this wouldn't work, so I'm not testing it for this one function.

"1234::4567::7890" is not a valid address as you cannot have multiple :: in an IPv6 address.

If you look at the ast_sockaddr_split_hostport() function, you will see that it does something very similar to what you suggest with '['


> On Nov. 9, 2011, 12:13 a.m., wdoekes wrote:
> > /branches/1.8/tests/test_netsock2.c, line 158
> > <https://reviewboard.asterisk.org/r/1575/diff/2/?file=21666#file21666line158>
> >
> >     Another test with this with a port perhaps?

There is already a test for IPv6 addresses with a port (they are enclosed in brackets in that case). I could add a "[::ffff:5.6.7.8]:5060" test, but it wouldn't be appreciably different than the "[fe80::200:5aee:feaa:20a2]:5060" test since all IPv6 addresses with ports must start with '['.


> On Nov. 9, 2011, 12:13 a.m., wdoekes wrote:
> > /branches/1.8/tests/test_netsock2.c, lines 167-169
> > <https://reviewboard.asterisk.org/r/1575/diff/2/?file=21666#file21666line167>
> >
> >     These tests could use the [ipv6] versions of the test as well.

I almost didn't add these tests either. I knew someone would ask for even more if I left them in. I was right. :-p


- Terry


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


On Nov. 8, 2011, 11:43 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1575/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2011, 11:43 p.m.)
> 
> 
> Review request for Asterisk Developers and Simon Perreault.
> 
> 
> Summary
> -------
> 
> Adds a unit tests for ast_sockaddr_split_hostport.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/tests/test_netsock2.c 343673 
> 
> Diff: https://reviewboard.asterisk.org/r/1575/diff
> 
> 
> Testing
> -------
> 
> Test passes.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111109/4d3952c4/attachment.htm>