[PATCH v2 0/2] tests/acceptance: Use QEMUMachine on tests that expect failure

[PATCH v2 0/2] tests/acceptance: Use QEMUMachine on tests that expect failure

[Wainer dos Santos Moschetta]

The linux_initrd and empty_cpu_model tests assert that QEMU exit
with failure on certain scenarios. Currently they are not able
to use QEMUMachine object due to the QMP monitor connection which
is tentatively established always. Instead they handle the QEMU binary
directy, but ideally they should use QEMUMachine in order to:
a) Take advantage of error handling and logging in QEMUMachine.
b) Follow the pattern in other acceptance tests.

Changes v2 -> v1:
- set_qmp_monitor() uses enabled=True instead of disabled=False
  (patch 01) [crosa]
- removed monitor_address parameter from set_qmp_monitor()
  (patch 01) [crosa]
- adjusted to use 'enabled' (patch 02) [crosa]

Changes v1 -> null:
- Patch 01 first appeared in [1]. Here I propose the same
implementation but the code was rebased.
- empty_cpu_model used to check both stdout and stderr of the process. The
QEMUMachine doesn't provide an interface to access the underneath
process object, instead the process' output is available through
get_log(). This method in turn returns the content of stdout (stderr is
redirected to stdout). Therefore, I adapted the assertion statement
to check the '-cpu option cannot be empty' message shows in the output.

Git:
- Tree: https://github.com/wainersm/qemu
- Branch: tests_without_qmp_v2

CI:
- Travis (PASS): https://travis-ci.org/wainersm/qemu/builds/623716505

References:
[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg627498.html


Wainer dos Santos Moschetta (2):
  python/qemu: Add set_qmp_monitor() to QEMUMachine
  tests/acceptance: Makes linux_initrd and empty_cpu_model use
    QEMUMachine

 python/qemu/machine.py              | 66 +++++++++++++++++++----------
 tests/acceptance/empty_cpu_model.py | 12 +++---
 tests/acceptance/linux_initrd.py    | 13 +++---
 3 files changed, 56 insertions(+), 35 deletions(-)

-- 
2.21.0

[PATCH v2 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Wainer dos Santos Moschetta]

The QEMUMachine VM has a monitor setup on which an QMP
connection is always attempted on _post_launch() (executed
by launch()). In case the QEMU process immediatly exits
then the qmp.accept() (used to establish the connection) stalls
until it reaches timeout and consequently an exception raises.

That behavior is undesirable when, for instance, it needs to
gather information from the QEMU binary ($ qemu -cpu list) or a
test which launches the VM expecting its failure.

This patch adds the set_qmp_monitor() method to QEMUMachine that
allows turn off the creation of the monitor machinery on VM launch.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
 python/qemu/machine.py | 66 +++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index a4631d6934..7d4d621a42 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -104,6 +104,7 @@ class QEMUMachine(object):
         self._events = []
         self._iolog = None
         self._socket_scm_helper = socket_scm_helper
+        self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp = None
         self._qemu_full_args = None
         self._test_dir = test_dir
@@ -228,15 +229,16 @@ class QEMUMachine(object):
                 self._iolog = iolog.read()
 
     def _base_args(self):
-        if isinstance(self._monitor_address, tuple):
-            moncdev = "socket,id=mon,host=%s,port=%s" % (
+        args = ['-display', 'none', '-vga', 'none']
+        if self._qmp_set:
+            if isinstance(self._monitor_address, tuple):
+                moncdev = "socket,id=mon,host=%s,port=%s" % (
                 self._monitor_address[0],
                 self._monitor_address[1])
-        else:
-            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
-        args = ['-chardev', moncdev,
-                '-mon', 'chardev=mon,mode=control',
-                '-display', 'none', '-vga', 'none']
+            else:
+                moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
+            args.extend(['-chardev', moncdev, '-mon',
+                         'chardev=mon,mode=control'])
         if self._machine is not None:
             args.extend(['-machine', self._machine])
         if self._console_set:
@@ -255,20 +257,21 @@ class QEMUMachine(object):
 
     def _pre_launch(self):
         self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
-        if self._monitor_address is not None:
-            self._vm_monitor = self._monitor_address
-        else:
-            self._vm_monitor = os.path.join(self._sock_dir,
-                                            self._name + "-monitor.sock")
-            self._remove_files.append(self._vm_monitor)
         self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
-        self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
-                                            server=True)
+        if self._qmp_set:
+            if self._monitor_address is not None:
+                self._vm_monitor = self._monitor_address
+            else:
+                self._vm_monitor = os.path.join(self._sock_dir,
+                                                self._name + "-monitor.sock")
+                self._remove_files.append(self._vm_monitor)
+            self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True)
 
     def _post_launch(self):
-        self._qmp.accept()
+        if self._qmp:
+            self._qmp.accept()
 
     def _post_shutdown(self):
         if self._qemu_log_file is not None:
@@ -330,7 +333,8 @@ class QEMUMachine(object):
         Wait for the VM to power off
         """
         self._popen.wait()
-        self._qmp.close()
+        if self._qmp:
+            self._qmp.close()
         self._load_io_log()
         self._post_shutdown()
 
@@ -346,12 +350,13 @@ class QEMUMachine(object):
             self._console_socket = None
 
         if self.is_running():
-            try:
-                if not has_quit:
-                    self._qmp.cmd('quit')
-                self._qmp.close()
-            except:
-                self._popen.kill()
+            if self._qmp:
+                try:
+                    if not has_quit:
+                        self._qmp.cmd('quit')
+                    self._qmp.close()
+                except:
+                    self._popen.kill()
             self._popen.wait()
 
         self._load_io_log()
@@ -368,6 +373,21 @@ class QEMUMachine(object):
 
         self._launched = False
 
+    def set_qmp_monitor(self, enabled=True):
+        """
+        Set the QMP monitor.
+
+        @param enabled: if False, qmp monitor options will be removed from
+                        the base arguments of the resulting QEMU command
+                        line. Default is True.
+        @note: call this function before launch().
+        """
+        if enabled:
+            self._qmp_set = True
+        else:
+            self._qmp_set = False
+            self._qmp = None
+
     def qmp(self, cmd, conv_keys=True, **args):
         """
         Invoke a QMP command and return the response dict
-- 
2.21.0

Re: [PATCH v2 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]

On Wed, Dec 11, 2019 at 01:55:35PM -0500, Wainer dos Santos Moschetta wrote:
> The QEMUMachine VM has a monitor setup on which an QMP
> connection is always attempted on _post_launch() (executed
> by launch()). In case the QEMU process immediatly exits
> then the qmp.accept() (used to establish the connection) stalls
> until it reaches timeout and consequently an exception raises.
> 
> That behavior is undesirable when, for instance, it needs to
> gather information from the QEMU binary ($ qemu -cpu list) or a
> test which launches the VM expecting its failure.
> 
> This patch adds the set_qmp_monitor() method to QEMUMachine that
> allows turn off the creation of the monitor machinery on VM launch.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
>  python/qemu/machine.py | 66 +++++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index a4631d6934..7d4d621a42 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -104,6 +104,7 @@ class QEMUMachine(object):
>          self._events = []
>          self._iolog = None
>          self._socket_scm_helper = socket_scm_helper
> +        self._qmp_set = True   # Enable QMP monitor by default.
>          self._qmp = None
>          self._qemu_full_args = None
>          self._test_dir = test_dir
> @@ -228,15 +229,16 @@ class QEMUMachine(object):
>                  self._iolog = iolog.read()
>  
>      def _base_args(self):
> -        if isinstance(self._monitor_address, tuple):
> -            moncdev = "socket,id=mon,host=%s,port=%s" % (
> +        args = ['-display', 'none', '-vga', 'none']
> +        if self._qmp_set:
> +            if isinstance(self._monitor_address, tuple):
> +                moncdev = "socket,id=mon,host=%s,port=%s" % (
>                  self._monitor_address[0],
>                  self._monitor_address[1])

One thing I missed in my review on v1 was this now became badly
indented.  No worries, it's a minor issue that I can fix on my side
when queueing this patch.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Wainer dos Santos Moschetta]

On 12/12/19 12:13 PM, Cleber Rosa wrote:
> On Wed, Dec 11, 2019 at 01:55:35PM -0500, Wainer dos Santos Moschetta wrote:
>> The QEMUMachine VM has a monitor setup on which an QMP
>> connection is always attempted on _post_launch() (executed
>> by launch()). In case the QEMU process immediatly exits
>> then the qmp.accept() (used to establish the connection) stalls
>> until it reaches timeout and consequently an exception raises.
>>
>> That behavior is undesirable when, for instance, it needs to
>> gather information from the QEMU binary ($ qemu -cpu list) or a
>> test which launches the VM expecting its failure.
>>
>> This patch adds the set_qmp_monitor() method to QEMUMachine that
>> allows turn off the creation of the monitor machinery on VM launch.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   python/qemu/machine.py | 66 +++++++++++++++++++++++++++---------------
>>   1 file changed, 43 insertions(+), 23 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index a4631d6934..7d4d621a42 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -104,6 +104,7 @@ class QEMUMachine(object):
>>           self._events = []
>>           self._iolog = None
>>           self._socket_scm_helper = socket_scm_helper
>> +        self._qmp_set = True   # Enable QMP monitor by default.
>>           self._qmp = None
>>           self._qemu_full_args = None
>>           self._test_dir = test_dir
>> @@ -228,15 +229,16 @@ class QEMUMachine(object):
>>                   self._iolog = iolog.read()
>>   
>>       def _base_args(self):
>> -        if isinstance(self._monitor_address, tuple):
>> -            moncdev = "socket,id=mon,host=%s,port=%s" % (
>> +        args = ['-display', 'none', '-vga', 'none']
>> +        if self._qmp_set:
>> +            if isinstance(self._monitor_address, tuple):
>> +                moncdev = "socket,id=mon,host=%s,port=%s" % (
>>                   self._monitor_address[0],
>>                   self._monitor_address[1])
> One thing I missed in my review on v1 was this now became badly
> indented.  No worries, it's a minor issue that I can fix on my side
> when queueing this patch.

Good catch. Thanks!

- Wainer

>
> - Cleber.

Re: [PATCH v2 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On Fri, Dec 13, 2019 at 10:46:17AM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 12/12/19 12:13 PM, Cleber Rosa wrote:
> > On Wed, Dec 11, 2019 at 01:55:35PM -0500, Wainer dos Santos Moschetta wrote:
> > > The QEMUMachine VM has a monitor setup on which an QMP
> > > connection is always attempted on _post_launch() (executed
> > > by launch()). In case the QEMU process immediatly exits
> > > then the qmp.accept() (used to establish the connection) stalls
> > > until it reaches timeout and consequently an exception raises.
> > > 
> > > That behavior is undesirable when, for instance, it needs to
> > > gather information from the QEMU binary ($ qemu -cpu list) or a
> > > test which launches the VM expecting its failure.
> > > 
> > > This patch adds the set_qmp_monitor() method to QEMUMachine that
> > > allows turn off the creation of the monitor machinery on VM launch.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > > ---

FIY, queued on my python-next branch.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/2] tests/acceptance: Makes linux_initrd and empty_cpu_model use QEMUMachine

[Wainer dos Santos Moschetta]

On linux_initrd and empty_cpu_model tests the same effect of
calling QEMU through run() to inspect the terminated process is
achieved with a sequence of set_qmp_monitor() / launch() / wait()
commands on an QEMUMachine object. This patch changes those
tests to use QEMUMachine instead, so they follow the same pattern
to launch QEMU found on other acceptance tests.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/empty_cpu_model.py | 12 ++++++------
 tests/acceptance/linux_initrd.py    | 13 +++++++------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/tests/acceptance/empty_cpu_model.py b/tests/acceptance/empty_cpu_model.py
index 3f4f663582..a1e59e45e4 100644
--- a/tests/acceptance/empty_cpu_model.py
+++ b/tests/acceptance/empty_cpu_model.py
@@ -7,13 +7,13 @@
 #
 # 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 subprocess
 from avocado_qemu import Test
 
 class EmptyCPUModel(Test):
     def test(self):
-        cmd = [self.qemu_bin, '-S', '-display', 'none', '-machine', 'none', '-cpu', '']
-        r = subprocess.run(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
-        self.assertEquals(r.returncode, 1, "QEMU exit code should be 1")
-        self.assertEquals(r.stdout, b'', "QEMU stdout should be empty")
-        self.assertNotEquals(r.stderr, b'', "QEMU stderr shouldn't be empty")
+        self.vm.add_args('-S', '-display', 'none', '-machine', 'none', '-cpu', '')
+        self.vm.set_qmp_monitor(enabled=False)
+        self.vm.launch()
+        self.vm.wait()
+        self.assertEquals(self.vm.exitcode(), 1, "QEMU exit code should be 1")
+        self.assertRegex(self.vm.get_log(), r'-cpu option cannot be empty')
diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
index c61d9826a4..aaa4eb9698 100644
--- a/tests/acceptance/linux_initrd.py
+++ b/tests/acceptance/linux_initrd.py
@@ -10,7 +10,6 @@
 
 import logging
 import tempfile
-from avocado.utils.process import run
 
 from avocado_qemu import Test
 
@@ -41,13 +40,15 @@ class LinuxInitrd(Test):
             initrd.seek(max_size)
             initrd.write(b'\0')
             initrd.flush()
-            cmd = "%s -kernel %s -initrd %s -m 4096" % (
-                  self.qemu_bin, kernel_path, initrd.name)
-            res = run(cmd, ignore_status=True)
-            self.assertEqual(res.exit_status, 1)
+            self.vm.add_args('-kernel', kernel_path, '-initrd', initrd.name,
+                             '-m', '4096')
+            self.vm.set_qmp_monitor(enabled=False)
+            self.vm.launch()
+            self.vm.wait()
+            self.assertEqual(self.vm.exitcode(), 1)
             expected_msg = r'.*initrd is too large.*max: \d+, need %s.*' % (
                 max_size + 1)
-            self.assertRegex(res.stderr_text, expected_msg)
+            self.assertRegex(self.vm.get_log(), expected_msg)
 
     def test_with_2gib_file_should_work_with_linux_v4_16(self):
         """
-- 
2.21.0

Re: [PATCH v2 2/2] tests/acceptance: Makes linux_initrd and empty_cpu_model use QEMUMachine

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

On Wed, Dec 11, 2019 at 01:55:36PM -0500, Wainer dos Santos Moschetta wrote:
> On linux_initrd and empty_cpu_model tests the same effect of
> calling QEMU through run() to inspect the terminated process is
> achieved with a sequence of set_qmp_monitor() / launch() / wait()
> commands on an QEMUMachine object. This patch changes those
> tests to use QEMUMachine instead, so they follow the same pattern
> to launch QEMU found on other acceptance tests.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>
> ---

FIY, queued on my python-next branch.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]