* [PULL 01/19] scripts/performance: Add dissect.py script
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 02/19] python/machine.py: consolidate _post_shutdown() Philippe Mathieu-Daudé
` (18 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, John Snow, Ahmed Karaman, Aleksandar Markovic,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 02/19] python/machine.py: consolidate _post_shutdown()
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 01/19] scripts/performance: Add dissect.py script Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 03/19] python/machine.py: Close QMP socket in cleanup Philippe Mathieu-Daudé
` (17 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 03/19] python/machine.py: Close QMP socket in cleanup
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 01/19] scripts/performance: Add dissect.py script Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 02/19] python/machine.py: consolidate _post_shutdown() Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 04/19] python/machine.py: Add _early_cleanup hook Philippe Mathieu-Daudé
` (16 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 04/19] python/machine.py: Add _early_cleanup hook
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-07-14 22:21 ` [PULL 03/19] python/machine.py: Close QMP socket in cleanup Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 05/19] python/machine.py: Perform early cleanup for wait() calls, too Philippe Mathieu-Daudé
` (15 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 05/19] python/machine.py: Perform early cleanup for wait() calls, too
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-07-14 22:21 ` [PULL 04/19] python/machine.py: Add _early_cleanup hook Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 06/19] python/machine.py: Prohibit multiple shutdown() calls Philippe Mathieu-Daudé
` (14 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 06/19] python/machine.py: Prohibit multiple shutdown() calls
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-07-14 22:21 ` [PULL 05/19] python/machine.py: Perform early cleanup for wait() calls, too Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 07/19] python/machine.py: Add a configurable timeout to shutdown() Philippe Mathieu-Daudé
` (13 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 07/19] python/machine.py: Add a configurable timeout to shutdown()
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2020-07-14 22:21 ` [PULL 06/19] python/machine.py: Prohibit multiple shutdown() calls Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 08/19] python/machine.py: Make wait() call shutdown() Philippe Mathieu-Daudé
` (12 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 08/19] python/machine.py: Make wait() call shutdown()
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2020-07-14 22:21 ` [PULL 07/19] python/machine.py: Add a configurable timeout to shutdown() Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 09/19] tests/acceptance: wait() instead of shutdown() where appropriate Philippe Mathieu-Daudé
` (11 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 09/19] tests/acceptance: wait() instead of shutdown() where appropriate
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2020-07-14 22:21 ` [PULL 08/19] python/machine.py: Make wait() call shutdown() Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 10/19] tests/acceptance: Don't test reboot on cubieboard Philippe Mathieu-Daudé
` (10 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Eduardo Habkost, John Snow,
Wainer dos Santos Moschetta, Ahmed Karaman, Aleksandar Markovic,
Cleber Rosa, Philippe Mathieu-Daudé,
Aurelien Jarno
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 10/19] tests/acceptance: Don't test reboot on cubieboard
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (8 preceding siblings ...)
2020-07-14 22:21 ` [PULL 09/19] tests/acceptance: wait() instead of shutdown() where appropriate Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 11/19] python/machine.py: split shutdown into hard and soft flavors Philippe Mathieu-Daudé
` (9 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, John Snow, Wainer dos Santos Moschetta,
Ahmed Karaman, Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 11/19] python/machine.py: split shutdown into hard and soft flavors
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (9 preceding siblings ...)
2020-07-14 22:21 ` [PULL 10/19] tests/acceptance: Don't test reboot on cubieboard Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 12/19] python/machine.py: re-add sigkill warning suppression Philippe Mathieu-Daudé
` (8 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 12/19] python/machine.py: re-add sigkill warning suppression
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (10 preceding siblings ...)
2020-07-14 22:21 ` [PULL 11/19] python/machine.py: split shutdown into hard and soft flavors Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 13/19] python/machine.py: change default wait timeout to 3 seconds Philippe Mathieu-Daudé
` (7 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 13/19] python/machine.py: change default wait timeout to 3 seconds
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (11 preceding siblings ...)
2020-07-14 22:21 ` [PULL 12/19] python/machine.py: re-add sigkill warning suppression Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 14/19] python/qmp.py: Define common types Philippe Mathieu-Daudé
` (6 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Ahmed Karaman, Philippe Mathieu-Daudé,
John Snow, Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 14/19] python/qmp.py: Define common types
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (12 preceding siblings ...)
2020-07-14 22:21 ` [PULL 13/19] python/machine.py: change default wait timeout to 3 seconds Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 15/19] iotests.py: use qemu.qmp type aliases Philippe Mathieu-Daudé
` (5 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, John Snow, Ahmed Karaman,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 15/19] iotests.py: use qemu.qmp type aliases
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (13 preceding siblings ...)
2020-07-14 22:21 ` [PULL 14/19] python/qmp.py: Define common types Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 16/19] python/qmp.py: re-absorb MonitorResponseError Philippe Mathieu-Daudé
` (4 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, open list:Block layer core,
John Snow, Max Reitz, Ahmed Karaman, Cleber Rosa,
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 16/19] python/qmp.py: re-absorb MonitorResponseError
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (14 preceding siblings ...)
2020-07-14 22:21 ` [PULL 15/19] iotests.py: use qemu.qmp type aliases Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 17/19] python/qmp.py: Do not return None from cmd_obj Philippe Mathieu-Daudé
` (3 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, John Snow, Ahmed Karaman,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 17/19] python/qmp.py: Do not return None from cmd_obj
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (15 preceding siblings ...)
2020-07-14 22:21 ` [PULL 16/19] python/qmp.py: re-absorb MonitorResponseError Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 18/19] python/qmp.py: add casts to JSON deserialization Philippe Mathieu-Daudé
` (2 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, John Snow, Ahmed Karaman,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 18/19] python/qmp.py: add casts to JSON deserialization
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (16 preceding siblings ...)
2020-07-14 22:21 ` [PULL 17/19] python/qmp.py: Do not return None from cmd_obj Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-14 22:21 ` [PULL 19/19] python/qmp.py: add QMPProtocolError Philippe Mathieu-Daudé
2020-07-15 12:53 ` [PULL 00/19] Python patches for 5.1 Peter Maydell
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, John Snow, Ahmed Karaman,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PULL 19/19] python/qmp.py: add QMPProtocolError
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (17 preceding siblings ...)
2020-07-14 22:21 ` [PULL 18/19] python/qmp.py: add casts to JSON deserialization Philippe Mathieu-Daudé
@ 2020-07-14 22:21 ` Philippe Mathieu-Daudé
2020-07-15 12:53 ` [PULL 00/19] Python patches for 5.1 Peter Maydell
19 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, John Snow, Ahmed Karaman,
Cleber Rosa, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PULL 00/19] Python patches for 5.1
2020-07-14 22:21 [PULL 00/19] Python patches for 5.1 Philippe Mathieu-Daudé
` (18 preceding siblings ...)
2020-07-14 22:21 ` [PULL 19/19] python/qmp.py: add QMPProtocolError Philippe Mathieu-Daudé
@ 2020-07-15 12:53 ` Peter Maydell
19 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2020-07-15 12:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Ahmed Karaman, Cleber Rosa, John Snow, QEMU Developers, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 21+ messages in thread