* [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() @ 2024-09-03 14:35 Philippe Mathieu-Daudé 2024-09-03 14:50 ` Daniel P. Berrangé 0 siblings, 1 reply; 4+ messages in thread From: Philippe Mathieu-Daudé @ 2024-09-03 14:35 UTC (permalink / raw) To: Thomas Huth, qemu-devel; +Cc: Philippe Mathieu-Daudé Pass the port range as argument. In order to reduce races when looking for free ports, use a per-target per-process base port (based on the target built-in hash). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- Based-on: <20240830133841.142644-33-thuth@redhat.com> --- tests/functional/test_vnc.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py index b769d3b268..508db0709d 100755 --- a/tests/functional/test_vnc.py +++ b/tests/functional/test_vnc.py @@ -10,6 +10,7 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. +import os import socket from typing import List @@ -18,7 +19,6 @@ VNC_ADDR = '127.0.0.1' VNC_PORT_START = 32768 -VNC_PORT_END = VNC_PORT_START + 1024 def check_bind(port: int) -> bool: @@ -41,9 +41,10 @@ def check_connect(port: int) -> bool: return True -def find_free_ports(count: int) -> List[int]: +# warning, racy function +def find_free_ports(portrange, count: int) -> List[int]: result = [] - for port in range(VNC_PORT_START, VNC_PORT_END): + for port in portrange: if check_bind(port): result.append(port) if len(result) >= count: @@ -91,7 +92,10 @@ def test_change_password(self): password='new_password') def test_change_listen(self): - a, b, c = find_free_ports(3) + per_arch_port_base = abs((os.getpid() + hash(self.arch)) % (10 ** 4)) + port_start = VNC_PORT_START + per_arch_port_base + port_stop = port_start + 100 + a, b, c = find_free_ports(range(port_start, port_stop), 3) self.assertFalse(check_connect(a)) self.assertFalse(check_connect(b)) self.assertFalse(check_connect(c)) -- 2.45.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() 2024-09-03 14:35 [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() Philippe Mathieu-Daudé @ 2024-09-03 14:50 ` Daniel P. Berrangé 2024-09-04 6:20 ` Thomas Huth 0 siblings, 1 reply; 4+ messages in thread From: Daniel P. Berrangé @ 2024-09-03 14:50 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Thomas Huth, qemu-devel On Tue, Sep 03, 2024 at 04:35:53PM +0200, Philippe Mathieu-Daudé wrote: > Pass the port range as argument. In order to reduce races > when looking for free ports, use a per-target per-process > base port (based on the target built-in hash). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > Based-on: <20240830133841.142644-33-thuth@redhat.com> > --- > tests/functional/test_vnc.py | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py > index b769d3b268..508db0709d 100755 > --- a/tests/functional/test_vnc.py > +++ b/tests/functional/test_vnc.py > @@ -10,6 +10,7 @@ > # This work is licensed under the terms of the GNU GPL, version 2 or > # later. See the COPYING file in the top-level directory. > > +import os > import socket > from typing import List > > @@ -18,7 +19,6 @@ > > VNC_ADDR = '127.0.0.1' > VNC_PORT_START = 32768 > -VNC_PORT_END = VNC_PORT_START + 1024 > > > def check_bind(port: int) -> bool: > @@ -41,9 +41,10 @@ def check_connect(port: int) -> bool: > return True > > > -def find_free_ports(count: int) -> List[int]: > +# warning, racy function > +def find_free_ports(portrange, count: int) -> List[int]: > result = [] > - for port in range(VNC_PORT_START, VNC_PORT_END): > + for port in portrange: > if check_bind(port): > result.append(port) > if len(result) >= count: > @@ -91,7 +92,10 @@ def test_change_password(self): > password='new_password') > > def test_change_listen(self): > - a, b, c = find_free_ports(3) > + per_arch_port_base = abs((os.getpid() + hash(self.arch)) % (10 ** 4)) > + port_start = VNC_PORT_START + per_arch_port_base > + port_stop = port_start + 100 > + a, b, c = find_free_ports(range(port_start, port_stop), 3) > self.assertFalse(check_connect(a)) > self.assertFalse(check_connect(b)) > self.assertFalse(check_connect(c)) As your comment says, this is still racey, and its also not too nice to read & understand this logic. How about we just make test_vnc.py be serialized wrt itself ? With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() 2024-09-03 14:50 ` Daniel P. Berrangé @ 2024-09-04 6:20 ` Thomas Huth 2024-09-04 7:13 ` Daniel P. Berrangé 0 siblings, 1 reply; 4+ messages in thread From: Thomas Huth @ 2024-09-04 6:20 UTC (permalink / raw) To: Daniel P. Berrangé, Philippe Mathieu-Daudé; +Cc: qemu-devel On 03/09/2024 16.50, Daniel P. Berrangé wrote: > On Tue, Sep 03, 2024 at 04:35:53PM +0200, Philippe Mathieu-Daudé wrote: >> Pass the port range as argument. In order to reduce races >> when looking for free ports, use a per-target per-process >> base port (based on the target built-in hash). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> Based-on: <20240830133841.142644-33-thuth@redhat.com> >> --- >> tests/functional/test_vnc.py | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py >> index b769d3b268..508db0709d 100755 >> --- a/tests/functional/test_vnc.py >> +++ b/tests/functional/test_vnc.py >> @@ -10,6 +10,7 @@ >> # This work is licensed under the terms of the GNU GPL, version 2 or >> # later. See the COPYING file in the top-level directory. >> >> +import os >> import socket >> from typing import List >> >> @@ -18,7 +19,6 @@ >> >> VNC_ADDR = '127.0.0.1' >> VNC_PORT_START = 32768 >> -VNC_PORT_END = VNC_PORT_START + 1024 >> >> >> def check_bind(port: int) -> bool: >> @@ -41,9 +41,10 @@ def check_connect(port: int) -> bool: >> return True >> >> >> -def find_free_ports(count: int) -> List[int]: >> +# warning, racy function >> +def find_free_ports(portrange, count: int) -> List[int]: >> result = [] >> - for port in range(VNC_PORT_START, VNC_PORT_END): >> + for port in portrange: >> if check_bind(port): >> result.append(port) >> if len(result) >= count: >> @@ -91,7 +92,10 @@ def test_change_password(self): >> password='new_password') >> >> def test_change_listen(self): >> - a, b, c = find_free_ports(3) >> + per_arch_port_base = abs((os.getpid() + hash(self.arch)) % (10 ** 4)) >> + port_start = VNC_PORT_START + per_arch_port_base >> + port_stop = port_start + 100 >> + a, b, c = find_free_ports(range(port_start, port_stop), 3) >> self.assertFalse(check_connect(a)) >> self.assertFalse(check_connect(b)) >> self.assertFalse(check_connect(c)) > > As your comment says, this is still racey, and its also not too > nice to read & understand this logic. How about we just make > test_vnc.py be serialized wrt itself ? We'll likely have more tests that need a free port in the future... tests/avocado/migration.py and tests/avocado/reverse_debugging.py use find_free_ports(), too, so we should maybe think of a logic that avoids clashes between different tests, too. Thomas ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() 2024-09-04 6:20 ` Thomas Huth @ 2024-09-04 7:13 ` Daniel P. Berrangé 0 siblings, 0 replies; 4+ messages in thread From: Daniel P. Berrangé @ 2024-09-04 7:13 UTC (permalink / raw) To: Thomas Huth; +Cc: Philippe Mathieu-Daudé, qemu-devel On Wed, Sep 04, 2024 at 08:20:12AM +0200, Thomas Huth wrote: > On 03/09/2024 16.50, Daniel P. Berrangé wrote: > > On Tue, Sep 03, 2024 at 04:35:53PM +0200, Philippe Mathieu-Daudé wrote: > > > Pass the port range as argument. In order to reduce races > > > when looking for free ports, use a per-target per-process > > > base port (based on the target built-in hash). > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > --- > > > Based-on: <20240830133841.142644-33-thuth@redhat.com> > > > --- > > > tests/functional/test_vnc.py | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py > > > index b769d3b268..508db0709d 100755 > > > --- a/tests/functional/test_vnc.py > > > +++ b/tests/functional/test_vnc.py > > > @@ -10,6 +10,7 @@ > > > # This work is licensed under the terms of the GNU GPL, version 2 or > > > # later. See the COPYING file in the top-level directory. > > > +import os > > > import socket > > > from typing import List > > > @@ -18,7 +19,6 @@ > > > VNC_ADDR = '127.0.0.1' > > > VNC_PORT_START = 32768 > > > -VNC_PORT_END = VNC_PORT_START + 1024 > > > def check_bind(port: int) -> bool: > > > @@ -41,9 +41,10 @@ def check_connect(port: int) -> bool: > > > return True > > > -def find_free_ports(count: int) -> List[int]: > > > +# warning, racy function > > > +def find_free_ports(portrange, count: int) -> List[int]: > > > result = [] > > > - for port in range(VNC_PORT_START, VNC_PORT_END): > > > + for port in portrange: > > > if check_bind(port): > > > result.append(port) > > > if len(result) >= count: > > > @@ -91,7 +92,10 @@ def test_change_password(self): > > > password='new_password') > > > def test_change_listen(self): > > > - a, b, c = find_free_ports(3) > > > + per_arch_port_base = abs((os.getpid() + hash(self.arch)) % (10 ** 4)) > > > + port_start = VNC_PORT_START + per_arch_port_base > > > + port_stop = port_start + 100 > > > + a, b, c = find_free_ports(range(port_start, port_stop), 3) > > > self.assertFalse(check_connect(a)) > > > self.assertFalse(check_connect(b)) > > > self.assertFalse(check_connect(c)) > > > > As your comment says, this is still racey, and its also not too > > nice to read & understand this logic. How about we just make > > test_vnc.py be serialized wrt itself ? > > We'll likely have more tests that need a free port in the future... > tests/avocado/migration.py and tests/avocado/reverse_debugging.py use > find_free_ports(), too, so we should maybe think of a logic that avoids > clashes between different tests, too. Create a context manager that holds a fcntl lockfile on disk, while giving you a free port ? class FreePort: def __enter__(self): self.num = find_free_port() ..acquire fcntl lock... def __exit__(self): ...release fcntl lock.. Letting tests do with FreePort() as port: ..do some test that uses port.num.... With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-09-04 7:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-03 14:35 [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports() Philippe Mathieu-Daudé 2024-09-03 14:50 ` Daniel P. Berrangé 2024-09-04 6:20 ` Thomas Huth 2024-09-04 7:13 ` Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.