[Asterisk-code-review] sipp, test suite utils: Default media port to an unused port (testsuite[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/8691 )

Change subject: sipp, test_suite_utils: Default media port to an unused port
......................................................................


Patch Set 5:

(7 comments)

https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py
File lib/python/asterisk/utils_socket.py:

https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@190
PS5, Line 190:             for attempt in range(attempts):
             :                 p = get_unused_os_port(host, port, socktype, family)
             : 
             :                 if p != 0 and not self.is_reserved(p, socktype, family):
             :                     res.append(p)
             :                     break
             : 
             :                 if port: return []
             :             else:
             :                 raise PortError(socktype, family, attempts)
> I'm not sure this is correct. It looks like it will continuously try to get
The goal of this function is to check the availability of given ports, or if any port is 0 then it attempts to retrieve a valid one from the OS.

If the port is 0 then this will try "attempt" times to get a free port from the OS and one not already reserved.

What the "if port: return []" does here is short circuit the "attempts" if a port was specified (non 0) since you are correct we only want to check once in that case.


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@215
PS5, Line 215: number(s)
> Looks like this should be just for a single port. Doesn't check to see if i
Originally this was going to be passed an array, but it complicated things for current use, so I dropped it.  I just noticed I left out the 'span' arg as well.


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@222
PS5, Line 222:         empty list.
> Is this true?  Seems like we throw an exception if we cannot get ports (or 
Kinda. If passed a port and that port is not available or any of its spanned ports then an empty list is returned (only the single check is done)

If port is initially 0 then "attempts" are tried to find a free port +/- its spanned ports. Once found those are returned, otherwise an exception is thrown.

I will mention that to make it more clear.


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@228
PS5, Line 228:                 return self.get_avail(host, range(port, port + span + step,
> This grabs an excess port if span > 0.  If span = 4:
Correct. I was thinking span would represent the number of extra ports *not* including the given port. As in give me a port plus "span" number of ports. 

If that seems counterintuitive or I need to rename the parameter (if that makes more sense) then I can change it. 

I guess too I could make it so a given span of 0 becomes 1, so things would even out for the inclusive way. Thoughts?


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@233
PS5, Line 233:             ports = self.get_avail(host, range(port[0] + 1, port[0] + span +
> I think this grabs excess ports.  Example:
Same reasoning as the other one (above). I was thinking span would represent the number of extra ports *not* including the given port. As in give me a port plus "span" number of ports.

Seems like that is more the expected behavior, so I'm leaning toward changing it.


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@233
PS5, Line 233: range(port[0] + 1, port[0] + span +
             :                 step, step)
> This is not returning what I would expect if span is 0. If I use a port of 
If given a port of '4' this code path would not be chosen, but instead the one just above it would - 'if port != 0: ....'


https://gerrit.asterisk.org/#/c/8691/5/lib/python/asterisk/utils_socket.py@238
PS5, Line 238:         PortError(socktype, family, attempts)
> Do you not need to 'raise PortError(...)'?
If searching for an available port and its 'spans' and nothing available can be found in "attempts" tries then we thrown an exception.

(unless I misunderstand your question)



-- 
To view, visit https://gerrit.asterisk.org/8691
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3da461123afc30e1f5ca12e65d289eaa42d6de00
Gerrit-Change-Number: 8691
Gerrit-PatchSet: 5
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 18 Apr 2018 22:35:48 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180418/917928ed/attachment-0001.html>