Re: [PATCH 2/5] tests/functional: Extract the find_free_ports() function into a helper file

[Thomas Huth <thuth@redhat.com>]

On 04/12/2024 11.46, Daniel P. Berrangé wrote:
> On Wed, Dec 04, 2024 at 08:19:08AM +0100, Thomas Huth wrote:
>> We'll need this functionality in other functional tests, too, so
>> let's extract it into the qemu_test module.
>> Also add  an __enter__ and __exit__ function that can be used for
>> using this functionality in a locked context, so that tests that
>> are running in parallel don't try to compete for the same ports
>> later.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/functional/qemu_test/ports.py | 53 +++++++++++++++++++++++++++++
>>   tests/functional/test_vnc.py        | 36 +++++---------------
>>   2 files changed, 61 insertions(+), 28 deletions(-)
>>   create mode 100644 tests/functional/qemu_test/ports.py
>>
>> diff --git a/tests/functional/qemu_test/ports.py b/tests/functional/qemu_test/ports.py
>> new file mode 100644
>> index 0000000000..d235d3432b
>> --- /dev/null
>> +++ b/tests/functional/qemu_test/ports.py
>> @@ -0,0 +1,53 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Simple functional tests for VNC functionality
>> +#
>> +# Copyright 2018, 2024 Red Hat, Inc.
>> +#
>> +# 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 fcntl
>> +import os
>> +import socket
>> +import sys
>> +import tempfile
>> +from typing import List
>> +
>> +class Ports():
>> +
>> +    PORTS_ADDR = '127.0.0.1'
>> +    PORTS_START = 32768
>> +    PORTS_END = PORTS_START + 1024
>> +
>> +    def __enter__(self):
>> +        lock_file = os.path.join(tempfile.gettempdir(), "qemu_port_lock")
>> +        self.lock_fh = os.open(lock_file, os.O_CREAT)
>> +        fcntl.flock(self.lock_fh, fcntl.LOCK_EX)
>> +        return self
>> +
>> +    def __exit__(self, exc_type, exc_value, traceback):
>> +        fcntl.flock(self.lock_fh, fcntl.LOCK_UN)
>> +        os.close(self.lock_fh)
> 
> This code will leave '/tmp/qemu_port_lock' existing forever.... which is
> correct, because if you try to unlink it after closing, you'll introduce
> a race because the 2nd __enter__ will now be locking an unlinked file,
> and a 3rd __enter__ that comes along will create & lock an entirely new
> file.
> 
> There are ways to make this safe by using stat + fstat either side of
> LOCK_EX, in a loop, to detect locking of an unlinked file. That is
> overkill though.  It is simpler to just put the lock file in the build
> directory IMHO, and thus avoid needing to care about unlinking - that'll
> be done when the user purges their build dir.

Putting the lock in the build directory is a nice idea - but it will fail if 
the user is using multiple build directory and running the test in the 
multiple directories in parallel. But maybe we could ease that situation by 
randomizing PORTS_START based on a seed calculated somehow by the build 
directory string?

  Thomas