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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|