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

[Thomas Huth <thuth@redhat.com>]

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?

  Thomas


>>           self.vm.set_console()
>>           self.vm.add_args("-cpu", "host")
>>           self.vm.add_args("-m", "2G")
>>           self.vm.add_args("-object", "memory-backend-memfd,id=mem,size=2G")
>>           self.vm.add_args("-machine", "pc,memory-backend=mem,accel=kvm")
>> -        self.vm.add_args("-chardev", "socket,id=vug,fd=%d" % 
>> qemu_sock.fileno())
>> +        self.vm.add_args("-chardev", 
>> f"socket,id=vug,fd={qemu_sock.fileno()}")
>>           self.vm.add_args("-device", "vhost-user-vga,chardev=vug")
>>           self.vm.add_args("-display", "egl-headless")
>>           self.vm.add_args(
>> @@ -128,7 +128,7 @@ def test_vhost_user_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?")
>>           self.wait_for_console_pattern("as init process")
>