[PATCH] tests: honor $TMPDIR for test_virtio_version

Maxim Cournoyer posted 1 patch 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250831125255.7066-1-maxim@guixotic.coop
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Zhao Liu <zhao1.liu@intel.com>
tests/functional/x86_64/test_virtio_version.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[PATCH] tests: honor $TMPDIR for test_virtio_version
Posted by Maxim Cournoyer 4 weeks ago
Until 10.1.0, the test suite could be run without having a writable
/var/tmp in the build environment.  To avoid now requiring /var/tmp in
the build environment (which can be a very minimal container like in
the case of GNU Guix), consult TMPDIR first, using /var/tmp as a
fallback.

Signed-off-by: Maxim Cournoyer <maxim@guixotic.coop>
---
 tests/functional/x86_64/test_virtio_version.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/functional/x86_64/test_virtio_version.py b/tests/functional/x86_64/test_virtio_version.py
index a5ea73237f..501545f655 100755
--- a/tests/functional/x86_64/test_virtio_version.py
+++ b/tests/functional/x86_64/test_virtio_version.py
@@ -10,6 +10,8 @@
 # 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
+
 from qemu.machine import QEMUMachine
 from qemu_test import QemuSystemTest
 
@@ -68,7 +70,9 @@ def run_device(self, devtype, opts=None, machine='pc'):
         """
         Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
         """
-        with QEMUMachine(self.qemu_bin) as vm:
+        with QEMUMachine(
+                self.qemu_bin,
+                base_temp_dir=os.environ.get('TMPDIR', '/var/tmp')) as vm:
             vm.set_machine(machine)
             if opts:
                 devtype += ',' + opts

base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
-- 
2.51.0
Re: [PATCH] tests: honor $TMPDIR for test_virtio_version
Posted by Michael Tokarev 3 weeks, 6 days ago
On 31.08.2025 15:52, Maxim Cournoyer wrote:
> Until 10.1.0, the test suite could be run without having a writable
> /var/tmp in the build environment.  To avoid now requiring /var/tmp in
> the build environment (which can be a very minimal container like in
> the case of GNU Guix), consult TMPDIR first, using /var/tmp as a
> fallback.

> diff --git a/tests/functional/x86_64/test_virtio_version.py b/tests/functional/x86_64/test_virtio_version.py
> index a5ea73237f..501545f655 100755
> --- a/tests/functional/x86_64/test_virtio_version.py
> +++ b/tests/functional/x86_64/test_virtio_version.py

> @@ -68,7 +70,9 @@ def run_device(self, devtype, opts=None, machine='pc'):
>           """
>           Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
>           """
> -        with QEMUMachine(self.qemu_bin) as vm:
> +        with QEMUMachine(
> +                self.qemu_bin,
> +                base_temp_dir=os.environ.get('TMPDIR', '/var/tmp')) as vm:

I don't think /var/tmp is a good choice here, - I'm a bit surprised it
come from you when your OS doesn't have /var/tmp by default - shouldn't
it be /tmp here?

Overall, /var/tmp is a strange choice here and in a few other tests too,
-- maybe only block.c default is the only right place to have it as the
fallback/default.

Thanks,

/mjt
Re: [PATCH] tests: honor $TMPDIR for test_virtio_version
Posted by Richard Henderson 3 weeks, 5 days ago
On 9/1/25 16:41, Michael Tokarev wrote:
> On 31.08.2025 15:52, Maxim Cournoyer wrote:
>> Until 10.1.0, the test suite could be run without having a writable
>> /var/tmp in the build environment.  To avoid now requiring /var/tmp in
>> the build environment (which can be a very minimal container like in
>> the case of GNU Guix), consult TMPDIR first, using /var/tmp as a
>> fallback.
> 
>> diff --git a/tests/functional/x86_64/test_virtio_version.py b/tests/functional/x86_64/ 
>> test_virtio_version.py
>> index a5ea73237f..501545f655 100755
>> --- a/tests/functional/x86_64/test_virtio_version.py
>> +++ b/tests/functional/x86_64/test_virtio_version.py
> 
>> @@ -68,7 +70,9 @@ def run_device(self, devtype, opts=None, machine='pc'):
>>           """
>>           Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
>>           """
>> -        with QEMUMachine(self.qemu_bin) as vm:
>> +        with QEMUMachine(
>> +                self.qemu_bin,
>> +                base_temp_dir=os.environ.get('TMPDIR', '/var/tmp')) as vm:
> 
> I don't think /var/tmp is a good choice here, - I'm a bit surprised it
> come from you when your OS doesn't have /var/tmp by default - shouldn't
> it be /tmp here?
> 
> Overall, /var/tmp is a strange choice here and in a few other tests too,
> -- maybe only block.c default is the only right place to have it as the
> fallback/default.

It's possible that it's the testsuite that should be changed to prefer /var/tmp when 
present and TMPDIR is tmpfs.  I don't know enough about what prompted the original change 
in the first place.

But my point is that with *only* this change, it is very likely that this simply exchanges 
one set of failures for another.


r~

Re: [PATCH] tests: honor $TMPDIR for test_virtio_version
Posted by Maxim Cournoyer 3 weeks, 6 days ago
Hi,

Michael Tokarev <mjt@tls.msk.ru> writes:

[...]

>> diff --git a/tests/functional/x86_64/test_virtio_version.py b/tests/functional/x86_64/test_virtio_version.py
>> index a5ea73237f..501545f655 100755
>> --- a/tests/functional/x86_64/test_virtio_version.py
>> +++ b/tests/functional/x86_64/test_virtio_version.py
>
>> @@ -68,7 +70,9 @@ def run_device(self, devtype, opts=None, machine='pc'):
>>           """
>>           Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
>>           """
>> -        with QEMUMachine(self.qemu_bin) as vm:
>> +        with QEMUMachine(
>> +                self.qemu_bin,
>> +                base_temp_dir=os.environ.get('TMPDIR', '/var/tmp')) as vm:
>
> I don't think /var/tmp is a good choice here, - I'm a bit surprised it
> come from you when your OS doesn't have /var/tmp by default - shouldn't
> it be /tmp here?

I thought I'd make the smallest possible change compared the current
behavior, to maximize the chances this lands. The current behavior comes
from the default base_temp_dir value of QEMUMachine, which defaults to
/var/tmp.

If you prefer to fallback to just '/tmp' rather than '/var/tmp', that's
fine for me as well, though it could be surprising considering
QEMUMachine normally uses /var/tmp at run time.

-- 
Thanks,
Maxim