[PATCH v2 0/7] tests/functional: a few misc cleanups and fixes

[PATCH v2 0/7] tests/functional: a few misc cleanups and fixes

[Daniel P. Berrangé]

See individual commit messages for descriptions of the
cleanups/fixes.

Changed in v2:

 * Drop the patch which skips tests when QEMU_TEST_QEMU_BINARY is unset
 * Change the way we skip tests on 32-bit builds to check the ELF binary
   format
 * Make ppc64 tuxrun tests work on 32-bit
 * Removed unused 'get_tag' method
 * Hide output from zstd command

Daniel P. Berrangé (7):
  tests/functional: remove unused 'bin_prefix' variable
  tests/functional: set 'qemu_bin' as an object level field
  tests/functional: remove all class level fields
  tests/functional: reduce tuxrun maxmem to work on 32-bit hosts
  tests/functional: skip memaddr tests on 32-bit builds
  tests/functional: drop unused 'get_tag' method
  tests/functional: stop output from zstd command when uncompressing

 docs/devel/testing/functional.rst        |  2 +-
 tests/functional/qemu_test/testcase.py   | 14 +++-------
 tests/functional/qemu_test/tuxruntest.py | 11 --------
 tests/functional/qemu_test/uncompress.py |  6 ++---
 tests/functional/test_mem_addr_space.py  | 34 ++++++++++++++++++++++++
 tests/functional/test_ppc64_tuxrun.py    |  2 +-
 6 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.48.1

[PATCH v2 1/7] tests/functional: remove unused 'bin_prefix' variable

[Daniel P. Berrangé]

This was copied over from avocado but has not been used in the new
functional tests.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/testcase.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 869f3949fe..9d5611c4d7 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -192,7 +192,7 @@ def assets_available(self):
                     return False
         return True
 
-    def setUp(self, bin_prefix):
+    def setUp(self):
         self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
         self.arch = self.qemu_bin.split('-')[-1]
         self.socketdir = None
@@ -254,7 +254,7 @@ def main():
 class QemuUserTest(QemuBaseTest):
 
     def setUp(self):
-        super().setUp('qemu-')
+        super().setUp()
         self._ldpath = []
 
     def add_ldpath(self, ldpath):
@@ -277,7 +277,7 @@ class QemuSystemTest(QemuBaseTest):
     def setUp(self):
         self._vms = {}
 
-        super().setUp('qemu-system-')
+        super().setUp()
 
         console_log = logging.getLogger('console')
         console_log.setLevel(logging.DEBUG)
-- 
2.48.1

[PATCH v2 2/7] tests/functional: set 'qemu_bin' as an object level field

[Daniel P. Berrangé]

The 'qemu_bin' field is currently set on the class, despite being
accessed as if it were an object instance field with 'self.qemu_bin'.

This is no obvious need to have it as a class field, so move it into
the object instance.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/testing/functional.rst      | 2 +-
 tests/functional/qemu_test/testcase.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
index ecc738922b..bcb5509512 100644
--- a/docs/devel/testing/functional.rst
+++ b/docs/devel/testing/functional.rst
@@ -173,7 +173,7 @@ QEMU binary selection
 ^^^^^^^^^^^^^^^^^^^^^
 
 The QEMU binary used for the ``self.vm`` QEMUMachine instance will
-primarily depend on the value of the ``qemu_bin`` class attribute.
+primarily depend on the value of the ``qemu_bin`` instance attribute.
 If it is not explicitly set by the test code, its default value will
 be the result the QEMU_TEST_QEMU_BINARY environment variable.
 
diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 9d5611c4d7..058bf270ec 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -33,7 +33,6 @@
 
 class QemuBaseTest(unittest.TestCase):
 
-    qemu_bin = os.getenv('QEMU_TEST_QEMU_BINARY')
     arch = None
 
     workdir = None
@@ -193,6 +192,7 @@ def assets_available(self):
         return True
 
     def setUp(self):
+        self.qemu_bin = os.getenv('QEMU_TEST_QEMU_BINARY')
         self.assertIsNotNone(self.qemu_bin, 'QEMU_TEST_QEMU_BINARY must be set')
         self.arch = self.qemu_bin.split('-')[-1]
         self.socketdir = None
-- 
2.48.1

[PATCH v2 3/7] tests/functional: remove all class level fields

[Daniel P. Berrangé]

A number of fields are set at the class level on QemuBaseTest, even
though the exact same named field is then set at the object level
later in most cases.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/testcase.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
index 058bf270ec..5b18416bed 100644
--- a/tests/functional/qemu_test/testcase.py
+++ b/tests/functional/qemu_test/testcase.py
@@ -33,12 +33,6 @@
 
 class QemuBaseTest(unittest.TestCase):
 
-    arch = None
-
-    workdir = None
-    log = None
-    logdir = None
-
     '''
     @params compressed: filename, Asset, or file-like object to uncompress
     @params format: optional compression format (gzip, lzma)
-- 
2.48.1

[PATCH v2 4/7] tests/functional: reduce tuxrun maxmem to work on 32-bit hosts

[Daniel P. Berrangé]

maxmem=4G is too large to address on 32-bit hosts, so reduce it
to 2G since the tuxrun tests don't actually need such an elevated
memory limit.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/test_ppc64_tuxrun.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/functional/test_ppc64_tuxrun.py b/tests/functional/test_ppc64_tuxrun.py
index 05c6162b5e..e8f79c676e 100755
--- a/tests/functional/test_ppc64_tuxrun.py
+++ b/tests/functional/test_ppc64_tuxrun.py
@@ -64,7 +64,7 @@ def ppc64_common_tuxrun(self, kernel_asset, rootfs_asset, prefix):
                          ',"index":1,"id":"pci.1"}')
         self.vm.add_args('-device', '{"driver":"spapr-vscsi","id":"scsi1"'
                          ',"reg":12288}')
-        self.vm.add_args('-m', '2G,slots=32,maxmem=4G',
+        self.vm.add_args('-m', '1G,slots=32,maxmem=2G',
                          '-object', 'memory-backend-ram,id=ram1,size=1G',
                          '-device', 'pc-dimm,id=dimm1,memdev=ram1')
 
-- 
2.48.1

[PATCH v2 5/7] tests/functional: skip memaddr tests on 32-bit builds

[Daniel P. Berrangé]

If the QEMU binary was built for a 32-bit ELF target we cannot run the
memory address space tests as they all require ability to address more
RAM that can be represented on 32-bit.

We can't use a decorator to skip the tests as we need setUp() to run to
pick the QEMU binary, thus we must call a method at the start of each
test to check and skip it. The functional result is effectively the
same as using a decorator, just less pretty. This code will go away when
32-bit hosts are full dropped from QEMU.

The code allows any non-ELF target since all macOS versions supported
at 64-bit only and we already dropped support for 32-bit Windows.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/test_mem_addr_space.py | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/functional/test_mem_addr_space.py b/tests/functional/test_mem_addr_space.py
index bb0cf062ca..c8bde77e51 100755
--- a/tests/functional/test_mem_addr_space.py
+++ b/tests/functional/test_mem_addr_space.py
@@ -20,6 +20,25 @@ class MemAddrCheck(QemuSystemTest):
     # this reason.
     DELAY_Q35_BOOT_SEQUENCE = 1
 
+    # This helper can go away when the 32-bit host deprecation
+    # turns into full & final removal of support.
+    def ensure_64bit_binary(self):
+        with open(self.qemu_bin, "rb") as fh:
+            ident = fh.read(4)
+
+            # "\x7fELF"
+            if ident != bytes([0x7f, 0x45, 0x4C, 0x46]):
+                # Non-ELF file implies macOS or Windows which
+                # we already assume to be 64-bit only
+                return
+
+            # bits == 1 -> 32-bit; bits == 2 -> 64-bit
+            bits = int.from_bytes(fh.read(1))
+            if bits != 2:
+                # 32-bit ELF builds won't be able to address sufficient
+                # RAM to run the tests
+                self.skipTest("64-bit build host is required")
+
     # first, lets test some 32-bit processors.
     # for all 32-bit cases, pci64_hole_size is 0.
     def test_phybits_low_pse36(self):
@@ -38,6 +57,7 @@ def test_phybits_low_pse36(self):
         If maxmem is set to 59.5G with all other QEMU parameters identical, QEMU
         should start fine.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'q35', '-m',
                          '512,slots=1,maxmem=59.6G',
                          '-cpu', 'pentium,pse36=on', '-display', 'none',
@@ -55,6 +75,7 @@ def test_phybits_low_pae(self):
         access up to a maximum of 64GiB of memory. Rest is the same as the case
         with pse36 above.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'q35', '-m',
                          '512,slots=1,maxmem=59.6G',
                          '-cpu', 'pentium,pae=on', '-display', 'none',
@@ -71,6 +92,7 @@ def test_phybits_ok_pentium_pse36(self):
         Setting maxmem to 59.5G and making sure that QEMU can start with the
         same options as the failing case above with pse36 cpu feature.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-machine', 'q35', '-m',
                          '512,slots=1,maxmem=59.5G',
                          '-cpu', 'pentium,pse36=on', '-display', 'none',
@@ -88,6 +110,7 @@ def test_phybits_ok_pentium_pae(self):
         Setting maxmem to 59.5G and making sure that QEMU can start fine
         with the same options as the case above.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-machine', 'q35', '-m',
                          '512,slots=1,maxmem=59.5G',
                          '-cpu', 'pentium,pae=on', '-display', 'none',
@@ -104,6 +127,7 @@ def test_phybits_ok_pentium2(self):
         Pentium2 has 36 bits of addressing, so its same as pentium
         with pse36 ON.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-machine', 'q35', '-m',
                          '512,slots=1,maxmem=59.5G',
                          '-cpu', 'pentium2', '-display', 'none',
@@ -123,6 +147,7 @@ def test_phybits_low_nonpse36(self):
         message because the region for memory hotplug is always placed
         above 4 GiB due to the PCI hole and simplicity.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'q35', '-m',
                          '512,slots=1,maxmem=4G',
                          '-cpu', 'pentium', '-display', 'none',
@@ -150,6 +175,7 @@ def test_phybits_low_tcg_q35_70_amd(self):
         which is equal to 987.5 GiB. Setting the value to 988 GiB should
         make QEMU fail with the error message.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'pc-q35-7.0', '-m',
                          '512,slots=1,maxmem=988G',
                          '-display', 'none',
@@ -170,6 +196,7 @@ def test_phybits_low_tcg_q35_71_amd(self):
         Make sure QEMU fails when maxmem size is 976 GiB (12 GiB less
         than 988 GiB).
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'pc-q35-7.1', '-m',
                          '512,slots=1,maxmem=976G',
                          '-display', 'none',
@@ -186,6 +213,7 @@ def test_phybits_ok_tcg_q35_70_amd(self):
         Same as q35-7.0 AMD case except that here we check that QEMU can
         successfully start when maxmem is < 988G.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'pc-q35-7.0', '-m',
                          '512,slots=1,maxmem=987.5G',
                          '-display', 'none',
@@ -202,6 +230,7 @@ def test_phybits_ok_tcg_q35_71_amd(self):
         Same as q35-7.1 AMD case except that here we check that QEMU can
         successfully start when maxmem is < 976G.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-machine', 'pc-q35-7.1', '-m',
                          '512,slots=1,maxmem=975.5G',
                          '-display', 'none',
@@ -219,6 +248,7 @@ def test_phybits_ok_tcg_q35_71_intel(self):
         Intel cpu instead. QEMU should start fine in this case as
         "above_4G" memory starts at 4G.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-cpu', 'Skylake-Server',
                          '-machine', 'pc-q35-7.1', '-m',
                          '512,slots=1,maxmem=976G',
@@ -243,6 +273,7 @@ def test_phybits_low_tcg_q35_71_amd_41bits(self):
         memory for the VM (1024 - 32 - 1 + 0.5). With 992 GiB, QEMU should
         fail to start.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-cpu', 'EPYC-v4,phys-bits=41',
                          '-machine', 'pc-q35-7.1', '-m',
                          '512,slots=1,maxmem=992G',
@@ -261,6 +292,7 @@ def test_phybits_ok_tcg_q35_71_amd_41bits(self):
         Same as above but by setting maxram between 976 GiB and 992 Gib,
         QEMU should start fine.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-cpu', 'EPYC-v4,phys-bits=41',
                          '-machine', 'pc-q35-7.1', '-m',
                          '512,slots=1,maxmem=990G',
@@ -281,6 +313,7 @@ def test_phybits_low_tcg_q35_intel_cxl(self):
         So maxmem here should be at most 986 GiB considering all memory boundary
         alignment constraints with 40 bits (1 TiB) of processor physical bits.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-cpu', 'Skylake-Server,phys-bits=40',
                          '-machine', 'q35,cxl=on', '-m',
                          '512,slots=1,maxmem=987G',
@@ -299,6 +332,7 @@ def test_phybits_ok_tcg_q35_intel_cxl(self):
         with the exact same parameters as above, QEMU should start fine even
         with cxl enabled.
         """
+        self.ensure_64bit_binary()
         self.vm.add_args('-S', '-cpu', 'Skylake-Server,phys-bits=40',
                          '-machine', 'q35,cxl=on', '-m',
                          '512,slots=1,maxmem=987G',
-- 
2.48.1

[PATCH v2 6/7] tests/functional: drop unused 'get_tag' method

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/tuxruntest.py | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/tests/functional/qemu_test/tuxruntest.py b/tests/functional/qemu_test/tuxruntest.py
index 41a4945a14..ad74156f9c 100644
--- a/tests/functional/qemu_test/tuxruntest.py
+++ b/tests/functional/qemu_test/tuxruntest.py
@@ -24,17 +24,6 @@ class TuxRunBaselineTest(QemuSystemTest):
     # Tests are ~10-40s, allow for --debug/--enable-gcov overhead
     timeout = 100
 
-    def get_tag(self, tagname, default=None):
-        """
-        Get the metadata tag or return the default.
-        """
-        utag = self._get_unique_tag_val(tagname)
-        print(f"{tagname}/{default} -> {utag}")
-        if utag:
-            return utag
-
-        return default
-
     def setUp(self):
         super().setUp()
 
-- 
2.48.1

[PATCH v2 7/7] tests/functional: stop output from zstd command when uncompressing

[Daniel P. Berrangé]

The zstd command will print incremental decompression progress to stderr
when running. Fortunately it is not on stdout as that would confuse the
TAP parsing, but we should still not have this printed. By switching
from 'check_call' to 'run' with the check=True and capture_output=True
we'll get the desired silence on success, and on failure the raised
exception will automatically include stdout/stderr data for diagnosis
purposes.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/uncompress.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/functional/qemu_test/uncompress.py b/tests/functional/qemu_test/uncompress.py
index 76dcf22385..ce79da1b68 100644
--- a/tests/functional/qemu_test/uncompress.py
+++ b/tests/functional/qemu_test/uncompress.py
@@ -13,7 +13,7 @@
 import stat
 import shutil
 from urllib.parse import urlparse
-from subprocess import check_call, CalledProcessError
+from subprocess import run, CalledProcessError, DEVNULL
 
 from .asset import Asset
 
@@ -46,8 +46,8 @@ def zstd_uncompress(zstd_path, output_path):
         return
 
     try:
-        check_call(['zstd', "-f", "-d", zstd_path,
-                    "-o", output_path])
+        run(['zstd', "-f", "-d", zstd_path,
+             "-o", output_path], capture_output=True, check=True)
     except CalledProcessError as e:
         os.remove(output_path)
         raise Exception(
-- 
2.48.1

Re: [PATCH v2 1/7] tests/functional: remove unused 'bin_prefix' variable

[Richard Henderson]

On 2/28/25 02:27, Daniel P. Berrangé wrote:
> This was copied over from avocado but has not been used in the new
> functional tests.
> 
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   tests/functional/qemu_test/testcase.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/7] tests/functional: set 'qemu_bin' as an object level field

[Richard Henderson]

On 2/28/25 02:27, Daniel P. Berrangé wrote:
> The 'qemu_bin' field is currently set on the class, despite being
> accessed as if it were an object instance field with 'self.qemu_bin'.
> 
> This is no obvious need to have it as a class field, so move it into
> the object instance.
> 
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   docs/devel/testing/functional.rst      | 2 +-
>   tests/functional/qemu_test/testcase.py | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 3/7] tests/functional: remove all class level fields

[Richard Henderson]

On 2/28/25 02:27, Daniel P. Berrangé wrote:
> A number of fields are set at the class level on QemuBaseTest, even
> though the exact same named field is then set at the object level
> later in most cases.
> 
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   tests/functional/qemu_test/testcase.py | 6 ------
>   1 file changed, 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/7] tests/functional: reduce tuxrun maxmem to work on 32-bit hosts

[Richard Henderson]

On 2/28/25 02:27, Daniel P. Berrangé wrote:
> maxmem=4G is too large to address on 32-bit hosts, so reduce it
> to 2G since the tuxrun tests don't actually need such an elevated
> memory limit.
> 
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   tests/functional/test_ppc64_tuxrun.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 5/7] tests/functional: skip memaddr tests on 32-bit builds

[Richard Henderson]

On 2/28/25 02:27, Daniel P. Berrangé wrote:
> If the QEMU binary was built for a 32-bit ELF target we cannot run the
> memory address space tests as they all require ability to address more
> RAM that can be represented on 32-bit.
> 
> We can't use a decorator to skip the tests as we need setUp() to run to
> pick the QEMU binary, thus we must call a method at the start of each
> test to check and skip it. The functional result is effectively the
> same as using a decorator, just less pretty. This code will go away when
> 32-bit hosts are full dropped from QEMU.
> 
> The code allows any non-ELF target since all macOS versions supported
> at 64-bit only and we already dropped support for 32-bit Windows.
> 
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   tests/functional/test_mem_addr_space.py | 34 +++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/7] tests/functional: reduce tuxrun maxmem to work on 32-bit hosts

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> maxmem=4G is too large to address on 32-bit hosts, so reduce it
> to 2G since the tuxrun tests don't actually need such an elevated
> memory limit.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/test_ppc64_tuxrun.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/functional/test_ppc64_tuxrun.py b/tests/functional/test_ppc64_tuxrun.py
> index 05c6162b5e..e8f79c676e 100755
> --- a/tests/functional/test_ppc64_tuxrun.py
> +++ b/tests/functional/test_ppc64_tuxrun.py
> @@ -64,7 +64,7 @@ def ppc64_common_tuxrun(self, kernel_asset, rootfs_asset, prefix):
>                            ',"index":1,"id":"pci.1"}')
>           self.vm.add_args('-device', '{"driver":"spapr-vscsi","id":"scsi1"'
>                            ',"reg":12288}')
> -        self.vm.add_args('-m', '2G,slots=32,maxmem=4G',
> +        self.vm.add_args('-m', '1G,slots=32,maxmem=2G',
>                            '-object', 'memory-backend-ram,id=ram1,size=1G',
>                            '-device', 'pc-dimm,id=dimm1,memdev=ram1')

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/7] tests/functional: skip memaddr tests on 32-bit builds

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> If the QEMU binary was built for a 32-bit ELF target we cannot run the
> memory address space tests as they all require ability to address more
> RAM that can be represented on 32-bit.
> 
> We can't use a decorator to skip the tests as we need setUp() to run to
> pick the QEMU binary, thus we must call a method at the start of each
> test to check and skip it. The functional result is effectively the
> same as using a decorator, just less pretty. This code will go away when
> 32-bit hosts are full dropped from QEMU.
> 
> The code allows any non-ELF target since all macOS versions supported
> at 64-bit only and we already dropped support for 32-bit Windows.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/test_mem_addr_space.py | 34 +++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 6/7] tests/functional: drop unused 'get_tag' method

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/tuxruntest.py | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/tuxruntest.py b/tests/functional/qemu_test/tuxruntest.py
> index 41a4945a14..ad74156f9c 100644
> --- a/tests/functional/qemu_test/tuxruntest.py
> +++ b/tests/functional/qemu_test/tuxruntest.py
> @@ -24,17 +24,6 @@ class TuxRunBaselineTest(QemuSystemTest):
>       # Tests are ~10-40s, allow for --debug/--enable-gcov overhead
>       timeout = 100
>   
> -    def get_tag(self, tagname, default=None):
> -        """
> -        Get the metadata tag or return the default.
> -        """
> -        utag = self._get_unique_tag_val(tagname)
> -        print(f"{tagname}/{default} -> {utag}")
> -        if utag:
> -            return utag
> -
> -        return default

A remainder from the avocado days... can be removed now, indeed.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 7/7] tests/functional: stop output from zstd command when uncompressing

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> The zstd command will print incremental decompression progress to stderr
> when running. Fortunately it is not on stdout as that would confuse the
> TAP parsing, but we should still not have this printed. By switching
> from 'check_call' to 'run' with the check=True and capture_output=True
> we'll get the desired silence on success, and on failure the raised
> exception will automatically include stdout/stderr data for diagnosis
> purposes.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/uncompress.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/uncompress.py b/tests/functional/qemu_test/uncompress.py
> index 76dcf22385..ce79da1b68 100644
> --- a/tests/functional/qemu_test/uncompress.py
> +++ b/tests/functional/qemu_test/uncompress.py
> @@ -13,7 +13,7 @@
>   import stat
>   import shutil
>   from urllib.parse import urlparse
> -from subprocess import check_call, CalledProcessError
> +from subprocess import run, CalledProcessError, DEVNULL
>   
>   from .asset import Asset
>   
> @@ -46,8 +46,8 @@ def zstd_uncompress(zstd_path, output_path):
>           return
>   
>       try:
> -        check_call(['zstd', "-f", "-d", zstd_path,
> -                    "-o", output_path])
> +        run(['zstd', "-f", "-d", zstd_path,
> +             "-o", output_path], capture_output=True, check=True)
>       except CalledProcessError as e:
>           os.remove(output_path)
>           raise Exception(

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/7] tests/functional: skip memaddr tests on 32-bit builds

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> If the QEMU binary was built for a 32-bit ELF target we cannot run the
> memory address space tests as they all require ability to address more
> RAM that can be represented on 32-bit.
> 
> We can't use a decorator to skip the tests as we need setUp() to run to
> pick the QEMU binary, thus we must call a method at the start of each
> test to check and skip it. The functional result is effectively the
> same as using a decorator, just less pretty. This code will go away when
> 32-bit hosts are full dropped from QEMU.
> 
> The code allows any non-ELF target since all macOS versions supported
> at 64-bit only and we already dropped support for 32-bit Windows.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/test_mem_addr_space.py | 34 +++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/tests/functional/test_mem_addr_space.py b/tests/functional/test_mem_addr_space.py
> index bb0cf062ca..c8bde77e51 100755
> --- a/tests/functional/test_mem_addr_space.py
> +++ b/tests/functional/test_mem_addr_space.py
> @@ -20,6 +20,25 @@ class MemAddrCheck(QemuSystemTest):
>       # this reason.
>       DELAY_Q35_BOOT_SEQUENCE = 1
>   
> +    # This helper can go away when the 32-bit host deprecation
> +    # turns into full & final removal of support.
> +    def ensure_64bit_binary(self):
> +        with open(self.qemu_bin, "rb") as fh:
> +            ident = fh.read(4)
> +
> +            # "\x7fELF"
> +            if ident != bytes([0x7f, 0x45, 0x4C, 0x46]):
> +                # Non-ELF file implies macOS or Windows which
> +                # we already assume to be 64-bit only
> +                return
> +
> +            # bits == 1 -> 32-bit; bits == 2 -> 64-bit
> +            bits = int.from_bytes(fh.read(1))

This unfortunately fails in the Centos CI job (so I guess there's something 
different with Python 3.8):

  https://gitlab.com/thuth/qemu/-/jobs/9316861212#L131

Looking at the test log:

Traceback (most recent call last):
   File "/builds/thuth/qemu/tests/functional/test_mem_addr_space.py", line 
335, in test_phybits_ok_tcg_q35_intel_cxl
     self.ensure_64bit_binary()
   File "/builds/thuth/qemu/tests/functional/test_mem_addr_space.py", line 
36, in ensure_64bit_binary
     bits = int.from_bytes(fh.read(1))
TypeError: from_bytes() missing required argument 'byteorder' (pos 2)

Could you please have a look?

  Thanks,
   Thomas

Re: [PATCH v2 5/7] tests/functional: skip memaddr tests on 32-bit builds

[Daniel P. Berrangé]

On Wed, Mar 05, 2025 at 12:32:31PM +0100, Thomas Huth wrote:
> On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> > If the QEMU binary was built for a 32-bit ELF target we cannot run the
> > memory address space tests as they all require ability to address more
> > RAM that can be represented on 32-bit.
> > 
> > We can't use a decorator to skip the tests as we need setUp() to run to
> > pick the QEMU binary, thus we must call a method at the start of each
> > test to check and skip it. The functional result is effectively the
> > same as using a decorator, just less pretty. This code will go away when
> > 32-bit hosts are full dropped from QEMU.
> > 
> > The code allows any non-ELF target since all macOS versions supported
> > at 64-bit only and we already dropped support for 32-bit Windows.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/test_mem_addr_space.py | 34 +++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> > 
> > diff --git a/tests/functional/test_mem_addr_space.py b/tests/functional/test_mem_addr_space.py
> > index bb0cf062ca..c8bde77e51 100755
> > --- a/tests/functional/test_mem_addr_space.py
> > +++ b/tests/functional/test_mem_addr_space.py
> > @@ -20,6 +20,25 @@ class MemAddrCheck(QemuSystemTest):
> >       # this reason.
> >       DELAY_Q35_BOOT_SEQUENCE = 1
> > +    # This helper can go away when the 32-bit host deprecation
> > +    # turns into full & final removal of support.
> > +    def ensure_64bit_binary(self):
> > +        with open(self.qemu_bin, "rb") as fh:
> > +            ident = fh.read(4)
> > +
> > +            # "\x7fELF"
> > +            if ident != bytes([0x7f, 0x45, 0x4C, 0x46]):
> > +                # Non-ELF file implies macOS or Windows which
> > +                # we already assume to be 64-bit only
> > +                return
> > +
> > +            # bits == 1 -> 32-bit; bits == 2 -> 64-bit
> > +            bits = int.from_bytes(fh.read(1))
> 
> This unfortunately fails in the Centos CI job (so I guess there's something
> different with Python 3.8):
> 
>  https://gitlab.com/thuth/qemu/-/jobs/9316861212#L131
> 
> Looking at the test log:
> 
> Traceback (most recent call last):
>   File "/builds/thuth/qemu/tests/functional/test_mem_addr_space.py", line
> 335, in test_phybits_ok_tcg_q35_intel_cxl
>     self.ensure_64bit_binary()
>   File "/builds/thuth/qemu/tests/functional/test_mem_addr_space.py", line
> 36, in ensure_64bit_binary
>     bits = int.from_bytes(fh.read(1))
> TypeError: from_bytes() missing required argument 'byteorder' (pos 2)
> 
> Could you please have a look?

It will want

     bits = int.from_bytes(fh.read(1), byteorder='little')

From the docs

  https://docs.python.org/3/library/stdtypes.html#int.from_bytes

'byteorder' gained  default value only in py 3.11, so that explains the
cnetos failure with 3.8

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

Re: [PATCH v2 3/7] tests/functional: remove all class level fields

[Thomas Huth]

On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> A number of fields are set at the class level on QemuBaseTest, even
> though the exact same named field is then set at the object level
> later in most cases.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/testcase.py | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
> index 058bf270ec..5b18416bed 100644
> --- a/tests/functional/qemu_test/testcase.py
> +++ b/tests/functional/qemu_test/testcase.py
> @@ -33,12 +33,6 @@
>   
>   class QemuBaseTest(unittest.TestCase):
>   
> -    arch = None
> -
> -    workdir = None
> -    log = None
> -    logdir = None

This seems to break the ACPI test:

https://gitlab.com/thuth/qemu/-/jobs/9316861224#L164

  Thomas

Re: [PATCH v2 3/7] tests/functional: remove all class level fields

[Daniel P. Berrangé]

On Wed, Mar 05, 2025 at 12:51:16PM +0100, Thomas Huth wrote:
> On 28/02/2025 11.27, Daniel P. Berrangé wrote:
> > A number of fields are set at the class level on QemuBaseTest, even
> > though the exact same named field is then set at the object level
> > later in most cases.
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/testcase.py | 6 ------
> >   1 file changed, 6 deletions(-)
> > 
> > diff --git a/tests/functional/qemu_test/testcase.py b/tests/functional/qemu_test/testcase.py
> > index 058bf270ec..5b18416bed 100644
> > --- a/tests/functional/qemu_test/testcase.py
> > +++ b/tests/functional/qemu_test/testcase.py
> > @@ -33,12 +33,6 @@
> >   class QemuBaseTest(unittest.TestCase):
> > -    arch = None
> > -
> > -    workdir = None
> > -    log = None
> > -    logdir = None
> 
> This seems to break the ACPI test:
> 
> https://gitlab.com/thuth/qemu/-/jobs/9316861224#L164

Our current code initializes  self.log in the setUp() method.

The ACPI bits code references self.log in the __init__() method.

So historically it has been referencing the class level 'log'
property which has always been None. This means if it were
to ever trigger codepaths calling self._print_log it will
crash on referncing None. So this patch has exposed a preexisting
flaw that was dormant.

I think the fix is to move ACPI bits logic out of __init__ and
into its own setUp() method.

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