Re: [PATCH] tests/functional/x86_64/test_virtio_gpu: Fix various issues reported by pylint

[Thomas Huth <thuth@redhat.com>]

On 07/11/2025 02.49, Akihiko Odaki wrote:
> On 2025/11/06 15:30, Thomas Huth wrote:
>> On 06/11/2025 03.34, Akihiko Odaki wrote:
>>> On 2025/11/05 21:09, Thomas Huth wrote:
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> Use the recommended order for import statements, specify the kind of
>>>> exceptions that we try to catch, use f-strings where it makes sense,
>>>> rewrite the vug_log_file part with a proper "with" statement and
>>>> fix some FIXMEs by checking for the availability of the devices, etc.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   tests/functional/x86_64/test_virtio_gpu.py | 46 +++++++++++-----------
>>>>   1 file changed, 23 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/tests/functional/x86_64/test_virtio_gpu.py b/tests/ 
>>>> functional/x86_64/test_virtio_gpu.py
>>>> index be96de24da2..a25f15cdb00 100755
>>>> --- a/tests/functional/x86_64/test_virtio_gpu.py
>>>> +++ b/tests/functional/x86_64/test_virtio_gpu.py
>>>> @@ -5,22 +5,23 @@
>>>>   # 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
>>>> +import subprocess
>>>>   from qemu_test import QemuSystemTest, Asset
>>>>   from qemu_test import wait_for_console_pattern
>>>>   from qemu_test import exec_command_and_wait_for_pattern
>>>>   from qemu_test import is_readable_executable_file
>>>> -
>>>> -import os
>>>> -import socket
>>>> -import subprocess
>>>> +from qemu.machine.machine import VMLaunchFailure
>>>>   def pick_default_vug_bin(test):
>>>>       bld_dir_path = test.build_file("contrib", "vhost-user-gpu", 
>>>> "vhost- user-gpu")
>>>>       if is_readable_executable_file(bld_dir_path):
>>>>           return bld_dir_path
>>>> +    return None
>>>>   class VirtioGPUx86(QemuSystemTest):
>>>> @@ -46,8 +47,8 @@ def wait_for_console_pattern(self, success_message, 
>>>> vm=None):
>>>>           )
>>>>       def test_virtio_vga_virgl(self):
>>>> -        # FIXME: should check presence of virtio, virgl etc
>>>>           self.require_accelerator('kvm')
>>>> +        self.require_device('virtio-vga-gl')
>>>>           kernel_path = self.ASSET_KERNEL.fetch()
>>>>           initrd_path = self.ASSET_INITRD.fetch()
>>>> @@ -68,7 +69,7 @@ def test_virtio_vga_virgl(self):
>>>>           )
>>>>           try:
>>>>               self.vm.launch()
>>>> -        except:
>>>> +        except VMLaunchFailure:
>>>>               # TODO: probably fails because we are missing the VirGL 
>>>> features
>>>>               self.skipTest("VirGL not enabled?")
>>>> @@ -78,8 +79,8 @@ def test_virtio_vga_virgl(self):
>>>>           )
>>>>       def test_vhost_user_vga_virgl(self):
>>>> -        # FIXME: should check presence of vhost-user-gpu, virgl, memfd etc
>>>>           self.require_accelerator('kvm')
>>>> +        self.require_device('vhost-user-vga')
>>>>           vug = pick_default_vug_bin(self)
>>>>           if not vug:
>>>> @@ -95,27 +96,26 @@ def test_vhost_user_vga_virgl(self):
>>>>           os.set_inheritable(qemu_sock.fileno(), True)
>>>>           os.set_inheritable(vug_sock.fileno(), True)
>>>> -        self._vug_log_path = self.log_file("vhost-user-gpu.log")
>>>> -        self._vug_log_file = open(self._vug_log_path, "wb")
>>>> -        self.log.info('Complete vhost-user-gpu.log file can be '
>>>> -                      'found at %s', self._vug_log_path)
>>>> -
>>>> -        vugp = subprocess.Popen(
>>>> -            [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],
>>>> -            stdin=subprocess.DEVNULL,
>>>> -            stdout=self._vug_log_file,
>>>> -            stderr=subprocess.STDOUT,
>>>> -            shell=False,
>>>> -            close_fds=False,
>>>> -        )
>>>> -        self._vug_log_file.close()
>>>> +        vug_log_path = self.log_file("vhost-user-gpu.log")
>>>> +        self.log.info('Complete vhost-user-gpu.log file can be found at 
>>>> %s',
>>>> +                      vug_log_path)
>>>> +        with open(vug_log_path, "wb") as vug_log_file:
>>>> +            # pylint: disable=consider-using-with
>>>> +            vugp = subprocess.Popen(
>>>> +                [vug, "--virgl", f"--fd={vug_sock.fileno()}"],
>>>> +                stdin=subprocess.DEVNULL,
>>>> +                stdout=vug_log_file,
>>>> +                stderr=subprocess.STDOUT,
>>>> +                shell=False,
>>>> +                close_fds=False,
>>>> +            )
>>>
>>> Let's use with for subproces.Popen() too.
>>>
>>> You may leave vug_log_file.close() after subprocess.Popen(); if 
>>> subprocess.Popen() raises an exception, the with statement for 
>>> vug_log_file will close the file. Otherwise, the vug_log_file.close() 
>>> after subprocess.Popen() will close it and the with statement for 
>>> vug_log_file will be no-op; the documentation says it is allowed to call 
>>> IOBase.call() twice:
>>> https://docs.python.org/3.14/library/io.html#io.IOBase.close
>>
>> Not quite sure whether I got your point, but the "vugp" variable is still 
>> used later in the function to terminate the subprocess manually. So if we 
>> really want to use a "when" here, I guess I have to move the whole 
>> remaining part of this function into the "when" block, don't I? Is that 
>> what you are suggesting?
> 
> I suppose you meant "with" instead of "when"; if so, yes, that's what I meant.

D'oh, of course I meant "with", not "when" ... must have been too tired 
yesterday to type properly, sorry!

  Thomas