Re: [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports()

[Thomas Huth]

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