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

[Thomas Huth <thuth@redhat.com>]

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