[PULL 00/19] Python patches for 5.1

[PULL 00/19] Python patches for 5.1

[Philippe Mathieu-Daudé]

The following changes since commit 1a53dfee92284d3016a579ef31d53367e84d9dd8:

  Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2020-07-13' into staging (2020-07-14 13:52:10 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/python-next-20200714

for you to fetch changes up to 84dcdf0887cdaaba7300442482c99e5064865a2d:

  python/qmp.py: add QMPProtocolError (2020-07-14 22:22:22 +0200)

----------------------------------------------------------------
Python patches for 5.1

- Reduce race conditions on QEMUMachine::shutdown()

 1. Remove the "bare except" pattern in the existing shutdown code,
    which can mask problems and make debugging difficult.
 2. Ensure that post-shutdown cleanup is always performed, even when
    graceful termination fails.
 3. Unify cleanup paths such that no matter how the VM is terminated,
    the same functions and steps are always taken to reset the object
    state.
 4. Rewrite shutdown() such that any error encountered when attempting
    a graceful shutdown will be raised as an AbnormalShutdown exception.
    The pythonic idiom is to allow the caller to decide if this is a
    problem or not.

- Modify part of the python/qemu library to comply with:

  . mypy --strict
  . pylint
  . flake8

- Script for the TCG Continuous Benchmarking project that uses
  callgrind to dissect QEMU execution into three main phases:

  . code generation
  . JIT execution
  . helpers execution

CI jobs results:
. https://cirrus-ci.com/build/5421349961203712
. https://gitlab.com/philmd/qemu/-/pipelines/166556001
. https://travis-ci.org/github/philmd/qemu/builds/708102347
----------------------------------------------------------------

Ahmed Karaman (1):
  scripts/performance: Add dissect.py script

John Snow (18):
  python/machine.py: consolidate _post_shutdown()
  python/machine.py: Close QMP socket in cleanup
  python/machine.py: Add _early_cleanup hook
  python/machine.py: Perform early cleanup for wait() calls, too
  python/machine.py: Prohibit multiple shutdown() calls
  python/machine.py: Add a configurable timeout to shutdown()
  python/machine.py: Make wait() call shutdown()
  tests/acceptance: wait() instead of shutdown() where appropriate
  tests/acceptance: Don't test reboot on cubieboard
  python/machine.py: split shutdown into hard and soft flavors
  python/machine.py: re-add sigkill warning suppression
  python/machine.py: change default wait timeout to 3 seconds
  python/qmp.py: Define common types
  iotests.py: use qemu.qmp type aliases
  python/qmp.py: re-absorb MonitorResponseError
  python/qmp.py: Do not return None from cmd_obj
  python/qmp.py: add casts to JSON deserialization
  python/qmp.py: add QMPProtocolError

 python/qemu/machine.py                   | 176 +++++++++++++++++------
 python/qemu/qmp.py                       |  67 +++++++--
 scripts/performance/dissect.py           | 166 +++++++++++++++++++++
 scripts/render_block_graph.py            |   7 +-
 tests/acceptance/boot_linux_console.py   |  14 +-
 tests/acceptance/linux_ssh_mips_malta.py |   2 +
 tests/qemu-iotests/iotests.py            |   9 +-
 7 files changed, 369 insertions(+), 72 deletions(-)
 create mode 100755 scripts/performance/dissect.py

-- 
2.21.3

[PULL 01/19] scripts/performance: Add dissect.py script

[Philippe Mathieu-Daudé]

From: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>

Python script that dissects QEMU execution into three main phases:
code generation, JIT execution and helpers execution.

Syntax:
dissect.py [-h] -- <qemu executable> [<qemu executable options>] \
                 <target executable> [<target executable options>]

[-h] - Print the script arguments help message.

Example of usage:
dissect.py -- qemu-arm coulomb_double-arm

Example output:
Total Instructions:        4,702,865,362

Code Generation:             115,819,309	 2.463%
JIT Execution:             1,081,980,528	23.007%
Helpers:                   3,505,065,525	74.530%

Signed-off-by: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200709052055.2650-2-ahmedkhaledkaraman@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 scripts/performance/dissect.py | 166 +++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100755 scripts/performance/dissect.py

diff --git a/scripts/performance/dissect.py b/scripts/performance/dissect.py
new file mode 100755
index 0000000000..bf24f50922
--- /dev/null
+++ b/scripts/performance/dissect.py
@@ -0,0 +1,166 @@
+#!/usr/bin/env python3
+
+#  Print the percentage of instructions spent in each phase of QEMU
+#  execution.
+#
+#  Syntax:
+#  dissect.py [-h] -- <qemu executable> [<qemu executable options>] \
+#                   <target executable> [<target executable options>]
+#
+#  [-h] - Print the script arguments help message.
+#
+#  Example of usage:
+#  dissect.py -- qemu-arm coulomb_double-arm
+#
+#  This file is a part of the project "TCG Continuous Benchmarking".
+#
+#  Copyright (C) 2020  Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
+#  Copyright (C) 2020  Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
+#
+#  This program is free software: you can redistribute it and/or modify
+#  it under the terms of the GNU General Public License as published by
+#  the Free Software Foundation, either version 2 of the License, or
+#  (at your option) any later version.
+#
+#  This program is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+#  GNU General Public License for more details.
+#
+#  You should have received a copy of the GNU General Public License
+#  along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+import argparse
+import os
+import subprocess
+import sys
+import tempfile
+
+
+def get_JIT_line(callgrind_data):
+    """
+    Search for the first instance of the JIT call in
+    the callgrind_annotate output when ran using --tree=caller
+    This is equivalent to the self number of instructions of JIT.
+
+    Parameters:
+    callgrind_data (list): callgrind_annotate output
+
+    Returns:
+    (int): Line number
+    """
+    line = -1
+    for i in range(len(callgrind_data)):
+        if callgrind_data[i].strip('\n') and \
+                callgrind_data[i].split()[-1] == "[???]":
+            line = i
+            break
+    if line == -1:
+        sys.exit("Couldn't locate the JIT call ... Exiting.")
+    return line
+
+
+def main():
+    # Parse the command line arguments
+    parser = argparse.ArgumentParser(
+        usage='dissect.py [-h] -- '
+        '<qemu executable> [<qemu executable options>] '
+        '<target executable> [<target executable options>]')
+
+    parser.add_argument('command', type=str, nargs='+', help=argparse.SUPPRESS)
+
+    args = parser.parse_args()
+
+    # Extract the needed variables from the args
+    command = args.command
+
+    # Insure that valgrind is installed
+    check_valgrind = subprocess.run(
+        ["which", "valgrind"], stdout=subprocess.DEVNULL)
+    if check_valgrind.returncode:
+        sys.exit("Please install valgrind before running the script.")
+
+    # Save all intermediate files in a temporary directory
+    with tempfile.TemporaryDirectory() as tmpdirname:
+        # callgrind output file path
+        data_path = os.path.join(tmpdirname, "callgrind.data")
+        # callgrind_annotate output file path
+        annotate_out_path = os.path.join(tmpdirname, "callgrind_annotate.out")
+
+        # Run callgrind
+        callgrind = subprocess.run((["valgrind",
+                                     "--tool=callgrind",
+                                     "--callgrind-out-file=" + data_path]
+                                    + command),
+                                   stdout=subprocess.DEVNULL,
+                                   stderr=subprocess.PIPE)
+        if callgrind.returncode:
+            sys.exit(callgrind.stderr.decode("utf-8"))
+
+        # Save callgrind_annotate output
+        with open(annotate_out_path, "w") as output:
+            callgrind_annotate = subprocess.run(
+                ["callgrind_annotate", data_path, "--tree=caller"],
+                stdout=output,
+                stderr=subprocess.PIPE)
+            if callgrind_annotate.returncode:
+                sys.exit(callgrind_annotate.stderr.decode("utf-8"))
+
+        # Read the callgrind_annotate output to callgrind_data[]
+        callgrind_data = []
+        with open(annotate_out_path, 'r') as data:
+            callgrind_data = data.readlines()
+
+        # Line number with the total number of instructions
+        total_instructions_line_number = 20
+        # Get the total number of instructions
+        total_instructions_line_data = \
+            callgrind_data[total_instructions_line_number]
+        total_instructions = total_instructions_line_data.split()[0]
+        total_instructions = int(total_instructions.replace(',', ''))
+
+        # Line number with the JIT self number of instructions
+        JIT_self_instructions_line_number = get_JIT_line(callgrind_data)
+        # Get the JIT self number of instructions
+        JIT_self_instructions_line_data = \
+            callgrind_data[JIT_self_instructions_line_number]
+        JIT_self_instructions = JIT_self_instructions_line_data.split()[0]
+        JIT_self_instructions = int(JIT_self_instructions.replace(',', ''))
+
+        # Line number with the JIT self + inclusive number of instructions
+        # It's the line above the first JIT call when running with --tree=caller
+        JIT_total_instructions_line_number = JIT_self_instructions_line_number-1
+        # Get the JIT self + inclusive number of instructions
+        JIT_total_instructions_line_data = \
+            callgrind_data[JIT_total_instructions_line_number]
+        JIT_total_instructions = JIT_total_instructions_line_data.split()[0]
+        JIT_total_instructions = int(JIT_total_instructions.replace(',', ''))
+
+        # Calculate number of instructions in helpers and code generation
+        helpers_instructions = JIT_total_instructions-JIT_self_instructions
+        code_generation_instructions = total_instructions-JIT_total_instructions
+
+        # Print results (Insert commas in large numbers)
+        # Print total number of instructions
+        print('{:<20}{:>20}\n'.
+              format("Total Instructions:",
+                     format(total_instructions, ',')))
+        # Print code generation instructions and percentage
+        print('{:<20}{:>20}\t{:>6.3f}%'.
+              format("Code Generation:",
+                     format(code_generation_instructions, ","),
+                     (code_generation_instructions / total_instructions) * 100))
+        # Print JIT instructions and percentage
+        print('{:<20}{:>20}\t{:>6.3f}%'.
+              format("JIT Execution:",
+                     format(JIT_self_instructions, ","),
+                     (JIT_self_instructions / total_instructions) * 100))
+        # Print helpers instructions and percentage
+        print('{:<20}{:>20}\t{:>6.3f}%'.
+              format("Helpers:",
+                     format(helpers_instructions, ","),
+                     (helpers_instructions/total_instructions)*100))
+
+
+if __name__ == "__main__":
+    main()
-- 
2.21.3

[PULL 02/19] python/machine.py: consolidate _post_shutdown()

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

Move more cleanup actions into _post_shutdown. As a change, if QEMU
should so happen to be terminated during a call to wait(), that event
will now be logged.

This is not likely to occur during normative use.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200710050649.32434-2-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c25f0b42cf..ca1f2114e6 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -294,6 +294,8 @@ def _post_launch(self):
             self._qmp.accept()
 
     def _post_shutdown(self):
+        self._load_io_log()
+
         if self._qemu_log_file is not None:
             self._qemu_log_file.close()
             self._qemu_log_file = None
@@ -307,6 +309,17 @@ def _post_shutdown(self):
         while len(self._remove_files) > 0:
             self._remove_if_exists(self._remove_files.pop())
 
+        exitcode = self.exitcode()
+        if exitcode is not None and exitcode < 0:
+            msg = 'qemu received signal %i; command: "%s"'
+            if self._qemu_full_args:
+                command = ' '.join(self._qemu_full_args)
+            else:
+                command = ''
+            LOG.warning(msg, -int(exitcode), command)
+
+        self._launched = False
+
     def launch(self):
         """
         Launch the VM and make sure we cleanup and expose the
@@ -355,7 +368,6 @@ def wait(self):
         self._popen.wait()
         if self._qmp:
             self._qmp.close()
-        self._load_io_log()
         self._post_shutdown()
 
     def shutdown(self, has_quit=False, hard=False):
@@ -382,21 +394,8 @@ def shutdown(self, has_quit=False, hard=False):
                     self._popen.kill()
             self._popen.wait()
 
-        self._load_io_log()
         self._post_shutdown()
 
-        exitcode = self.exitcode()
-        if exitcode is not None and exitcode < 0 and \
-                not (exitcode == -9 and hard):
-            msg = 'qemu received signal %i: %s'
-            if self._qemu_full_args:
-                command = ' '.join(self._qemu_full_args)
-            else:
-                command = ''
-            LOG.warning(msg, -int(exitcode), command)
-
-        self._launched = False
-
     def kill(self):
         self.shutdown(hard=True)
 
-- 
2.21.3

[PULL 03/19] python/machine.py: Close QMP socket in cleanup

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

It's not important to do this before waiting for the process to exit, so
it can be done during generic post-shutdown cleanup.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-3-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index ca1f2114e6..d3faa9a84c 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -294,6 +294,10 @@ def _post_launch(self):
             self._qmp.accept()
 
     def _post_shutdown(self):
+        if self._qmp:
+            self._qmp.close()
+            self._qmp = None
+
         self._load_io_log()
 
         if self._qemu_log_file is not None:
@@ -366,8 +370,6 @@ def wait(self):
         Wait for the VM to power off
         """
         self._popen.wait()
-        if self._qmp:
-            self._qmp.close()
         self._post_shutdown()
 
     def shutdown(self, has_quit=False, hard=False):
@@ -388,7 +390,6 @@ def shutdown(self, has_quit=False, hard=False):
                 try:
                     if not has_quit:
                         self._qmp.cmd('quit')
-                    self._qmp.close()
                     self._popen.wait(timeout=3)
                 except:
                     self._popen.kill()
-- 
2.21.3

[PULL 04/19] python/machine.py: Add _early_cleanup hook

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

Some parts of cleanup need to occur prior to shutdown, otherwise
shutdown might break. Move this into a suitably named method/callback.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-4-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index d3faa9a84c..127926b276 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -365,6 +365,17 @@ def _launch(self):
                                        close_fds=False)
         self._post_launch()
 
+    def _early_cleanup(self) -> None:
+        """
+        Perform any cleanup that needs to happen before the VM exits.
+        """
+        # If we keep the console socket open, we may deadlock waiting
+        # for QEMU to exit, while QEMU is waiting for the socket to
+        # become writeable.
+        if self._console_socket is not None:
+            self._console_socket.close()
+            self._console_socket = None
+
     def wait(self):
         """
         Wait for the VM to power off
@@ -376,12 +387,7 @@ def shutdown(self, has_quit=False, hard=False):
         """
         Terminate the VM and clean up
         """
-        # If we keep the console socket open, we may deadlock waiting
-        # for QEMU to exit, while QEMU is waiting for the socket to
-        # become writeable.
-        if self._console_socket is not None:
-            self._console_socket.close()
-            self._console_socket = None
+        self._early_cleanup()
 
         if self.is_running():
             if hard:
-- 
2.21.3

[PULL 05/19] python/machine.py: Perform early cleanup for wait() calls, too

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

This is primarily for consistency, and is a step towards wait() and
shutdown() sharing the same implementation so that the two cleanup paths
cannot diverge.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-5-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 127926b276..63e40879e2 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -380,6 +380,7 @@ def wait(self):
         """
         Wait for the VM to power off
         """
+        self._early_cleanup()
         self._popen.wait()
         self._post_shutdown()
 
-- 
2.21.3

[PULL 06/19] python/machine.py: Prohibit multiple shutdown() calls

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

If the VM is not launched, don't try to shut it down. As a change,
_post_shutdown now unconditionally also calls _early_cleanup in order to
offer comprehensive object cleanup in failure cases.

As a courtesy, treat it as a NOP instead of rejecting it as an
error. This is slightly nicer for acceptance tests where vm.shutdown()
is issued unconditionally in tearDown callbacks.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200710050649.32434-6-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 63e40879e2..c28957ee82 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -294,6 +294,13 @@ def _post_launch(self):
             self._qmp.accept()
 
     def _post_shutdown(self):
+        """
+        Called to cleanup the VM instance after the process has exited.
+        May also be called after a failed launch.
+        """
+        # Comprehensive reset for the failed launch case:
+        self._early_cleanup()
+
         if self._qmp:
             self._qmp.close()
             self._qmp = None
@@ -339,7 +346,7 @@ def launch(self):
             self._launch()
             self._launched = True
         except:
-            self.shutdown()
+            self._post_shutdown()
 
             LOG.debug('Error launching VM')
             if self._qemu_full_args:
@@ -368,6 +375,8 @@ def _launch(self):
     def _early_cleanup(self) -> None:
         """
         Perform any cleanup that needs to happen before the VM exits.
+
+        Called additionally by _post_shutdown for comprehensive cleanup.
         """
         # If we keep the console socket open, we may deadlock waiting
         # for QEMU to exit, while QEMU is waiting for the socket to
@@ -388,6 +397,9 @@ def shutdown(self, has_quit=False, hard=False):
         """
         Terminate the VM and clean up
         """
+        if not self._launched:
+            return
+
         self._early_cleanup()
 
         if self.is_running():
-- 
2.21.3

[PULL 07/19] python/machine.py: Add a configurable timeout to shutdown()

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

Three seconds is hardcoded. Use it as a default parameter instead, and use that
value for both waits that may occur in the function.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-7-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c28957ee82..e825f0bdc6 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -393,7 +393,9 @@ def wait(self):
         self._popen.wait()
         self._post_shutdown()
 
-    def shutdown(self, has_quit=False, hard=False):
+    def shutdown(self, has_quit: bool = False,
+                 hard: bool = False,
+                 timeout: Optional[int] = 3) -> None:
         """
         Terminate the VM and clean up
         """
@@ -409,10 +411,10 @@ def shutdown(self, has_quit=False, hard=False):
                 try:
                     if not has_quit:
                         self._qmp.cmd('quit')
-                    self._popen.wait(timeout=3)
+                    self._popen.wait(timeout=timeout)
                 except:
                     self._popen.kill()
-            self._popen.wait()
+            self._popen.wait(timeout=timeout)
 
         self._post_shutdown()
 
-- 
2.21.3

[PULL 08/19] python/machine.py: Make wait() call shutdown()

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

At this point, shutdown(has_quit=True) and wait() do essentially the
same thing; they perform cleanup without actually instructing QEMU to
quit.

Define one in terms of the other.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-8-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index e825f0bdc6..3f0b873f58 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -385,14 +385,6 @@ def _early_cleanup(self) -> None:
             self._console_socket.close()
             self._console_socket = None
 
-    def wait(self):
-        """
-        Wait for the VM to power off
-        """
-        self._early_cleanup()
-        self._popen.wait()
-        self._post_shutdown()
-
     def shutdown(self, has_quit: bool = False,
                  hard: bool = False,
                  timeout: Optional[int] = 3) -> None:
@@ -421,6 +413,15 @@ def shutdown(self, has_quit: bool = False,
     def kill(self):
         self.shutdown(hard=True)
 
+    def wait(self, timeout: Optional[int] = None) -> None:
+        """
+        Wait for the VM to power off and perform post-shutdown cleanup.
+
+        :param timeout: Optional timeout in seconds.
+                        Default None, an infinite wait.
+        """
+        self.shutdown(has_quit=True, timeout=timeout)
+
     def set_qmp_monitor(self, enabled=True):
         """
         Set the QMP monitor.
-- 
2.21.3

[PULL 09/19] tests/acceptance: wait() instead of shutdown() where appropriate

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

When issuing 'reboot' to a VM with the no-reboot option, that VM will
exit. When then issuing a shutdown command, the cleanup may race.

Add calls to vm.wait() which will gracefully mark the VM as having
exited. Subsequent vm.shutdown() calls in generic tearDown code will not
race when called after completion of the call.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-9-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/acceptance/boot_linux_console.py   | 10 ++++++++++
 tests/acceptance/linux_ssh_mips_malta.py |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 3d02519660..5867ef760c 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -191,6 +191,8 @@ def test_mips_malta_cpio(self):
                                                 'Debian')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # Wait for VM to shut down gracefully
+        self.vm.wait()
 
     @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
     def test_mips64el_malta_5KEc_cpio(self):
@@ -231,6 +233,8 @@ def test_mips64el_malta_5KEc_cpio(self):
                                                 '3.19.3.mtoman.20150408')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # Wait for VM to shut down gracefully
+        self.vm.wait()
 
     def do_test_mips_malta32el_nanomips(self, kernel_url, kernel_hash):
         kernel_path_xz = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
@@ -506,6 +510,7 @@ def test_arm_cubieboard_initrd(self):
                                                 'system-control@1c00000')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # NB: Do not issue vm.wait() here, cubieboard's reboot does not exit!
 
     def test_arm_cubieboard_sata(self):
         """
@@ -550,6 +555,7 @@ def test_arm_cubieboard_sata(self):
                                                 'sda')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # NB: Do not issue vm.wait() here, cubieboard's reboot does not exit!
 
     def test_arm_orangepi(self):
         """
@@ -615,6 +621,8 @@ def test_arm_orangepi_initrd(self):
                                                 'system-control@1c00000')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # Wait for VM to shut down gracefully
+        self.vm.wait()
 
     def test_arm_orangepi_sd(self):
         """
@@ -662,6 +670,8 @@ def test_arm_orangepi_sd(self):
             '3 packets transmitted, 3 packets received, 0% packet loss')
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
+        # Wait for VM to shut down gracefully
+        self.vm.wait()
 
     @skipUnless(os.getenv('AVOCADO_ALLOW_LARGE_STORAGE'), 'storage limited')
     @skipUnless(P7ZIP_AVAILABLE, '7z not installed')
diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
index 90d7f2f167..25c5c5f741 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -212,6 +212,8 @@ def check_mips_malta(self, uname_m, endianess):
 
         self.run_common_commands(wordsize)
         self.shutdown_via_ssh()
+        # Wait for VM to shut down gracefully
+        self.vm.wait()
 
     def test_mips_malta32eb_kernel3_2_0(self):
         """
-- 
2.21.3

[PULL 10/19] tests/acceptance: Don't test reboot on cubieboard

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

cubieboard does not have a functioning reboot, it halts and QEMU does
not exit.

vm.shutdown() is modified in a forthcoming patch that makes it less tolerant
of race conditions on shutdown; tests should consciously decide to WAIT
or to SHUTDOWN qemu.

So long as this test is attempting to reboot, the correct choice would
be to WAIT for the VM to exit. However, since that's broken, we should
SHUTDOWN instead.

SHUTDOWN is indeed what already happens when the test performs teardown,
however, if anyone fixes cubieboard reboot in the future, this test will
develop a new race condition that might be hard to debug.

Therefore: remove the reboot test and make it obvious that the VM is
still running when the test concludes, where the test teardown will do
the right thing.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-10-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/acceptance/boot_linux_console.py | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 5867ef760c..8b8b828bc5 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -508,9 +508,7 @@ def test_arm_cubieboard_initrd(self):
                                                 'Allwinner sun4i/sun5i')
         exec_command_and_wait_for_pattern(self, 'cat /proc/iomem',
                                                 'system-control@1c00000')
-        exec_command_and_wait_for_pattern(self, 'reboot',
-                                                'reboot: Restarting system')
-        # NB: Do not issue vm.wait() here, cubieboard's reboot does not exit!
+        # cubieboard's reboot is not functioning; omit reboot test.
 
     def test_arm_cubieboard_sata(self):
         """
@@ -553,9 +551,7 @@ def test_arm_cubieboard_sata(self):
                                                 'Allwinner sun4i/sun5i')
         exec_command_and_wait_for_pattern(self, 'cat /proc/partitions',
                                                 'sda')
-        exec_command_and_wait_for_pattern(self, 'reboot',
-                                                'reboot: Restarting system')
-        # NB: Do not issue vm.wait() here, cubieboard's reboot does not exit!
+        # cubieboard's reboot is not functioning; omit reboot test.
 
     def test_arm_orangepi(self):
         """
-- 
2.21.3

[PULL 11/19] python/machine.py: split shutdown into hard and soft flavors

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

This is done primarily to avoid the 'bare except' pattern, which
suppresses all exceptions during shutdown and can obscure errors.

Replace this with a pattern that isolates the different kind of shutdown
paradigms (_hard_shutdown and _soft_shutdown), and a new fallback shutdown
handler (_do_shutdown) that gracefully attempts one before the other.

This split now also ensures that no matter what happens,
_post_shutdown() is always invoked.

shutdown() changes in behavior such that if it attempts to do a graceful
shutdown and is unable to, it will now always raise an exception to
indicate this. This can be avoided by the test writer in three ways:

1. If the VM is expected to have already exited or is in the process of
exiting, wait() can be used instead of shutdown() to clean up resources
instead. This helps avoid race conditions in shutdown.

2. If a test writer is expecting graceful shutdown to fail, shutdown
should be called in a try...except block.

3. If the test writer has no interest in performing a graceful shutdown
at all, kill() can be used instead.

Handling shutdown in this way makes it much more explicit which type of
shutdown we want and allows the library to report problems with this
process.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-11-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 98 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 15 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 3f0b873f58..a955e3f221 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -49,6 +49,12 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
     """
 
 
+class AbnormalShutdown(QEMUMachineError):
+    """
+    Exception raised when a graceful shutdown was requested, but not performed.
+    """
+
+
 class MonitorResponseError(qmp.QMPError):
     """
     Represents erroneous QMP monitor reply
@@ -376,6 +382,7 @@ def _early_cleanup(self) -> None:
         """
         Perform any cleanup that needs to happen before the VM exits.
 
+        May be invoked by both soft and hard shutdown in failover scenarios.
         Called additionally by _post_shutdown for comprehensive cleanup.
         """
         # If we keep the console socket open, we may deadlock waiting
@@ -385,32 +392,93 @@ def _early_cleanup(self) -> None:
             self._console_socket.close()
             self._console_socket = None
 
+    def _hard_shutdown(self) -> None:
+        """
+        Perform early cleanup, kill the VM, and wait for it to terminate.
+
+        :raise subprocess.Timeout: When timeout is exceeds 60 seconds
+            waiting for the QEMU process to terminate.
+        """
+        self._early_cleanup()
+        self._popen.kill()
+        self._popen.wait(timeout=60)
+
+    def _soft_shutdown(self, has_quit: bool = False,
+                       timeout: Optional[int] = 3) -> None:
+        """
+        Perform early cleanup, attempt to gracefully shut down the VM, and wait
+        for it to terminate.
+
+        :param has_quit: When True, don't attempt to issue 'quit' QMP command
+        :param timeout: Optional timeout in seconds for graceful shutdown.
+                        Default 3 seconds, A value of None is an infinite wait.
+
+        :raise ConnectionReset: On QMP communication errors
+        :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
+            the QEMU process to terminate.
+        """
+        self._early_cleanup()
+
+        if self._qmp is not None:
+            if not has_quit:
+                # Might raise ConnectionReset
+                self._qmp.cmd('quit')
+
+        # May raise subprocess.TimeoutExpired
+        self._popen.wait(timeout=timeout)
+
+    def _do_shutdown(self, has_quit: bool = False,
+                     timeout: Optional[int] = 3) -> None:
+        """
+        Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
+
+        :param has_quit: When True, don't attempt to issue 'quit' QMP command
+        :param timeout: Optional timeout in seconds for graceful shutdown.
+                        Default 3 seconds, A value of None is an infinite wait.
+
+        :raise AbnormalShutdown: When the VM could not be shut down gracefully.
+            The inner exception will likely be ConnectionReset or
+            subprocess.TimeoutExpired. In rare cases, non-graceful termination
+            may result in its own exceptions, likely subprocess.TimeoutExpired.
+        """
+        try:
+            self._soft_shutdown(has_quit, timeout)
+        except Exception as exc:
+            self._hard_shutdown()
+            raise AbnormalShutdown("Could not perform graceful shutdown") \
+                from exc
+
     def shutdown(self, has_quit: bool = False,
                  hard: bool = False,
                  timeout: Optional[int] = 3) -> None:
         """
-        Terminate the VM and clean up
+        Terminate the VM (gracefully if possible) and perform cleanup.
+        Cleanup will always be performed.
+
+        If the VM has not yet been launched, or shutdown(), wait(), or kill()
+        have already been called, this method does nothing.
+
+        :param has_quit: When true, do not attempt to issue 'quit' QMP command.
+        :param hard: When true, do not attempt graceful shutdown, and
+                     suppress the SIGKILL warning log message.
+        :param timeout: Optional timeout in seconds for graceful shutdown.
+                        Default 3 seconds, A value of None is an infinite wait.
         """
         if not self._launched:
             return
 
-        self._early_cleanup()
-
-        if self.is_running():
+        try:
             if hard:
-                self._popen.kill()
-            elif self._qmp:
-                try:
-                    if not has_quit:
-                        self._qmp.cmd('quit')
-                    self._popen.wait(timeout=timeout)
-                except:
-                    self._popen.kill()
-            self._popen.wait(timeout=timeout)
-
-        self._post_shutdown()
+                self._hard_shutdown()
+            else:
+                self._do_shutdown(has_quit, timeout=timeout)
+        finally:
+            self._post_shutdown()
 
     def kill(self):
+        """
+        Terminate the VM forcefully, wait for it to exit, and perform cleanup.
+        """
         self.shutdown(hard=True)
 
     def wait(self, timeout: Optional[int] = None) -> None:
-- 
2.21.3

[PULL 12/19] python/machine.py: re-add sigkill warning suppression

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

If the user kills QEMU on purpose, we don't need to warn
them about that having happened: they know already.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-12-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index a955e3f221..736a3c906f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -22,6 +22,7 @@
 import os
 import subprocess
 import shutil
+import signal
 import socket
 import tempfile
 from typing import Optional, Type
@@ -133,6 +134,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         self._console_address = None
         self._console_socket = None
         self._remove_files = []
+        self._user_killed = False
         self._console_log_path = console_log
         if self._console_log_path:
             # In order to log the console, buffering needs to be enabled.
@@ -327,7 +329,8 @@ def _post_shutdown(self):
             self._remove_if_exists(self._remove_files.pop())
 
         exitcode = self.exitcode()
-        if exitcode is not None and exitcode < 0:
+        if (exitcode is not None and exitcode < 0
+                and not (self._user_killed and exitcode == -signal.SIGKILL)):
             msg = 'qemu received signal %i; command: "%s"'
             if self._qemu_full_args:
                 command = ' '.join(self._qemu_full_args)
@@ -335,6 +338,7 @@ def _post_shutdown(self):
                 command = ''
             LOG.warning(msg, -int(exitcode), command)
 
+        self._user_killed = False
         self._launched = False
 
     def launch(self):
@@ -469,6 +473,7 @@ def shutdown(self, has_quit: bool = False,
 
         try:
             if hard:
+                self._user_killed = True
                 self._hard_shutdown()
             else:
                 self._do_shutdown(has_quit, timeout=timeout)
-- 
2.21.3

[PULL 13/19] python/machine.py: change default wait timeout to 3 seconds

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

Machine.wait() does not appear to be used except in the acceptance tests,
and an infinite timeout by default in a test suite is not the most helpful.

Change it to 3 seconds, like the default shutdown timeout.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
Message-Id: <20200710050649.32434-13-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 736a3c906f..69055189bd 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -486,12 +486,12 @@ def kill(self):
         """
         self.shutdown(hard=True)
 
-    def wait(self, timeout: Optional[int] = None) -> None:
+    def wait(self, timeout: Optional[int] = 3) -> None:
         """
         Wait for the VM to power off and perform post-shutdown cleanup.
 
         :param timeout: Optional timeout in seconds.
-                        Default None, an infinite wait.
+                        Default 3 seconds, A value of None is an infinite wait.
         """
         self.shutdown(has_quit=True, timeout=timeout)
 
-- 
2.21.3

[PULL 14/19] python/qmp.py: Define common types

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

Define some common types that we'll need to annotate a lot of other
functions going forward.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-2-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/qmp.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index e64b6b5faa..8388c7b603 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -12,13 +12,31 @@
 import socket
 import logging
 from typing import (
+    Any,
+    Dict,
     Optional,
     TextIO,
     Type,
+    Tuple,
+    Union,
 )
 from types import TracebackType
 
 
+# QMPMessage is a QMP Message of any kind.
+# e.g. {'yee': 'haw'}
+#
+# QMPReturnValue is the inner value of return values only.
+# {'return': {}} is the QMPMessage,
+# {} is the QMPReturnValue.
+QMPMessage = Dict[str, Any]
+QMPReturnValue = Dict[str, Any]
+
+InternetAddrT = Tuple[str, str]
+UnixAddrT = str
+SocketAddrT = Union[InternetAddrT, UnixAddrT]
+
+
 class QMPError(Exception):
     """
     QMP base exception
-- 
2.21.3

[PULL 15/19] iotests.py: use qemu.qmp type aliases

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

iotests.py should use the type definitions from qmp.py instead of its
own.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-3-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qemu-iotests/iotests.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8b760405ee..3590ed78a0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,13 +35,10 @@
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
+from qemu.qmp import QMPMessage
 
 assert sys.version_info >= (3, 6)
 
-# Type Aliases
-QMPResponse = Dict[str, Any]
-
-
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
 logger.addHandler(logging.NullHandler())
@@ -561,7 +558,7 @@ def add_incoming(self, addr):
         self._args.append(addr)
         return self
 
-    def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
+    def hmp(self, command_line: str, use_log: bool = False) -> QMPMessage:
         cmd = 'human-monitor-command'
         kwargs = {'command-line': command_line}
         if use_log:
@@ -582,7 +579,7 @@ def resume_drive(self, drive: str) -> None:
         self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
 
     def hmp_qemu_io(self, drive: str, cmd: str,
-                    use_log: bool = False) -> QMPResponse:
+                    use_log: bool = False) -> QMPMessage:
         """Write to a given drive using an HMP command"""
         return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
 
-- 
2.21.3

[PULL 16/19] python/qmp.py: re-absorb MonitorResponseError

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

When I initially split this out, I considered this more of a machine
error than a QMP protocol error, but I think that's misguided.

Move this back to qmp.py and name it QMPResponseError. Convert
qmp.command() to use this exception type.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-4-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/machine.py        | 15 +--------------
 python/qemu/qmp.py            | 17 +++++++++++++++--
 scripts/render_block_graph.py |  7 +++++--
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 69055189bd..80c4d4a8b6 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -56,19 +56,6 @@ class AbnormalShutdown(QEMUMachineError):
     """
 
 
-class MonitorResponseError(qmp.QMPError):
-    """
-    Represents erroneous QMP monitor reply
-    """
-    def __init__(self, reply):
-        try:
-            desc = reply["error"]["desc"]
-        except KeyError:
-            desc = reply
-        super().__init__(desc)
-        self.reply = reply
-
-
 class QEMUMachine:
     """
     A QEMU VM
@@ -533,7 +520,7 @@ def command(self, cmd, conv_keys=True, **args):
         if reply is None:
             raise qmp.QMPError("Monitor is closed")
         if "error" in reply:
-            raise MonitorResponseError(reply)
+            raise qmp.QMPResponseError(reply)
         return reply["return"]
 
     def get_qmp_event(self, wait=False):
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 8388c7b603..aa8a666b8a 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -61,6 +61,19 @@ class QMPTimeoutError(QMPError):
     """
 
 
+class QMPResponseError(QMPError):
+    """
+    Represents erroneous QMP monitor reply
+    """
+    def __init__(self, reply: QMPMessage):
+        try:
+            desc = reply['error']['desc']
+        except KeyError:
+            desc = reply
+        super().__init__(desc)
+        self.reply = reply
+
+
 class QEMUMonitorProtocol:
     """
     Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -251,8 +264,8 @@ def command(self, cmd, **kwds):
         Build and send a QMP command to the monitor, report errors if any
         """
         ret = self.cmd(cmd, kwds)
-        if "error" in ret:
-            raise Exception(ret['error']['desc'])
+        if 'error' in ret:
+            raise QMPResponseError(ret)
         return ret['return']
 
     def pull_event(self, wait=False):
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index 409b4321f2..da6acf050d 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -25,7 +25,10 @@
 from graphviz import Digraph
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.machine import MonitorResponseError
+from qemu.qmp import (
+    QEMUMonitorProtocol,
+    QMPResponseError,
+)
 
 
 def perm(arr):
@@ -102,7 +105,7 @@ def command(self, cmd):
         reply = json.loads(subprocess.check_output(ar))
 
         if 'error' in reply:
-            raise MonitorResponseError(reply)
+            raise QMPResponseError(reply)
 
         return reply['return']
 
-- 
2.21.3

[PULL 17/19] python/qmp.py: Do not return None from cmd_obj

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

This makes typing the qmp library difficult, as it necessitates wrapping
Optional[] around the type for every return type up the stack. At some
point, it becomes difficult to discern or remember why it's None instead
of the expected object.

Use the python exception system to tell us exactly why we didn't get an
object. Remove this special-cased return.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-5-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/qmp.py | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index aa8a666b8a..ef3c919b76 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -225,22 +225,18 @@ def accept(self, timeout=15.0):
         self.__sockfile = self.__sock.makefile(mode='r')
         return self.__negotiate_capabilities()
 
-    def cmd_obj(self, qmp_cmd):
+    def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
         """
         Send a QMP command to the QMP Monitor.
 
         @param qmp_cmd: QMP command to be sent as a Python dict
-        @return QMP response as a Python dict or None if the connection has
-                been closed
+        @return QMP response as a Python dict
         """
         self.logger.debug(">>> %s", qmp_cmd)
-        try:
-            self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
-        except OSError as err:
-            if err.errno == errno.EPIPE:
-                return None
-            raise err
+        self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
         resp = self.__json_read()
+        if resp is None:
+            raise QMPConnectError("Unexpected empty reply from server")
         self.logger.debug("<<< %s", resp)
         return resp
 
-- 
2.21.3

[PULL 18/19] python/qmp.py: add casts to JSON deserialization

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

mypy and python type hints are not powerful enough to properly describe
JSON messages in Python 3.6. The best we can do, generally, is describe
them as Dict[str, Any].

Add casts to coerce this type for static analysis; but do NOT enforce
this type at runtime in any way.

Note: Python 3.8 adds a TypedDict construct which allows for the
description of more arbitrary Dictionary shapes. There is a third-party
module, "Pydantic", which is compatible with 3.6 that can be used
instead of the JSON library that parses JSON messages to fully-typed
Python objects, and may be preferable in some cases.

(That is well beyond the scope of this commit or series.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-6-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/qmp.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index ef3c919b76..1ae36050a4 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -13,6 +13,7 @@
 import logging
 from typing import (
     Any,
+    cast,
     Dict,
     Optional,
     TextIO,
@@ -130,7 +131,10 @@ def __json_read(self, only_event=False):
             data = self.__sockfile.readline()
             if not data:
                 return None
-            resp = json.loads(data)
+            # By definition, any JSON received from QMP is a QMPMessage,
+            # and we are asserting only at static analysis time that it
+            # has a particular shape.
+            resp: QMPMessage = json.loads(data)
             if 'event' in resp:
                 self.logger.debug("<<< %s", resp)
                 self.__events.append(resp)
@@ -262,7 +266,7 @@ def command(self, cmd, **kwds):
         ret = self.cmd(cmd, kwds)
         if 'error' in ret:
             raise QMPResponseError(ret)
-        return ret['return']
+        return cast(QMPReturnValue, ret['return'])
 
     def pull_event(self, wait=False):
         """
-- 
2.21.3

[PULL 19/19] python/qmp.py: add QMPProtocolError

[Philippe Mathieu-Daudé]

From: John Snow <jsnow@redhat.com>

In the case that we receive a reply but are unable to understand it,
use this exception name to indicate that case.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200710052220.3306-7-jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/qmp.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 1ae36050a4..7935dababb 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -62,6 +62,12 @@ class QMPTimeoutError(QMPError):
     """
 
 
+class QMPProtocolError(QMPError):
+    """
+    QMP protocol error; unexpected response
+    """
+
+
 class QMPResponseError(QMPError):
     """
     Represents erroneous QMP monitor reply
@@ -266,6 +272,10 @@ def command(self, cmd, **kwds):
         ret = self.cmd(cmd, kwds)
         if 'error' in ret:
             raise QMPResponseError(ret)
+        if 'return' not in ret:
+            raise QMPProtocolError(
+                "'return' key not found in QMP response '{}'".format(str(ret))
+            )
         return cast(QMPReturnValue, ret['return'])
 
     def pull_event(self, wait=False):
-- 
2.21.3

Re: [PULL 00/19] Python patches for 5.1

[Peter Maydell]

On Tue, 14 Jul 2020 at 23:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> The following changes since commit 1a53dfee92284d3016a579ef31d53367e84d9dd8:
>
>   Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2020-07-13' into staging (2020-07-14 13:52:10 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/python-next-20200714
>
> for you to fetch changes up to 84dcdf0887cdaaba7300442482c99e5064865a2d:
>
>   python/qmp.py: add QMPProtocolError (2020-07-14 22:22:22 +0200)
>
> ----------------------------------------------------------------
> Python patches for 5.1
>
> - Reduce race conditions on QEMUMachine::shutdown()
>
>  1. Remove the "bare except" pattern in the existing shutdown code,
>     which can mask problems and make debugging difficult.
>  2. Ensure that post-shutdown cleanup is always performed, even when
>     graceful termination fails.
>  3. Unify cleanup paths such that no matter how the VM is terminated,
>     the same functions and steps are always taken to reset the object
>     state.
>  4. Rewrite shutdown() such that any error encountered when attempting
>     a graceful shutdown will be raised as an AbnormalShutdown exception.
>     The pythonic idiom is to allow the caller to decide if this is a
>     problem or not.
>
> - Modify part of the python/qemu library to comply with:
>
>   . mypy --strict
>   . pylint
>   . flake8
>
> - Script for the TCG Continuous Benchmarking project that uses
>   callgrind to dissect QEMU execution into three main phases:
>
>   . code generation
>   . JIT execution
>   . helpers execution
>
> CI jobs results:
> . https://cirrus-ci.com/build/5421349961203712
> . https://gitlab.com/philmd/qemu/-/pipelines/166556001
> . https://travis-ci.org/github/philmd/qemu/builds/708102347


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM