[PATCH v3 0/5] tests/acceptance: Add bFLT loader linux-user test

[PATCH v3 0/5] tests/acceptance: Add bFLT loader linux-user test

[Philippe Mathieu-Daudé]

Since v2:
- rebased tests/acceptance/avocado_qemu/__init__.py patches
- extract has_cmd() from virtiofs_submounts.py
- check cpio availability with has_cmd()

Philippe Mathieu-Daudé (5):
  tests/acceptance: Extract QemuBaseTest from Test
  tests/acceptance: Make pick_default_qemu_bin() more generic
  tests/acceptance: Introduce QemuUserTest base class
  tests/acceptance: Share useful helpers from virtiofs_submounts test
  tests/acceptance: Add bFLT loader linux-user test

 tests/acceptance/avocado_qemu/__init__.py | 102 ++++++++++++++++++++--
 tests/acceptance/load_bflt.py             |  54 ++++++++++++
 tests/acceptance/virtiofs_submounts.py    |  59 +------------
 3 files changed, 151 insertions(+), 64 deletions(-)
 create mode 100644 tests/acceptance/load_bflt.py

-- 
2.26.2

[PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Philippe Mathieu-Daudé]

The Avocado Test::fetch_asset() is handy to download artifacts
before running tests. The current class is named Test but only
tests system emulation. As we want to test user emulation,
refactor the common code as QemuBaseTest.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/avocado_qemu/__init__.py | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index df167b142cc..4f814047176 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -155,7 +155,7 @@ def exec_command_and_wait_for_pattern(test, command,
     """
     _console_interaction(test, success_message, failure_message, command + '\r')
 
-class Test(avocado.Test):
+class QemuBaseTest(avocado.Test):
     def _get_unique_tag_val(self, tag_name):
         """
         Gets a tag value, if unique for a key
@@ -188,8 +188,6 @@ def require_accelerator(self, accelerator):
                         "available" % accelerator)
 
     def setUp(self):
-        self._vms = {}
-
         self.arch = self.params.get('arch',
                                     default=self._get_unique_tag_val('arch'))
 
@@ -202,6 +200,25 @@ def setUp(self):
         if self.qemu_bin is None:
             self.cancel("No QEMU binary defined or found in the build tree")
 
+
+    def fetch_asset(self, name,
+                    asset_hash=None, algorithm=None,
+                    locations=None, expire=None,
+                    find_only=False, cancel_on_missing=True):
+        return super(QemuBaseTest, self).fetch_asset(name,
+                        asset_hash=asset_hash,
+                        algorithm=algorithm,
+                        locations=locations,
+                        expire=expire,
+                        find_only=find_only,
+                        cancel_on_missing=cancel_on_missing)
+
+# a.k.a. QemuSystemTest for system emulation...
+class Test(QemuBaseTest):
+    def setUp(self):
+        self._vms = {}
+        super(Test, self).setUp()
+
     def _new_vm(self, *args):
         self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
         vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
-- 
2.26.2

[PATCH v3 2/5] tests/acceptance: Make pick_default_qemu_bin() more generic

[Philippe Mathieu-Daudé]

Make pick_default_qemu_bin() generic to find qemu-system or
qemu-user binaries.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/avocado_qemu/__init__.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 4f814047176..08b3fa1124f 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -48,7 +48,7 @@ def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
 
 
-def pick_default_qemu_bin(arch=None):
+def pick_default_qemu_bin(bin_fmt, arch=None):
     """
     Picks the path of a QEMU binary, starting either in the current working
     directory or in the source tree root directory.
@@ -67,7 +67,7 @@ def pick_default_qemu_bin(arch=None):
     # qemu binary path does not match arch for powerpc, handle it
     if 'ppc64le' in arch:
         arch = 'ppc64'
-    qemu_bin_relative_path = "./qemu-system-%s" % arch
+    qemu_bin_relative_path = os.path.join(".", bin_fmt % arch)
     if is_readable_executable_file(qemu_bin_relative_path):
         return qemu_bin_relative_path
 
@@ -187,14 +187,14 @@ def require_accelerator(self, accelerator):
             self.cancel("%s accelerator does not seem to be "
                         "available" % accelerator)
 
-    def setUp(self):
+    def setUp(self, bin_fmt):
         self.arch = self.params.get('arch',
                                     default=self._get_unique_tag_val('arch'))
 
         self.machine = self.params.get('machine',
                                        default=self._get_unique_tag_val('machine'))
 
-        default_qemu_bin = pick_default_qemu_bin(arch=self.arch)
+        default_qemu_bin = pick_default_qemu_bin(bin_fmt, arch=self.arch)
         self.qemu_bin = self.params.get('qemu_bin',
                                         default=default_qemu_bin)
         if self.qemu_bin is None:
@@ -217,7 +217,7 @@ def fetch_asset(self, name,
 class Test(QemuBaseTest):
     def setUp(self):
         self._vms = {}
-        super(Test, self).setUp()
+        super(Test, self).setUp("qemu-system-%s")
 
     def _new_vm(self, *args):
         self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
-- 
2.26.2

[PATCH v3 3/5] tests/acceptance: Introduce QemuUserTest base class

[Philippe Mathieu-Daudé]

Similarly to the 'System' Test base class with methods for testing
system emulation, the QemuUserTest class contains methods useful to
test user-mode emulation.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/avocado_qemu/__init__.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 08b3fa1124f..b471bee66e0 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -20,6 +20,7 @@
 from avocado.utils import cloudinit
 from avocado.utils import datadrainer
 from avocado.utils import network
+from avocado.utils import process
 from avocado.utils import vmimage
 from avocado.utils.path import find_command
 
@@ -256,6 +257,19 @@ def fetch_asset(self, name,
                         find_only=find_only,
                         cancel_on_missing=cancel_on_missing)
 
+class QemuUserTest(QemuBaseTest):
+    def setUp(self):
+        self._ldpath = []
+        super(QemuUserTest, self).setUp("qemu-%s")
+
+    def add_ldpath(self, ldpath):
+        self._ldpath += [os.path.abspath(ldpath)]
+
+    def run(self, bin_path, args=[]):
+        qemu_args = " ".join(["-L %s" % ldpath for ldpath in self._ldpath])
+        bin_args = " ".join(args)
+        return process.run("%s %s %s %s" % (self.qemu_bin, qemu_args,
+                                            bin_path, bin_args))
 
 class LinuxTest(Test):
     """Facilitates having a cloud-image Linux based available.
-- 
2.26.2

[PATCH v3 4/5] tests/acceptance: Share useful helpers from virtiofs_submounts test

[Philippe Mathieu-Daudé]

Move the useful has_cmd()/has_cmds() helpers from the virtiofs
test to the avocado_qemu public class.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/avocado_qemu/__init__.py | 57 ++++++++++++++++++++++
 tests/acceptance/virtiofs_submounts.py    | 59 +----------------------
 2 files changed, 59 insertions(+), 57 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index b471bee66e0..48c705fe3d2 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -11,6 +11,7 @@
 import logging
 import os
 import shutil
+import subprocess
 import sys
 import uuid
 import tempfile
@@ -45,6 +46,62 @@
 from qemu.accel import tcg_available
 from qemu.machine import QEMUMachine
 
+def has_cmd(name, args=None):
+    """
+    This function is for use in a @avocado.skipUnless decorator, e.g.:
+
+        @skipUnless(*has_cmd('sudo -n', ('sudo', '-n', 'true')))
+        def test_something_that_needs_sudo(self):
+            ...
+    """
+
+    if args is None:
+        args = ('which', name)
+
+    try:
+        _, stderr, exitcode = run_cmd(args)
+    except Exception as e:
+        exitcode = -1
+        stderr = str(e)
+
+    if exitcode != 0:
+        cmd_line = ' '.join(args)
+        err = f'{name} required, but "{cmd_line}" failed: {stderr.strip()}'
+        return (False, err)
+    else:
+        return (True, '')
+
+def has_cmds(*cmds):
+    """
+    This function is for use in a @avocado.skipUnless decorator and
+    allows checking for the availability of multiple commands, e.g.:
+
+        @skipUnless(*has_cmds(('cmd1', ('cmd1', '--some-parameter')),
+                              'cmd2', 'cmd3'))
+        def test_something_that_needs_cmd1_and_cmd2(self):
+            ...
+    """
+
+    for cmd in cmds:
+        if isinstance(cmd, str):
+            cmd = (cmd,)
+
+        ok, errstr = has_cmd(*cmd)
+        if not ok:
+            return (False, errstr)
+
+    return (True, '')
+
+def run_cmd(args):
+    subp = subprocess.Popen(args,
+                            stdout=subprocess.PIPE,
+                            stderr=subprocess.PIPE,
+                            universal_newlines=True)
+    stdout, stderr = subp.communicate()
+    ret = subp.returncode
+
+    return (stdout, stderr, ret)
+
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
 
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 46fa65392a1..201645cd740 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -6,67 +6,12 @@
 
 from avocado import skipUnless
 from avocado_qemu import LinuxTest, BUILD_DIR
+from avocado_qemu import has_cmds
+from avocado_qemu import run_cmd
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
 
-def run_cmd(args):
-    subp = subprocess.Popen(args,
-                            stdout=subprocess.PIPE,
-                            stderr=subprocess.PIPE,
-                            universal_newlines=True)
-    stdout, stderr = subp.communicate()
-    ret = subp.returncode
-
-    return (stdout, stderr, ret)
-
-def has_cmd(name, args=None):
-    """
-    This function is for use in a @avocado.skipUnless decorator, e.g.:
-
-        @skipUnless(*has_cmd('sudo -n', ('sudo', '-n', 'true')))
-        def test_something_that_needs_sudo(self):
-            ...
-    """
-
-    if args is None:
-        args = ('which', name)
-
-    try:
-        _, stderr, exitcode = run_cmd(args)
-    except Exception as e:
-        exitcode = -1
-        stderr = str(e)
-
-    if exitcode != 0:
-        cmd_line = ' '.join(args)
-        err = f'{name} required, but "{cmd_line}" failed: {stderr.strip()}'
-        return (False, err)
-    else:
-        return (True, '')
-
-def has_cmds(*cmds):
-    """
-    This function is for use in a @avocado.skipUnless decorator and
-    allows checking for the availability of multiple commands, e.g.:
-
-        @skipUnless(*has_cmds(('cmd1', ('cmd1', '--some-parameter')),
-                              'cmd2', 'cmd3'))
-        def test_something_that_needs_cmd1_and_cmd2(self):
-            ...
-    """
-
-    for cmd in cmds:
-        if isinstance(cmd, str):
-            cmd = (cmd,)
-
-        ok, errstr = has_cmd(*cmd)
-        if not ok:
-            return (False, errstr)
-
-    return (True, '')
-
-
 class VirtiofsSubmountsTest(LinuxTest):
     """
     :avocado: tags=arch:x86_64
-- 
2.26.2

[PATCH v3 5/5] tests/acceptance: Add bFLT loader linux-user test

[Philippe Mathieu-Daudé]

Add a very quick test that runs a busybox binary in bFLT format:

  $ avocado --show=app run -t linux_user tests/acceptance/load_bflt.py
  JOB ID     : db94d5960ce564c50904d666a7e259148c27e88f
  JOB LOG    : ~/avocado/job-results/job-2019-06-25T10.52-db94d59/job.log
   (1/1) tests/acceptance/load_bflt.py:LoadBFLT.test_stm32: PASS (0.15 s)
  RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
  JOB TIME   : 0.54 s

Reviewed-by: Willian Rampazzo <willianr@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3: Check for 'cpio' (thuth)
---
 tests/acceptance/load_bflt.py | 54 +++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 tests/acceptance/load_bflt.py

diff --git a/tests/acceptance/load_bflt.py b/tests/acceptance/load_bflt.py
new file mode 100644
index 00000000000..f071a979d8e
--- /dev/null
+++ b/tests/acceptance/load_bflt.py
@@ -0,0 +1,54 @@
+# Test the bFLT loader format
+#
+# Copyright (C) 2019 Philippe Mathieu-Daudé <f4bug@amsat.org>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import bz2
+import subprocess
+
+from avocado import skipUnless
+from avocado_qemu import QemuUserTest
+from avocado_qemu import has_cmd
+
+
+class LoadBFLT(QemuUserTest):
+
+    def extract_cpio(self, cpio_path):
+        """
+        Extracts a cpio archive into the test workdir
+
+        :param cpio_path: path to the cpio archive
+        """
+        cwd = os.getcwd()
+        os.chdir(self.workdir)
+        with bz2.open(cpio_path, 'rb') as archive_cpio:
+            subprocess.run(['cpio', '-i'], input=archive_cpio.read(),
+                           stderr=subprocess.DEVNULL)
+        os.chdir(cwd)
+
+    @skipUnless(*has_cmd('cpio'))
+    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
+    def test_stm32(self):
+        """
+        :avocado: tags=arch:arm
+        :avocado: tags=linux_user
+        :avocado: tags=quick
+        """
+        # See https://elinux.org/STM32#User_Space
+        rootfs_url = ('https://elinux.org/images/5/51/'
+                      'Stm32_mini_rootfs.cpio.bz2')
+        rootfs_hash = '9f065e6ba40cce7411ba757f924f30fcc57951e6'
+        rootfs_path_bz2 = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
+        busybox_path = self.workdir + "/bin/busybox"
+
+        self.extract_cpio(rootfs_path_bz2)
+
+        res = self.run(busybox_path)
+        ver = 'BusyBox v1.24.0.git (2015-02-03 22:17:13 CET) multi-call binary.'
+        self.assertIn(ver, res.stdout_text)
+
+        res = self.run(busybox_path, ['uname', '-a'])
+        unm = 'armv7l GNU/Linux'
+        self.assertIn(unm, res.stdout_text)
-- 
2.26.2

Re: [PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Wainer dos Santos Moschetta]

Hi,

On 3/15/21 8:08 PM, Philippe Mathieu-Daudé wrote:
> The Avocado Test::fetch_asset() is handy to download artifacts
> before running tests. The current class is named Test but only
> tests system emulation. As we want to test user emulation,
> refactor the common code as QemuBaseTest.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index df167b142cc..4f814047176 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -155,7 +155,7 @@ def exec_command_and_wait_for_pattern(test, command,
>       """
>       _console_interaction(test, success_message, failure_message, command + '\r')
>   
> -class Test(avocado.Test):
> +class QemuBaseTest(avocado.Test):

The QemuBaseTest class still defines require_accelerator() which is only 
used by qemu-system tests (thus, it should rather live on the Test 
class). Same thing for self.machine, unless that property is used on 
qemu-user tests.

>       def _get_unique_tag_val(self, tag_name):
>           """
>           Gets a tag value, if unique for a key
> @@ -188,8 +188,6 @@ def require_accelerator(self, accelerator):
>                           "available" % accelerator)
>   
>       def setUp(self):
> -        self._vms = {}
> -
>           self.arch = self.params.get('arch',
>                                       default=self._get_unique_tag_val('arch'))
>   
> @@ -202,6 +200,25 @@ def setUp(self):
>           if self.qemu_bin is None:
>               self.cancel("No QEMU binary defined or found in the build tree")
>   
> +
> +    def fetch_asset(self, name,
> +                    asset_hash=None, algorithm=None,
> +                    locations=None, expire=None,
> +                    find_only=False, cancel_on_missing=True):
> +        return super(QemuBaseTest, self).fetch_asset(name,
> +                        asset_hash=asset_hash,
> +                        algorithm=algorithm,
> +                        locations=locations,
> +                        expire=expire,
> +                        find_only=find_only,
> +                        cancel_on_missing=cancel_on_missing)
Do you overwrite this fetch_asset() on class Test on purpose? I didn't 
get why fetch_asset() is defined on the classes inherited from QemuBaseTest.
> +
> +# a.k.a. QemuSystemTest for system emulation...
Above comment could become the class docstring.
> +class Test(QemuBaseTest):
> +    def setUp(self):
> +        self._vms = {}
> +        super(Test, self).setUp()
> +
>       def _new_vm(self, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
>           vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)

Re: [PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Philippe Mathieu-Daudé]

On 3/17/21 2:08 PM, Wainer dos Santos Moschetta wrote:> On 3/15/21 8:08
PM, Philippe Mathieu-Daudé wrote:
>> The Avocado Test::fetch_asset() is handy to download artifacts
>> before running tests. The current class is named Test but only
>> tests system emulation. As we want to test user emulation,
>> refactor the common code as QemuBaseTest.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   tests/acceptance/avocado_qemu/__init__.py | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py
>> b/tests/acceptance/avocado_qemu/__init__.py
>> index df167b142cc..4f814047176 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -155,7 +155,7 @@ def exec_command_and_wait_for_pattern(test, command,
>>       """
>>       _console_interaction(test, success_message, failure_message,
>> command + '\r')
>>   -class Test(avocado.Test):
>> +class QemuBaseTest(avocado.Test):
> 
> The QemuBaseTest class still defines require_accelerator() which is only
> used by qemu-system tests (thus, it should rather live on the Test
> class). Same thing for self.machine, unless that property is used on
> qemu-user tests.

require_accelerator() has been added recently (efe30d5011b7, 2021-02-03)
and this patch is 2 years old, so I missed that while rebasing.

> 
>>       def _get_unique_tag_val(self, tag_name):
>>           """
>>           Gets a tag value, if unique for a key
>> @@ -188,8 +188,6 @@ def require_accelerator(self, accelerator):
>>                           "available" % accelerator)
>>         def setUp(self):
>> -        self._vms = {}
>> -
>>           self.arch = self.params.get('arch',
>>                                      
>> default=self._get_unique_tag_val('arch'))
>>   @@ -202,6 +200,25 @@ def setUp(self):
>>           if self.qemu_bin is None:
>>               self.cancel("No QEMU binary defined or found in the
>> build tree")
>>   +
>> +    def fetch_asset(self, name,
>> +                    asset_hash=None, algorithm=None,
>> +                    locations=None, expire=None,
>> +                    find_only=False, cancel_on_missing=True):
>> +        return super(QemuBaseTest, self).fetch_asset(name,
>> +                        asset_hash=asset_hash,
>> +                        algorithm=algorithm,
>> +                        locations=locations,
>> +                        expire=expire,
>> +                        find_only=find_only,
>> +                        cancel_on_missing=cancel_on_missing)
> Do you overwrite this fetch_asset() on class Test on purpose? I didn't
> get why fetch_asset() is defined on the classes inherited from
> QemuBaseTest.

No idea. Maybe I had to do that back then, and now it is pointless.
This is why peer-review is helpful :) I'll revisit.

>> +
>> +# a.k.a. QemuSystemTest for system emulation...
> Above comment could become the class docstring.

No, there should be no comment at all. We have to see if we are OK to
rename or not. You suggested this change:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg659843.html
But the there was more discussion and we never agreed on anything.
I don't want to restart doing this change if it is ignored again,
as it was a waste of time.

>> +class Test(QemuBaseTest):
>> +    def setUp(self):
>> +        self._vms = {}
>> +        super(Test, self).setUp()
>> +
>>       def _new_vm(self, *args):
>>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
>>           vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
> 

Re: [PATCH v3 2/5] tests/acceptance: Make pick_default_qemu_bin() more generic

[Wainer dos Santos Moschetta]

Hi,

On 3/15/21 8:08 PM, Philippe Mathieu-Daudé wrote:
> Make pick_default_qemu_bin() generic to find qemu-system or
> qemu-user binaries.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 4f814047176..08b3fa1124f 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -48,7 +48,7 @@ def is_readable_executable_file(path):
>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>   
>   
> -def pick_default_qemu_bin(arch=None):
> +def pick_default_qemu_bin(bin_fmt, arch=None):
I suggest that bin_fmt defaults to "qemu-system-%" (or "qemu-system-", 
see below) so that you don't need to change the existing callers.
>       """
>       Picks the path of a QEMU binary, starting either in the current working
>       directory or in the source tree root directory.
> @@ -67,7 +67,7 @@ def pick_default_qemu_bin(arch=None):
>       # qemu binary path does not match arch for powerpc, handle it
>       if 'ppc64le' in arch:
>           arch = 'ppc64'
> -    qemu_bin_relative_path = "./qemu-system-%s" % arch
> +    qemu_bin_relative_path = os.path.join(".", bin_fmt % arch)

Above construct fails (unless I missed something):

   >>> bin_fmt="qemu-system-%"

   >>> arch="aarch64"
   >>> qemu_bin_relative_path = os.path.join(".", bin_fmt % arch)
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
   ValueError: incomplete format

Instead it could be "bin_prefix" where the value is either 
"qemu-system-" or "qemu-", then "arch" is just appended.

>       if is_readable_executable_file(qemu_bin_relative_path):
>           return qemu_bin_relative_path
>   
> @@ -187,14 +187,14 @@ def require_accelerator(self, accelerator):
>               self.cancel("%s accelerator does not seem to be "
>                           "available" % accelerator)
>   
> -    def setUp(self):
> +    def setUp(self, bin_fmt):
>           self.arch = self.params.get('arch',
>                                       default=self._get_unique_tag_val('arch'))
>   
>           self.machine = self.params.get('machine',
>                                          default=self._get_unique_tag_val('machine'))
>   
> -        default_qemu_bin = pick_default_qemu_bin(arch=self.arch)
> +        default_qemu_bin = pick_default_qemu_bin(bin_fmt, arch=self.arch)
>           self.qemu_bin = self.params.get('qemu_bin',
>                                           default=default_qemu_bin)
>           if self.qemu_bin is None:
> @@ -217,7 +217,7 @@ def fetch_asset(self, name,
>   class Test(QemuBaseTest):
>       def setUp(self):
>           self._vms = {}
> -        super(Test, self).setUp()
> +        super(Test, self).setUp("qemu-system-%s")
>   
>       def _new_vm(self, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")

Re: [PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Wainer dos Santos Moschetta]

On 3/17/21 10:26 AM, Philippe Mathieu-Daudé wrote:
> On 3/17/21 2:08 PM, Wainer dos Santos Moschetta wrote:> On 3/15/21 8:08
> PM, Philippe Mathieu-Daudé wrote:
>>> The Avocado Test::fetch_asset() is handy to download artifacts
>>> before running tests. The current class is named Test but only
>>> tests system emulation. As we want to test user emulation,
>>> refactor the common code as QemuBaseTest.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>    tests/acceptance/avocado_qemu/__init__.py | 23 ++++++++++++++++++++---
>>>    1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py
>>> b/tests/acceptance/avocado_qemu/__init__.py
>>> index df167b142cc..4f814047176 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -155,7 +155,7 @@ def exec_command_and_wait_for_pattern(test, command,
>>>        """
>>>        _console_interaction(test, success_message, failure_message,
>>> command + '\r')
>>>    -class Test(avocado.Test):
>>> +class QemuBaseTest(avocado.Test):
>> The QemuBaseTest class still defines require_accelerator() which is only
>> used by qemu-system tests (thus, it should rather live on the Test
>> class). Same thing for self.machine, unless that property is used on
>> qemu-user tests.
> require_accelerator() has been added recently (efe30d5011b7, 2021-02-03)
> and this patch is 2 years old, so I missed that while rebasing.
>
>>>        def _get_unique_tag_val(self, tag_name):
>>>            """
>>>            Gets a tag value, if unique for a key
>>> @@ -188,8 +188,6 @@ def require_accelerator(self, accelerator):
>>>                            "available" % accelerator)
>>>          def setUp(self):
>>> -        self._vms = {}
>>> -
>>>            self.arch = self.params.get('arch',
>>>                                       
>>> default=self._get_unique_tag_val('arch'))
>>>    @@ -202,6 +200,25 @@ def setUp(self):
>>>            if self.qemu_bin is None:
>>>                self.cancel("No QEMU binary defined or found in the
>>> build tree")
>>>    +
>>> +    def fetch_asset(self, name,
>>> +                    asset_hash=None, algorithm=None,
>>> +                    locations=None, expire=None,
>>> +                    find_only=False, cancel_on_missing=True):
>>> +        return super(QemuBaseTest, self).fetch_asset(name,
>>> +                        asset_hash=asset_hash,
>>> +                        algorithm=algorithm,
>>> +                        locations=locations,
>>> +                        expire=expire,
>>> +                        find_only=find_only,
>>> +                        cancel_on_missing=cancel_on_missing)
>> Do you overwrite this fetch_asset() on class Test on purpose? I didn't
>> get why fetch_asset() is defined on the classes inherited from
>> QemuBaseTest.
> No idea. Maybe I had to do that back then, and now it is pointless.
> This is why peer-review is helpful :) I'll revisit.
>
>>> +
>>> +# a.k.a. QemuSystemTest for system emulation...
>> Above comment could become the class docstring.
> No, there should be no comment at all. We have to see if we are OK to
> rename or not. You suggested this change:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg659843.html
> But the there was more discussion and we never agreed on anything.
> I don't want to restart doing this change if it is ignored again,
> as it was a waste of time.

I stand my suggestion to rename the Test class. However, IMO this could 
be done on a separate series later after we get this merged, so to avoid 
more discussion. I can work on it myself. Sound as a good plan for you?

>
>>> +class Test(QemuBaseTest):
>>> +    def setUp(self):
>>> +        self._vms = {}
>>> +        super(Test, self).setUp()
>>> +
>>>        def _new_vm(self, *args):
>>>            self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
>>>            vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)

Re: [PATCH v3 3/5] tests/acceptance: Introduce QemuUserTest base class

[Wainer dos Santos Moschetta]

Hi,

On 3/15/21 8:08 PM, Philippe Mathieu-Daudé wrote:
> Similarly to the 'System' Test base class with methods for testing
> system emulation, the QemuUserTest class contains methods useful to
> test user-mode emulation.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 08b3fa1124f..b471bee66e0 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -20,6 +20,7 @@
>   from avocado.utils import cloudinit
>   from avocado.utils import datadrainer
>   from avocado.utils import network
> +from avocado.utils import process
>   from avocado.utils import vmimage
>   from avocado.utils.path import find_command
>   
> @@ -256,6 +257,19 @@ def fetch_asset(self, name,
>                           find_only=find_only,
>                           cancel_on_missing=cancel_on_missing)
>   
> +class QemuUserTest(QemuBaseTest):
> +    def setUp(self):
> +        self._ldpath = []
> +        super(QemuUserTest, self).setUp("qemu-%s")

There is my comment on patch 02 regarding the setUp() argument. Apart 
from that, this code looks good to me:

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

> +
> +    def add_ldpath(self, ldpath):
> +        self._ldpath += [os.path.abspath(ldpath)]
> +
> +    def run(self, bin_path, args=[]):
> +        qemu_args = " ".join(["-L %s" % ldpath for ldpath in self._ldpath])
> +        bin_args = " ".join(args)
> +        return process.run("%s %s %s %s" % (self.qemu_bin, qemu_args,
> +                                            bin_path, bin_args))
>   
>   class LinuxTest(Test):
>       """Facilitates having a cloud-image Linux based available.

Re: [PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Philippe Mathieu-Daudé]

On 3/17/21 2:44 PM, Wainer dos Santos Moschetta wrote:> On 3/17/21 10:26
AM, Philippe Mathieu-Daudé wrote:
>> On 3/17/21 2:08 PM, Wainer dos Santos Moschetta wrote:> On 3/15/21 8:08
>> PM, Philippe Mathieu-Daudé wrote:
>>>> The Avocado Test::fetch_asset() is handy to download artifacts
>>>> before running tests. The current class is named Test but only
>>>> tests system emulation. As we want to test user emulation,
>>>> refactor the common code as QemuBaseTest.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>    tests/acceptance/avocado_qemu/__init__.py | 23
>>>> ++++++++++++++++++++---
>>>>    1 file changed, 20 insertions(+), 3 deletions(-)

>>>> +# a.k.a. QemuSystemTest for system emulation...
>>> Above comment could become the class docstring.
>> No, there should be no comment at all. We have to see if we are OK to
>> rename or not. You suggested this change:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg659843.html
>> But the there was more discussion and we never agreed on anything.
>> I don't want to restart doing this change if it is ignored again,
>> as it was a waste of time.
> 
> I stand my suggestion to rename the Test class. However, IMO this could
> be done on a separate series later after we get this merged, so to avoid
> more discussion. I can work on it myself. Sound as a good plan for you?

Sure, certainly! Thanks.

Re: [PATCH v3 4/5] tests/acceptance: Share useful helpers from virtiofs_submounts test

[Wainer dos Santos Moschetta]

On 3/15/21 8:08 PM, Philippe Mathieu-Daudé wrote:
> Move the useful has_cmd()/has_cmds() helpers from the virtiofs
> test to the avocado_qemu public class.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 57 ++++++++++++++++++++++
>   tests/acceptance/virtiofs_submounts.py    | 59 +----------------------
>   2 files changed, 59 insertions(+), 57 deletions(-)

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index b471bee66e0..48c705fe3d2 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -11,6 +11,7 @@
>   import logging
>   import os
>   import shutil
> +import subprocess
>   import sys
>   import uuid
>   import tempfile
> @@ -45,6 +46,62 @@
>   from qemu.accel import tcg_available
>   from qemu.machine import QEMUMachine
>   
> +def has_cmd(name, args=None):
> +    """
> +    This function is for use in a @avocado.skipUnless decorator, e.g.:
> +
> +        @skipUnless(*has_cmd('sudo -n', ('sudo', '-n', 'true')))
> +        def test_something_that_needs_sudo(self):
> +            ...
> +    """
> +
> +    if args is None:
> +        args = ('which', name)
> +
> +    try:
> +        _, stderr, exitcode = run_cmd(args)
> +    except Exception as e:
> +        exitcode = -1
> +        stderr = str(e)
> +
> +    if exitcode != 0:
> +        cmd_line = ' '.join(args)
> +        err = f'{name} required, but "{cmd_line}" failed: {stderr.strip()}'
> +        return (False, err)
> +    else:
> +        return (True, '')
> +
> +def has_cmds(*cmds):
> +    """
> +    This function is for use in a @avocado.skipUnless decorator and
> +    allows checking for the availability of multiple commands, e.g.:
> +
> +        @skipUnless(*has_cmds(('cmd1', ('cmd1', '--some-parameter')),
> +                              'cmd2', 'cmd3'))
> +        def test_something_that_needs_cmd1_and_cmd2(self):
> +            ...
> +    """
> +
> +    for cmd in cmds:
> +        if isinstance(cmd, str):
> +            cmd = (cmd,)
> +
> +        ok, errstr = has_cmd(*cmd)
> +        if not ok:
> +            return (False, errstr)
> +
> +    return (True, '')
> +
> +def run_cmd(args):
> +    subp = subprocess.Popen(args,
> +                            stdout=subprocess.PIPE,
> +                            stderr=subprocess.PIPE,
> +                            universal_newlines=True)
> +    stdout, stderr = subp.communicate()
> +    ret = subp.returncode
> +
> +    return (stdout, stderr, ret)
> +
>   def is_readable_executable_file(path):
>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>   
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 46fa65392a1..201645cd740 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -6,67 +6,12 @@
>   
>   from avocado import skipUnless
>   from avocado_qemu import LinuxTest, BUILD_DIR
> +from avocado_qemu import has_cmds
> +from avocado_qemu import run_cmd
>   from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import ssh
>   
>   
> -def run_cmd(args):
> -    subp = subprocess.Popen(args,
> -                            stdout=subprocess.PIPE,
> -                            stderr=subprocess.PIPE,
> -                            universal_newlines=True)
> -    stdout, stderr = subp.communicate()
> -    ret = subp.returncode
> -
> -    return (stdout, stderr, ret)
> -
> -def has_cmd(name, args=None):
> -    """
> -    This function is for use in a @avocado.skipUnless decorator, e.g.:
> -
> -        @skipUnless(*has_cmd('sudo -n', ('sudo', '-n', 'true')))
> -        def test_something_that_needs_sudo(self):
> -            ...
> -    """
> -
> -    if args is None:
> -        args = ('which', name)
> -
> -    try:
> -        _, stderr, exitcode = run_cmd(args)
> -    except Exception as e:
> -        exitcode = -1
> -        stderr = str(e)
> -
> -    if exitcode != 0:
> -        cmd_line = ' '.join(args)
> -        err = f'{name} required, but "{cmd_line}" failed: {stderr.strip()}'
> -        return (False, err)
> -    else:
> -        return (True, '')
> -
> -def has_cmds(*cmds):
> -    """
> -    This function is for use in a @avocado.skipUnless decorator and
> -    allows checking for the availability of multiple commands, e.g.:
> -
> -        @skipUnless(*has_cmds(('cmd1', ('cmd1', '--some-parameter')),
> -                              'cmd2', 'cmd3'))
> -        def test_something_that_needs_cmd1_and_cmd2(self):
> -            ...
> -    """
> -
> -    for cmd in cmds:
> -        if isinstance(cmd, str):
> -            cmd = (cmd,)
> -
> -        ok, errstr = has_cmd(*cmd)
> -        if not ok:
> -            return (False, errstr)
> -
> -    return (True, '')
> -
> -
>   class VirtiofsSubmountsTest(LinuxTest):
>       """
>       :avocado: tags=arch:x86_64