* [PATCH v3 01/16] python/qmp.py: Define common types
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 02/16] iotests.py: use qemu.qmp type aliases John Snow
` (16 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
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>
---
python/qemu/qmp.py | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index e64b6b5faa7..8388c7b6030 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] 34+ messages in thread* [PATCH v3 02/16] iotests.py: use qemu.qmp type aliases
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
2020-06-04 20:22 ` [PATCH v3 01/16] python/qmp.py: Define common types John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 03/16] python/qmp.py: re-absorb MonitorResponseError John Snow
` (15 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
iotests.py should use the type definitions from qmp.py instead of its
own.
Signed-off-by: John Snow <jsnow@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 f20d90f9698..7c1773bba37 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())
@@ -554,7 +551,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:
@@ -575,7 +572,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] 34+ messages in thread* [PATCH v3 03/16] python/qmp.py: re-absorb MonitorResponseError
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
2020-06-04 20:22 ` [PATCH v3 01/16] python/qmp.py: Define common types John Snow
2020-06-04 20:22 ` [PATCH v3 02/16] iotests.py: use qemu.qmp type aliases John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 04/16] python/qmp.py: Do not return None from cmd_obj John Snow
` (14 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
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>
---
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 99bcb499878..8ea027566e4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -49,19 +49,6 @@ class QEMUMachineAddDeviceError(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
@@ -469,7 +456,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 8388c7b6030..aa8a666b8ab 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 409b4321f2e..da6acf050d1 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] 34+ messages in thread* [PATCH v3 04/16] python/qmp.py: Do not return None from cmd_obj
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (2 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 03/16] python/qmp.py: re-absorb MonitorResponseError John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 05/16] python/qmp.py: add casts to JSON deserialization John Snow
` (13 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
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>
---
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 aa8a666b8ab..ef3c919b76c 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] 34+ messages in thread* [PATCH v3 05/16] python/qmp.py: add casts to JSON deserialization
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (3 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 04/16] python/qmp.py: Do not return None from cmd_obj John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 06/16] python/qmp.py: add QMPProtocolError John Snow
` (12 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
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>
---
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 ef3c919b76c..1ae36050a42 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] 34+ messages in thread* [PATCH v3 06/16] python/qmp.py: add QMPProtocolError
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (4 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 05/16] python/qmp.py: add casts to JSON deserialization John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 07/16] python/machine.py: Fix monitor address typing John Snow
` (11 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
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>
---
python/qemu/qmp.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 1ae36050a42..7935dababbf 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] 34+ messages in thread* [PATCH v3 07/16] python/machine.py: Fix monitor address typing
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (5 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 06/16] python/qmp.py: add QMPProtocolError John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 08/16] python/machine.py: reorder __init__ John Snow
` (10 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
Prior to this, it's difficult for mypy to intuit what the concrete type
of the monitor address is; it has difficulty inferring the type across
two variables.
Create _monitor_address as a property that always returns a valid
address to simply static type analysis.
To preserve our ability to clean up, use a simple boolean to indicate
whether or not we should try to clean up the sock file after execution.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 45 +++++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 8ea027566e4..34713ba9b15 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -25,10 +25,14 @@
import signal
import socket
import tempfile
-from typing import Optional, Type
+from typing import (
+ Optional,
+ Type,
+)
from types import TracebackType
from . import qmp
+from .qmp import SocketAddrT
LOG = logging.getLogger(__name__)
@@ -62,7 +66,8 @@ class QEMUMachine:
"""
def __init__(self, binary, args=None, wrapper=None, name=None,
- test_dir="/var/tmp", monitor_address=None,
+ test_dir="/var/tmp",
+ monitor_address: Optional[SocketAddrT] = None,
socket_scm_helper=None, sock_dir=None):
'''
Initialize a QEMUMachine
@@ -85,8 +90,14 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
if sock_dir is None:
sock_dir = test_dir
self._name = name
- self._monitor_address = monitor_address
- self._vm_monitor = None
+ if monitor_address is not None:
+ self._monitor_address = monitor_address
+ self._remove_monitor_sockfile = False
+ else:
+ self._monitor_address = os.path.join(
+ sock_dir, f"{name}-monitor.sock"
+ )
+ self._remove_monitor_sockfile = True
self._qemu_log_path = None
self._qemu_log_file = None
self._popen = None
@@ -225,15 +236,17 @@ def _load_io_log(self):
def _base_args(self):
args = ['-display', 'none', '-vga', 'none']
+
if self._qmp_set:
if isinstance(self._monitor_address, tuple):
- moncdev = "socket,id=mon,host=%s,port=%s" % (
- self._monitor_address[0],
- self._monitor_address[1])
+ moncdev = "socket,id=mon,host={},port={}".format(
+ *self._monitor_address
+ )
else:
- moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
+ moncdev = f"socket,id=mon,path={self._monitor_address}"
args.extend(['-chardev', moncdev, '-mon',
'chardev=mon,mode=control'])
+
if self._machine is not None:
args.extend(['-machine', self._machine])
for _ in range(self._console_index):
@@ -258,14 +271,14 @@ def _pre_launch(self):
self._qemu_log_file = open(self._qemu_log_path, 'wb')
if self._qmp_set:
- if self._monitor_address is not None:
- self._vm_monitor = self._monitor_address
- else:
- self._vm_monitor = os.path.join(self._sock_dir,
- self._name + "-monitor.sock")
- self._remove_files.append(self._vm_monitor)
- self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True,
- nickname=self._name)
+ if self._remove_monitor_sockfile:
+ assert isinstance(self._monitor_address, str)
+ self._remove_files.append(self._monitor_address)
+ self._qmp = qmp.QEMUMonitorProtocol(
+ self._monitor_address,
+ server=True,
+ nickname=self._name
+ )
def _post_launch(self):
if self._qmp:
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 08/16] python/machine.py: reorder __init__
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (6 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 07/16] python/machine.py: Fix monitor address typing John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 09/16] python/machine.py: Don't modify state in _base_args() John Snow
` (9 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
Put the init arg handling all at the top, and mostly in order (deviating
when one is dependent on another), and put what is effectively runtime
state declaration at the bottom.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
python/qemu/machine.py | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 34713ba9b15..aae78e3bc87 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -81,38 +81,43 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
@param socket_scm_helper: helper program, required for send_fd_scm()
@note: Qemu process is not started until launch() is used.
'''
+ # Direct user configuration
+
+ self._binary = binary
+
if args is None:
args = []
+ # Copy mutable input: we will be modifying our copy
+ self._args = list(args)
+
if wrapper is None:
wrapper = []
- if name is None:
- name = "qemu-%d" % os.getpid()
- if sock_dir is None:
- sock_dir = test_dir
- self._name = name
+ self._wrapper = wrapper
+
+ self._name = name or "qemu-%d" % os.getpid()
+ self._test_dir = test_dir
+ self._sock_dir = sock_dir or self._test_dir
+ self._socket_scm_helper = socket_scm_helper
+
if monitor_address is not None:
self._monitor_address = monitor_address
self._remove_monitor_sockfile = False
else:
self._monitor_address = os.path.join(
- sock_dir, f"{name}-monitor.sock"
+ self._sock_dir, f"{self._name}-monitor.sock"
)
self._remove_monitor_sockfile = True
+
+ # Runstate
self._qemu_log_path = None
self._qemu_log_file = None
self._popen = None
- self._binary = binary
- self._args = list(args) # Force copy args in case we modify them
- self._wrapper = wrapper
self._events = []
self._iolog = None
- self._socket_scm_helper = socket_scm_helper
self._qmp_set = True # Enable QMP monitor by default.
self._qmp = None
self._qemu_full_args = None
- self._test_dir = test_dir
self._temp_dir = None
- self._sock_dir = sock_dir
self._launched = False
self._machine = None
self._console_index = 0
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 09/16] python/machine.py: Don't modify state in _base_args()
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (7 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 08/16] python/machine.py: reorder __init__ John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 10/16] python/machine.py: Handle None events in events_wait John Snow
` (8 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
Don't append to the _remove_files list during _base_args; instead do so
during _launch. Rework _base_args as a @property to help facilitate
this impression.
This has the additional benefit of making the type of _console_address
easier to analyze statically.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 16 ++++++++++------
python/qemu/qtest.py | 11 ++++++++---
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index aae78e3bc87..92528a44097 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -26,6 +26,7 @@
import socket
import tempfile
from typing import (
+ List,
Optional,
Type,
)
@@ -123,7 +124,9 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
self._console_index = 0
self._console_set = False
self._console_device_type = None
- self._console_address = None
+ self._console_address = os.path.join(
+ self._sock_dir, f"{self._name}-console.sock"
+ )
self._console_socket = None
self._remove_files = []
self._killed = False
@@ -239,7 +242,8 @@ def _load_io_log(self):
with open(self._qemu_log_path, "r") as iolog:
self._iolog = iolog.read()
- def _base_args(self):
+ @property
+ def _base_args(self) -> List[str]:
args = ['-display', 'none', '-vga', 'none']
if self._qmp_set:
@@ -257,9 +261,6 @@ def _base_args(self):
for _ in range(self._console_index):
args.extend(['-serial', 'null'])
if self._console_set:
- self._console_address = os.path.join(self._sock_dir,
- self._name + "-console.sock")
- self._remove_files.append(self._console_address)
chardev = ('socket,id=console,path=%s,server,nowait' %
self._console_address)
args.extend(['-chardev', chardev])
@@ -275,6 +276,9 @@ def _pre_launch(self):
self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
self._qemu_log_file = open(self._qemu_log_path, 'wb')
+ if self._console_set:
+ self._remove_files.append(self._console_address)
+
if self._qmp_set:
if self._remove_monitor_sockfile:
assert isinstance(self._monitor_address, str)
@@ -349,7 +353,7 @@ def _launch(self):
devnull = open(os.path.devnull, 'rb')
self._pre_launch()
self._qemu_full_args = (self._wrapper + [self._binary] +
- self._base_args() + self._args)
+ self._base_args + self._args)
LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
self._popen = subprocess.Popen(self._qemu_full_args,
stdin=devnull,
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 888c8bd2f60..05c63a1d583 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -19,7 +19,11 @@
import socket
import os
-from typing import Optional, TextIO
+from typing import (
+ List,
+ Optional,
+ TextIO,
+)
from .machine import QEMUMachine
@@ -111,8 +115,9 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
self._qtest = None
self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
- def _base_args(self):
- args = super()._base_args()
+ @property
+ def _base_args(self) -> List[str]:
+ args = super()._base_args
args.extend(['-qtest', 'unix:path=' + self._qtest_path,
'-accel', 'qtest'])
return args
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 10/16] python/machine.py: Handle None events in events_wait
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (8 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 09/16] python/machine.py: Don't modify state in _base_args() John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 11/16] python/machine.py: use qmp.command John Snow
` (7 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
If the timeout is 0, we can get None back. Handle this explicitly.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 92528a44097..4afd67a9351 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -33,7 +33,7 @@
from types import TracebackType
from . import qmp
-from .qmp import SocketAddrT
+from .qmp import SocketAddrT, QMPMessage
LOG = logging.getLogger(__name__)
@@ -544,13 +544,20 @@ def event_wait(self, name, timeout=60.0, match=None):
def events_wait(self, events, timeout=60.0):
"""
- events_wait waits for and returns a named event
- from QMP with a timeout.
+ events_wait waits for and returns a single named event from QMP.
+ In the case of multiple qualifying events, this function returns the
+ first one.
- events: a sequence of (name, match_criteria) tuples.
- The match criteria are optional and may be None.
- See event_match for details.
- timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+ :param events: A sequence of (name, match_criteria) tuples.
+ The match criteria are optional and may be None.
+ See event_match for details.
+ :param timeout: Optional timeout, in seconds.
+ See QEMUMonitorProtocol.pull_event.
+
+ :raise QMPTimeoutError: If timeout was non-zero and no matching events
+ were found.
+ :return: A QMP event matching the filter criteria.
+ If timeout was 0 and no event matched, None.
"""
def _match(event):
for name, match in events:
@@ -558,6 +565,8 @@ def _match(event):
return True
return False
+ event: Optional[QMPMessage]
+
# Search cached events
for event in self._events:
if _match(event):
@@ -567,6 +576,10 @@ def _match(event):
# Poll for new events
while True:
event = self._qmp.pull_event(wait=timeout)
+ if event is None:
+ # NB: None is only returned when timeout is false-ish.
+ # Timeouts raise QMPTimeoutError instead!
+ break
if _match(event):
return event
self._events.append(event)
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 11/16] python/machine.py: use qmp.command
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (9 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 10/16] python/machine.py: Handle None events in events_wait John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 12/16] python/machine.py: Add _qmp access shim John Snow
` (6 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
machine.py and qmp.py both do the same thing here; refactor machine.py
to use qmp.py's functionality more directly.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 4afd67a9351..d8289936816 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -26,6 +26,8 @@
import socket
import tempfile
from typing import (
+ Any,
+ Dict,
List,
Optional,
Type,
@@ -455,17 +457,23 @@ def set_qmp_monitor(self, enabled=True):
self._qmp_set = False
self._qmp = None
- def qmp(self, cmd, conv_keys=True, **args):
- """
- Invoke a QMP command and return the response dict
- """
+ @classmethod
+ def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
qmp_args = dict()
for key, value in args.items():
- if conv_keys:
+ if _conv_keys:
qmp_args[key.replace('_', '-')] = value
else:
qmp_args[key] = value
+ return qmp_args
+ def qmp(self, cmd: str,
+ conv_keys: bool = True,
+ **args: Any) -> QMPMessage:
+ """
+ Invoke a QMP command and return the response dict
+ """
+ qmp_args = self._qmp_args(conv_keys, **args)
return self._qmp.cmd(cmd, args=qmp_args)
def command(self, cmd, conv_keys=True, **args):
@@ -474,12 +482,8 @@ def command(self, cmd, conv_keys=True, **args):
On success return the response dict.
On failure raise an exception.
"""
- reply = self.qmp(cmd, conv_keys, **args)
- if reply is None:
- raise qmp.QMPError("Monitor is closed")
- if "error" in reply:
- raise qmp.QMPResponseError(reply)
- return reply["return"]
+ qmp_args = self._qmp_args(conv_keys, **args)
+ return self._qmp.command(cmd, **qmp_args)
def get_qmp_event(self, wait=False):
"""
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (10 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 11/16] python/machine.py: use qmp.command John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-20 8:14 ` Philippe Mathieu-Daudé
2020-06-04 20:22 ` [PATCH v3 13/16] python/machine.py: fix _popen access John Snow
` (5 subsequent siblings)
17 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
Like many other Optional[] types, it's not always a given that this
object will be set. Wrap it in a type-shim that raises a meaningful
error and will always return a concrete type.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index d8289936816..a451f9000d6 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
self._events = []
self._iolog = None
self._qmp_set = True # Enable QMP monitor by default.
- self._qmp = None
+ self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
self._qemu_full_args = None
self._temp_dir = None
self._launched = False
@@ -285,7 +285,7 @@ def _pre_launch(self):
if self._remove_monitor_sockfile:
assert isinstance(self._monitor_address, str)
self._remove_files.append(self._monitor_address)
- self._qmp = qmp.QEMUMonitorProtocol(
+ self._qmp_connection = qmp.QEMUMonitorProtocol(
self._monitor_address,
server=True,
nickname=self._name
@@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
self._qmp_set = True
else:
self._qmp_set = False
- self._qmp = None
+ self._qmp_connection = None
+
+ @property
+ def _qmp(self) -> qmp.QEMUMonitorProtocol:
+ if self._qmp_connection is None:
+ raise QEMUMachineError("Attempt to access QMP with no connection")
+ return self._qmp_connection
@classmethod
def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-04 20:22 ` [PATCH v3 12/16] python/machine.py: Add _qmp access shim John Snow
@ 2020-06-20 8:14 ` Philippe Mathieu-Daudé
2020-06-20 8:20 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-20 8:14 UTC (permalink / raw)
To: John Snow, qemu-devel, Eduardo Habkost,
Wainer dos Santos Moschetta
Cc: Kevin Wolf, Cleber Rosa, qemu-block, Max Reitz
On 6/4/20 10:22 PM, John Snow wrote:
> Like many other Optional[] types, it's not always a given that this
> object will be set. Wrap it in a type-shim that raises a meaningful
> error and will always return a concrete type.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine.py | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index d8289936816..a451f9000d6 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
> self._events = []
> self._iolog = None
> self._qmp_set = True # Enable QMP monitor by default.
> - self._qmp = None
> + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
> self._qemu_full_args = None
> self._temp_dir = None
> self._launched = False
> @@ -285,7 +285,7 @@ def _pre_launch(self):
> if self._remove_monitor_sockfile:
> assert isinstance(self._monitor_address, str)
> self._remove_files.append(self._monitor_address)
> - self._qmp = qmp.QEMUMonitorProtocol(
> + self._qmp_connection = qmp.QEMUMonitorProtocol(
> self._monitor_address,
> server=True,
> nickname=self._name
> @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
> self._qmp_set = True
> else:
> self._qmp_set = False
> - self._qmp = None
> + self._qmp_connection = None
> +
> + @property
> + def _qmp(self) -> qmp.QEMUMonitorProtocol:
> + if self._qmp_connection is None:
> + raise QEMUMachineError("Attempt to access QMP with no connection")
> + return self._qmp_connection
>
> @classmethod
> def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>
This patch breaks the EmptyCPUModel test:
(043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
ERROR: Attempt to access QMP with no connection (0.03 s)
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-20 8:14 ` Philippe Mathieu-Daudé
@ 2020-06-20 8:20 ` Philippe Mathieu-Daudé
2020-06-22 10:23 ` Kevin Wolf
0 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-20 8:20 UTC (permalink / raw)
To: John Snow, QEMU Developers, Eduardo Habkost,
Wainer dos Santos Moschetta
Cc: Kevin Wolf, Cleber Rosa, Qemu-block, Max Reitz
On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 6/4/20 10:22 PM, John Snow wrote:
> > Like many other Optional[] types, it's not always a given that this
> > object will be set. Wrap it in a type-shim that raises a meaningful
> > error and will always return a concrete type.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > python/qemu/machine.py | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index d8289936816..a451f9000d6 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
> > self._events = []
> > self._iolog = None
> > self._qmp_set = True # Enable QMP monitor by default.
> > - self._qmp = None
> > + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
> > self._qemu_full_args = None
> > self._temp_dir = None
> > self._launched = False
> > @@ -285,7 +285,7 @@ def _pre_launch(self):
> > if self._remove_monitor_sockfile:
> > assert isinstance(self._monitor_address, str)
> > self._remove_files.append(self._monitor_address)
> > - self._qmp = qmp.QEMUMonitorProtocol(
> > + self._qmp_connection = qmp.QEMUMonitorProtocol(
> > self._monitor_address,
> > server=True,
> > nickname=self._name
> > @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
> > self._qmp_set = True
> > else:
> > self._qmp_set = False
> > - self._qmp = None
> > + self._qmp_connection = None
> > +
> > + @property
> > + def _qmp(self) -> qmp.QEMUMonitorProtocol:
> > + if self._qmp_connection is None:
> > + raise QEMUMachineError("Attempt to access QMP with no connection")
> > + return self._qmp_connection
> >
> > @classmethod
> > def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> >
>
> This patch breaks the EmptyCPUModel test:
>
> (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
> ERROR: Attempt to access QMP with no connection (0.03 s)
Fixed with:
-- >8 --
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index ba6397dd7e..26ae7be89b 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
@property
def _qmp(self) -> qmp.QEMUMonitorProtocol:
- if self._qmp_connection is None:
+ if self._qmp_set and self._qmp_connection is None:
raise QEMUMachineError("Attempt to access QMP with no connection")
return self._qmp_connection
---
Does that sound reasonable to you?
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-20 8:20 ` Philippe Mathieu-Daudé
@ 2020-06-22 10:23 ` Kevin Wolf
2020-06-22 11:32 ` Philippe Mathieu-Daudé
2020-06-22 14:24 ` John Snow
0 siblings, 2 replies; 34+ messages in thread
From: Kevin Wolf @ 2020-06-22 10:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eduardo Habkost, Qemu-block, QEMU Developers,
Wainer dos Santos Moschetta, Max Reitz, Cleber Rosa, John Snow
Am 20.06.2020 um 10:20 hat Philippe Mathieu-Daudé geschrieben:
> On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > On 6/4/20 10:22 PM, John Snow wrote:
> > > Like many other Optional[] types, it's not always a given that this
> > > object will be set. Wrap it in a type-shim that raises a meaningful
> > > error and will always return a concrete type.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > > python/qemu/machine.py | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > > index d8289936816..a451f9000d6 100644
> > > --- a/python/qemu/machine.py
> > > +++ b/python/qemu/machine.py
> > > @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
> > > self._events = []
> > > self._iolog = None
> > > self._qmp_set = True # Enable QMP monitor by default.
> > > - self._qmp = None
> > > + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
> > > self._qemu_full_args = None
> > > self._temp_dir = None
> > > self._launched = False
> > > @@ -285,7 +285,7 @@ def _pre_launch(self):
> > > if self._remove_monitor_sockfile:
> > > assert isinstance(self._monitor_address, str)
> > > self._remove_files.append(self._monitor_address)
> > > - self._qmp = qmp.QEMUMonitorProtocol(
> > > + self._qmp_connection = qmp.QEMUMonitorProtocol(
> > > self._monitor_address,
> > > server=True,
> > > nickname=self._name
> > > @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
> > > self._qmp_set = True
> > > else:
> > > self._qmp_set = False
> > > - self._qmp = None
> > > + self._qmp_connection = None
> > > +
> > > + @property
> > > + def _qmp(self) -> qmp.QEMUMonitorProtocol:
> > > + if self._qmp_connection is None:
> > > + raise QEMUMachineError("Attempt to access QMP with no connection")
> > > + return self._qmp_connection
> > >
> > > @classmethod
> > > def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> > >
> >
> > This patch breaks the EmptyCPUModel test:
> >
> > (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
> > ERROR: Attempt to access QMP with no connection (0.03 s)
>
> Fixed with:
>
> -- >8 --
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index ba6397dd7e..26ae7be89b 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>
> @property
> def _qmp(self) -> qmp.QEMUMonitorProtocol:
> - if self._qmp_connection is None:
> + if self._qmp_set and self._qmp_connection is None:
> raise QEMUMachineError("Attempt to access QMP with no connection")
> return self._qmp_connection
>
> ---
>
> Does that sound reasonable to you?
Wouldn't that make the return type Optional[qmp.QEMUMonitorProtocol]?
Maybe this is what we want, but then we don't need the shim that this
patch adds but can just declare the variable this way.
And why does the feeling code even try to acess _qmp when _qmp_set is
False? Shouldn't it first check whether it's even valid?
Or maybe going a step back, why do we even have a separate _qmp_set
instead of only using None for _qmp?
Kevin
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-22 10:23 ` Kevin Wolf
@ 2020-06-22 11:32 ` Philippe Mathieu-Daudé
2020-06-26 20:34 ` John Snow
2020-06-22 14:24 ` John Snow
1 sibling, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 11:32 UTC (permalink / raw)
To: Kevin Wolf
Cc: Eduardo Habkost, Qemu-block, QEMU Developers,
Wainer dos Santos Moschetta, Max Reitz, Cleber Rosa, John Snow
On 6/22/20 12:23 PM, Kevin Wolf wrote:
> Am 20.06.2020 um 10:20 hat Philippe Mathieu-Daudé geschrieben:
>> On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
>> <philmd@redhat.com> wrote:
>>>
>>> On 6/4/20 10:22 PM, John Snow wrote:
>>>> Like many other Optional[] types, it's not always a given that this
>>>> object will be set. Wrap it in a type-shim that raises a meaningful
>>>> error and will always return a concrete type.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> python/qemu/machine.py | 12 +++++++++---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>>> index d8289936816..a451f9000d6 100644
>>>> --- a/python/qemu/machine.py
>>>> +++ b/python/qemu/machine.py
>>>> @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>>>> self._events = []
>>>> self._iolog = None
>>>> self._qmp_set = True # Enable QMP monitor by default.
>>>> - self._qmp = None
>>>> + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
>>>> self._qemu_full_args = None
>>>> self._temp_dir = None
>>>> self._launched = False
>>>> @@ -285,7 +285,7 @@ def _pre_launch(self):
>>>> if self._remove_monitor_sockfile:
>>>> assert isinstance(self._monitor_address, str)
>>>> self._remove_files.append(self._monitor_address)
>>>> - self._qmp = qmp.QEMUMonitorProtocol(
>>>> + self._qmp_connection = qmp.QEMUMonitorProtocol(
>>>> self._monitor_address,
>>>> server=True,
>>>> nickname=self._name
>>>> @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
>>>> self._qmp_set = True
>>>> else:
>>>> self._qmp_set = False
>>>> - self._qmp = None
>>>> + self._qmp_connection = None
>>>> +
>>>> + @property
>>>> + def _qmp(self) -> qmp.QEMUMonitorProtocol:
>>>> + if self._qmp_connection is None:
>>>> + raise QEMUMachineError("Attempt to access QMP with no connection")
>>>> + return self._qmp_connection
>>>>
>>>> @classmethod
>>>> def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>>>>
>>>
>>> This patch breaks the EmptyCPUModel test:
>>>
>>> (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
>>> ERROR: Attempt to access QMP with no connection (0.03 s)
>>
>> Fixed with:
>>
>> -- >8 --
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index ba6397dd7e..26ae7be89b 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>>
>> @property
>> def _qmp(self) -> qmp.QEMUMonitorProtocol:
>> - if self._qmp_connection is None:
>> + if self._qmp_set and self._qmp_connection is None:
>> raise QEMUMachineError("Attempt to access QMP with no connection")
>> return self._qmp_connection
>>
>> ---
>>
>> Does that sound reasonable to you?
>
> Wouldn't that make the return type Optional[qmp.QEMUMonitorProtocol]?
> Maybe this is what we want, but then we don't need the shim that this
> patch adds but can just declare the variable this way.
>
> And why does the feeling code even try to acess _qmp when _qmp_set is
> False? Shouldn't it first check whether it's even valid?
>
> Or maybe going a step back, why do we even have a separate _qmp_set
> instead of only using None for _qmp?
Better indeed.
John, at this point from a maintenance perspective it is easier
if you respin the series (and please, run the Travis-CI tests).
Regards,
Phil.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-22 11:32 ` Philippe Mathieu-Daudé
@ 2020-06-26 20:34 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-26 20:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Kevin Wolf
Cc: Eduardo Habkost, Qemu-block, QEMU Developers,
Wainer dos Santos Moschetta, Max Reitz, Cleber Rosa
On 6/22/20 7:32 AM, Philippe Mathieu-Daudé wrote:
> On 6/22/20 12:23 PM, Kevin Wolf wrote:
>> Am 20.06.2020 um 10:20 hat Philippe Mathieu-Daudé geschrieben:
>>> On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
>>> <philmd@redhat.com> wrote:
>>>>
>>>> On 6/4/20 10:22 PM, John Snow wrote:
>>>>> Like many other Optional[] types, it's not always a given that this
>>>>> object will be set. Wrap it in a type-shim that raises a meaningful
>>>>> error and will always return a concrete type.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> python/qemu/machine.py | 12 +++++++++---
>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>>>> index d8289936816..a451f9000d6 100644
>>>>> --- a/python/qemu/machine.py
>>>>> +++ b/python/qemu/machine.py
>>>>> @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>>>>> self._events = []
>>>>> self._iolog = None
>>>>> self._qmp_set = True # Enable QMP monitor by default.
>>>>> - self._qmp = None
>>>>> + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
>>>>> self._qemu_full_args = None
>>>>> self._temp_dir = None
>>>>> self._launched = False
>>>>> @@ -285,7 +285,7 @@ def _pre_launch(self):
>>>>> if self._remove_monitor_sockfile:
>>>>> assert isinstance(self._monitor_address, str)
>>>>> self._remove_files.append(self._monitor_address)
>>>>> - self._qmp = qmp.QEMUMonitorProtocol(
>>>>> + self._qmp_connection = qmp.QEMUMonitorProtocol(
>>>>> self._monitor_address,
>>>>> server=True,
>>>>> nickname=self._name
>>>>> @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
>>>>> self._qmp_set = True
>>>>> else:
>>>>> self._qmp_set = False
>>>>> - self._qmp = None
>>>>> + self._qmp_connection = None
>>>>> +
>>>>> + @property
>>>>> + def _qmp(self) -> qmp.QEMUMonitorProtocol:
>>>>> + if self._qmp_connection is None:
>>>>> + raise QEMUMachineError("Attempt to access QMP with no connection")
>>>>> + return self._qmp_connection
>>>>>
>>>>> @classmethod
>>>>> def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>>>>>
>>>>
>>>> This patch breaks the EmptyCPUModel test:
>>>>
>>>> (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
>>>> ERROR: Attempt to access QMP with no connection (0.03 s)
>>>
>>> Fixed with:
>>>
>>> -- >8 --
>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>> index ba6397dd7e..26ae7be89b 100644
>>> --- a/python/qemu/machine.py
>>> +++ b/python/qemu/machine.py
>>> @@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>>>
>>> @property
>>> def _qmp(self) -> qmp.QEMUMonitorProtocol:
>>> - if self._qmp_connection is None:
>>> + if self._qmp_set and self._qmp_connection is None:
>>> raise QEMUMachineError("Attempt to access QMP with no connection")
>>> return self._qmp_connection
>>>
>>> ---
>>>
>>> Does that sound reasonable to you?
>>
Not entirely; the idea is to protect access to the socket in all cases
where the socket was not created, for any and all reasons.
>> Wouldn't that make the return type Optional[qmp.QEMUMonitorProtocol]?
>> Maybe this is what we want, but then we don't need the shim that this
>> patch adds but can just declare the variable this way.
>>
The _qmp_connection variable is already Optional[], so you could indeed
remove the shim. The shim is here to peel away the Optional[] so you
don't need the "if qmp" checks everywhere.
>> And why does the feeling code even try to acess _qmp when _qmp_set is
>> False? Shouldn't it first check whether it's even valid?
>>
Yup. The code in lifetime management functions should check for the
socket. Code in qmp-centric commands should assume it's there with the shim.
>> Or maybe going a step back, why do we even have a separate _qmp_set
>> instead of only using None for _qmp?
Because the socket isn't initialized until after launch, so "_qmp_set"
is a request/instruction and not reflective of current state.
_qmp_set is the flag for a desire to create QMP or not,
_qmp_connection is the actual connection.
set | conn |
T F | OK (we are pre-launch)
T T | OK (we are running)
F F | OK (we set no QMP before launch)
F T | Set QMP false after launch
The last one doesn't make a lot of sense. Worse, it just None's the
socket variable so we can't even close it properly.
That's sorta busted, so I am patching it to just simply set the variable.
>
> Better indeed.
>
> John, at this point from a maintenance perspective it is easier
> if you respin the series (and please, run the Travis-CI tests).
>
> Regards,
>
> Phil.
>
OK, I am resending. I didn't make any big changes, but I thought about it.
(Read as: I tried, didn't like how it looked, and came back to this
patchset which is fairly close as-is.)
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 12/16] python/machine.py: Add _qmp access shim
2020-06-22 10:23 ` Kevin Wolf
2020-06-22 11:32 ` Philippe Mathieu-Daudé
@ 2020-06-22 14:24 ` John Snow
1 sibling, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-22 14:24 UTC (permalink / raw)
To: Kevin Wolf, Philippe Mathieu-Daudé
Cc: Eduardo Habkost, Qemu-block, QEMU Developers,
Wainer dos Santos Moschetta, Max Reitz, Cleber Rosa
On 6/22/20 6:23 AM, Kevin Wolf wrote:
> Am 20.06.2020 um 10:20 hat Philippe Mathieu-Daudé geschrieben:
>> On Sat, Jun 20, 2020 at 10:14 AM Philippe Mathieu-Daudé
>> <philmd@redhat.com> wrote:
>>>
>>> On 6/4/20 10:22 PM, John Snow wrote:
>>>> Like many other Optional[] types, it's not always a given that this
>>>> object will be set. Wrap it in a type-shim that raises a meaningful
>>>> error and will always return a concrete type.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> python/qemu/machine.py | 12 +++++++++---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>>> index d8289936816..a451f9000d6 100644
>>>> --- a/python/qemu/machine.py
>>>> +++ b/python/qemu/machine.py
>>>> @@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>>>> self._events = []
>>>> self._iolog = None
>>>> self._qmp_set = True # Enable QMP monitor by default.
>>>> - self._qmp = None
>>>> + self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
>>>> self._qemu_full_args = None
>>>> self._temp_dir = None
>>>> self._launched = False
>>>> @@ -285,7 +285,7 @@ def _pre_launch(self):
>>>> if self._remove_monitor_sockfile:
>>>> assert isinstance(self._monitor_address, str)
>>>> self._remove_files.append(self._monitor_address)
>>>> - self._qmp = qmp.QEMUMonitorProtocol(
>>>> + self._qmp_connection = qmp.QEMUMonitorProtocol(
>>>> self._monitor_address,
>>>> server=True,
>>>> nickname=self._name
>>>> @@ -455,7 +455,13 @@ def set_qmp_monitor(self, enabled=True):
>>>> self._qmp_set = True
>>>> else:
>>>> self._qmp_set = False
>>>> - self._qmp = None
>>>> + self._qmp_connection = None
>>>> +
>>>> + @property
>>>> + def _qmp(self) -> qmp.QEMUMonitorProtocol:
>>>> + if self._qmp_connection is None:
>>>> + raise QEMUMachineError("Attempt to access QMP with no connection")
>>>> + return self._qmp_connection
>>>>
>>>> @classmethod
>>>> def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>>>>
>>>
>>> This patch breaks the EmptyCPUModel test:
>>>
>>> (043/101) tests/acceptance/empty_cpu_model.py:EmptyCPUModel.test:
>>> ERROR: Attempt to access QMP with no connection (0.03 s)
>>
>> Fixed with:
>>
>> -- >8 --
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index ba6397dd7e..26ae7be89b 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -480,7 +480,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>>
>> @property
>> def _qmp(self) -> qmp.QEMUMonitorProtocol:
>> - if self._qmp_connection is None:
>> + if self._qmp_set and self._qmp_connection is None:
>> raise QEMUMachineError("Attempt to access QMP with no connection")
>> return self._qmp_connection
>>
>> ---
>>
>> Does that sound reasonable to you?
>
> Wouldn't that make the return type Optional[qmp.QEMUMonitorProtocol]?
> Maybe this is what we want, but then we don't need the shim that this
> patch adds but can just declare the variable this way.
>
> And why does the feeling code even try to acess _qmp when _qmp_set is
> False? Shouldn't it first check whether it's even valid?
>
> Or maybe going a step back, why do we even have a separate _qmp_set
> instead of only using None for _qmp?
>
> Kevin
>
Using Optional[qmp.QEMUMonitorProtocol] does work; it does require a lot
more "if not None" checks littered everywhere.
From memory, it was just difficult to type a qmp(...) passthrough
command that integrated the "if self._qmp_socket" check -- the faster
hack was the shim around accessing the socket itself.
IIRC, mypy had difficulty reasoning about constraints otherwise -- but
I'll look into it again. At this point it warrants stronger effort.
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 13/16] python/machine.py: fix _popen access
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (11 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 12/16] python/machine.py: Add _qmp access shim John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 14/16] python/qemu: make 'args' style arguments immutable John Snow
` (4 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
As always, Optional[T] causes problems with unchecked access. Add a
helper that asserts the pipe is present before we attempt to talk with
it.
Signed-off-by: John Snow <jsnow@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 a451f9000d6..a81a422b60e 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -114,7 +114,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
# Runstate
self._qemu_log_path = None
self._qemu_log_file = None
- self._popen = None
+ self._popen: Optional['subprocess.Popen[bytes]'] = None
self._events = []
self._iolog = None
self._qmp_set = True # Enable QMP monitor by default.
@@ -227,6 +227,12 @@ def is_running(self):
"""Returns true if the VM is running."""
return self._popen is not None and self._popen.poll() is None
+ @property
+ def _subp(self) -> 'subprocess.Popen[bytes]':
+ if self._popen is None:
+ raise QEMUMachineError('Subprocess pipe not present')
+ return self._popen
+
def exitcode(self):
"""Returns the exit code if possible, or None."""
if self._popen is None:
@@ -237,7 +243,7 @@ def get_pid(self):
"""Returns the PID of the running process, or None."""
if not self.is_running():
return None
- return self._popen.pid
+ return self._subp.pid
def _load_io_log(self):
if self._qemu_log_path is not None:
@@ -369,7 +375,7 @@ def wait(self):
"""
Wait for the VM to power off
"""
- self._popen.wait()
+ self._subp.wait()
if self._qmp:
self._qmp.close()
self._post_shutdown()
@@ -381,8 +387,8 @@ def _hard_shutdown(self) -> None:
if not self.is_running():
return
- self._popen.kill()
- self._popen.wait(timeout=60)
+ self._subp.kill()
+ self._subp.wait(timeout=60)
def _soft_shutdown(self, has_quit: bool = False, timeout: int = 3) -> None:
"""
@@ -399,7 +405,7 @@ def _soft_shutdown(self, has_quit: bool = False, timeout: int = 3) -> None:
self._qmp.cmd('quit')
self._qmp.close()
- self._popen.wait(timeout=timeout)
+ self._subp.wait(timeout=timeout)
def _do_shutdown(self, has_quit: bool = False, timeout: int = 3) -> None:
"""
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 14/16] python/qemu: make 'args' style arguments immutable
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (12 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 13/16] python/machine.py: fix _popen access John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 15/16] iotests.py: Adjust HMP kwargs typing John Snow
` (3 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
These arguments don't need to be mutable and aren't really used as
such. Clarify their types as immutable and adjust code to match where
necessary.
In general, It's probably best not to accept a user-defined mutable
object and store it as internal object state unless there's a strong
justification for doing so. Instead, try to use generic types as input
with empty tuples as the default, and coerce to list where necessary.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 30 +++++++++++++++++-------------
python/qemu/qtest.py | 16 ++++++++++++----
2 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index a81a422b60e..c053c946401 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -18,6 +18,7 @@
#
import errno
+from itertools import chain
import logging
import os
import subprocess
@@ -30,6 +31,8 @@
Dict,
List,
Optional,
+ Sequence,
+ Tuple,
Type,
)
from types import TracebackType
@@ -68,8 +71,12 @@ class QEMUMachine:
# vm is guaranteed to be shut down here
"""
- def __init__(self, binary, args=None, wrapper=None, name=None,
- test_dir="/var/tmp",
+ def __init__(self,
+ binary: str,
+ args: Sequence[str] = (),
+ wrapper: Sequence[str] = (),
+ name: Optional[str] = None,
+ test_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
socket_scm_helper=None, sock_dir=None):
'''
@@ -87,14 +94,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
# Direct user configuration
self._binary = binary
-
- if args is None:
- args = []
- # Copy mutable input: we will be modifying our copy
self._args = list(args)
-
- if wrapper is None:
- wrapper = []
self._wrapper = wrapper
self._name = name or "qemu-%d" % os.getpid()
@@ -119,7 +119,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
self._iolog = None
self._qmp_set = True # Enable QMP monitor by default.
self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
- self._qemu_full_args = None
+ self._qemu_full_args: Tuple[str, ...] = ()
self._temp_dir = None
self._launched = False
self._machine = None
@@ -340,7 +340,7 @@ def launch(self):
raise QEMUMachineError('VM already launched')
self._iolog = None
- self._qemu_full_args = None
+ self._qemu_full_args = ()
try:
self._launch()
self._launched = True
@@ -360,8 +360,12 @@ def _launch(self):
"""
devnull = open(os.path.devnull, 'rb')
self._pre_launch()
- self._qemu_full_args = (self._wrapper + [self._binary] +
- self._base_args + self._args)
+ self._qemu_full_args = tuple(
+ chain(self._wrapper,
+ [self._binary],
+ self._base_args,
+ self._args)
+ )
LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
self._popen = subprocess.Popen(self._qemu_full_args,
stdin=devnull,
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 05c63a1d583..ae4661d4d3e 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -22,6 +22,7 @@
from typing import (
List,
Optional,
+ Sequence,
TextIO,
)
@@ -103,8 +104,13 @@ class QEMUQtestMachine(QEMUMachine):
A QEMU VM, with a qtest socket available.
"""
- def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
- socket_scm_helper=None, sock_dir=None):
+ def __init__(self,
+ binary: str,
+ args: Sequence[str] = (),
+ name: Optional[str] = None,
+ test_dir: str = "/var/tmp",
+ socket_scm_helper: Optional[str] = None,
+ sock_dir: Optional[str] = None):
if name is None:
name = "qemu-%d" % os.getpid()
if sock_dir is None:
@@ -118,8 +124,10 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
@property
def _base_args(self) -> List[str]:
args = super()._base_args
- args.extend(['-qtest', 'unix:path=' + self._qtest_path,
- '-accel', 'qtest'])
+ args.extend([
+ '-qtest', f"unix:path={self._qtest_path}",
+ '-accel', 'qtest'
+ ])
return args
def _pre_launch(self):
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 15/16] iotests.py: Adjust HMP kwargs typing
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (13 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 14/16] python/qemu: make 'args' style arguments immutable John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-04 20:22 ` [PATCH v3 16/16] python/qemu: Add mypy type annotations John Snow
` (2 subsequent siblings)
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
mypy wants to ensure there's consistency between the kwargs arguments
types and any unspecified keyword arguments. In this case, conv_keys is
a bool, but the remaining keys are Any type. Mypy (correctly) infers the
**kwargs type to be **Dict[str, str], which is not compatible with
conv_keys: bool.
Because QMP typing is a little fraught right now anyway, re-type kwargs
to Dict[str, Any] which has the benefit of silencing this check right
now.
A future re-design might type these more aggressively, but this will
give us a baseline to work from with minimal disruption.
(Thanks Kevin Wolf for the debugging assist here)
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7c1773bba37..19b22bee618 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -553,7 +553,7 @@ def add_incoming(self, addr):
def hmp(self, command_line: str, use_log: bool = False) -> QMPMessage:
cmd = 'human-monitor-command'
- kwargs = {'command-line': command_line}
+ kwargs: Dict[str, Any] = {'command-line': command_line}
if use_log:
return self.qmp_log(cmd, **kwargs)
else:
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 16/16] python/qemu: Add mypy type annotations
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (14 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 15/16] iotests.py: Adjust HMP kwargs typing John Snow
@ 2020-06-04 20:22 ` John Snow
2020-06-05 9:26 ` [PATCH v3 00/16] python: add mypy support to python/qemu Kevin Wolf
2020-06-17 17:18 ` [PATCH v3 00/16] python: add mypy support to python/qemu Philippe Mathieu-Daudé
17 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-04 20:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, John Snow, Max Reitz,
Cleber Rosa, philmd
These should all be purely annotations with no changes in behavior at
all. You need to be in the python folder, but you should be able to
confirm that these annotations are correct (or at least self-consistent)
by running `mypy --strict qemu`.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/accel.py | 8 ++--
python/qemu/machine.py | 94 ++++++++++++++++++++++++------------------
python/qemu/qmp.py | 44 +++++++++++---------
python/qemu/qtest.py | 26 +++++++-----
4 files changed, 98 insertions(+), 74 deletions(-)
diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 7fabe629208..4325114e51f 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -17,6 +17,7 @@
import logging
import os
import subprocess
+from typing import List, Optional
LOG = logging.getLogger(__name__)
@@ -29,7 +30,7 @@
}
-def list_accel(qemu_bin):
+def list_accel(qemu_bin: str) -> List[str]:
"""
List accelerators enabled in the QEMU binary.
@@ -49,7 +50,8 @@ def list_accel(qemu_bin):
return [acc.strip() for acc in out.splitlines()[1:]]
-def kvm_available(target_arch=None, qemu_bin=None):
+def kvm_available(target_arch: Optional[str] = None,
+ qemu_bin: Optional[str] = None) -> bool:
"""
Check if KVM is available using the following heuristic:
- Kernel module is present in the host;
@@ -72,7 +74,7 @@ def kvm_available(target_arch=None, qemu_bin=None):
return True
-def tcg_available(qemu_bin):
+def tcg_available(qemu_bin: str) -> bool:
"""
Check if TCG is available.
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c053c946401..d4da4e8ce15 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -28,6 +28,7 @@
import tempfile
from typing import (
Any,
+ BinaryIO,
Dict,
List,
Optional,
@@ -38,7 +39,7 @@
from types import TracebackType
from . import qmp
-from .qmp import SocketAddrT, QMPMessage
+from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
LOG = logging.getLogger(__name__)
@@ -61,7 +62,7 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
class QEMUMachine:
"""
- A QEMU VM
+ A QEMU VM.
Use this object as a context manager to ensure
the QEMU process terminates::
@@ -78,7 +79,8 @@ def __init__(self,
name: Optional[str] = None,
test_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper=None, sock_dir=None):
+ socket_scm_helper: Optional[str] = None,
+ sock_dir: Optional[str] = None):
'''
Initialize a QEMUMachine
@@ -112,28 +114,28 @@ def __init__(self,
self._remove_monitor_sockfile = True
# Runstate
- self._qemu_log_path = None
- self._qemu_log_file = None
+ self._qemu_log_path: Optional[str] = None
+ self._qemu_log_file: Optional[BinaryIO] = None
self._popen: Optional['subprocess.Popen[bytes]'] = None
- self._events = []
- self._iolog = None
+ self._events: List[QMPMessage] = []
+ self._iolog: Optional[str] = None
self._qmp_set = True # Enable QMP monitor by default.
self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
self._qemu_full_args: Tuple[str, ...] = ()
- self._temp_dir = None
+ self._temp_dir: Optional[str] = None
self._launched = False
- self._machine = None
+ self._machine: Optional[str] = None
self._console_index = 0
self._console_set = False
- self._console_device_type = None
+ self._console_device_type: Optional[str] = None
self._console_address = os.path.join(
self._sock_dir, f"{self._name}-console.sock"
)
- self._console_socket = None
- self._remove_files = []
+ self._console_socket: Optional[socket.socket] = None
+ self._remove_files: List[str] = []
self._killed = False
- def __enter__(self):
+ def __enter__(self) -> 'QEMUMachine':
return self
def __exit__(self,
@@ -142,14 +144,15 @@ def __exit__(self,
exc_tb: Optional[TracebackType]) -> None:
self.shutdown()
- def add_monitor_null(self):
+ def add_monitor_null(self) -> None:
"""
This can be used to add an unused monitor instance.
"""
self._args.append('-monitor')
self._args.append('null')
- def add_fd(self, fd, fdset, opaque, opts=''):
+ def add_fd(self, fd: int, fdset: int,
+ opaque: str, opts: str = '') -> 'QEMUMachine':
"""
Pass a file descriptor to the VM
"""
@@ -168,7 +171,8 @@ def add_fd(self, fd, fdset, opaque, opts=''):
self._args.append(','.join(options))
return self
- def send_fd_scm(self, fd=None, file_path=None):
+ def send_fd_scm(self, fd: Optional[int] = None,
+ file_path: Optional[str] = None) -> int:
"""
Send an fd or file_path to socket_scm_helper.
@@ -212,7 +216,7 @@ def send_fd_scm(self, fd=None, file_path=None):
return proc.returncode
@staticmethod
- def _remove_if_exists(path):
+ def _remove_if_exists(path: str) -> None:
"""
Remove file object at path if it exists
"""
@@ -223,7 +227,7 @@ def _remove_if_exists(path):
return
raise
- def is_running(self):
+ def is_running(self) -> bool:
"""Returns true if the VM is running."""
return self._popen is not None and self._popen.poll() is None
@@ -233,19 +237,19 @@ def _subp(self) -> 'subprocess.Popen[bytes]':
raise QEMUMachineError('Subprocess pipe not present')
return self._popen
- def exitcode(self):
+ def exitcode(self) -> Optional[int]:
"""Returns the exit code if possible, or None."""
if self._popen is None:
return None
return self._popen.poll()
- def get_pid(self):
+ def get_pid(self) -> Optional[int]:
"""Returns the PID of the running process, or None."""
if not self.is_running():
return None
return self._subp.pid
- def _load_io_log(self):
+ def _load_io_log(self) -> None:
if self._qemu_log_path is not None:
with open(self._qemu_log_path, "r") as iolog:
self._iolog = iolog.read()
@@ -279,7 +283,7 @@ def _base_args(self) -> List[str]:
args.extend(['-device', device])
return args
- def _pre_launch(self):
+ def _pre_launch(self) -> None:
self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
self._qemu_log_file = open(self._qemu_log_path, 'wb')
@@ -297,7 +301,7 @@ def _pre_launch(self):
nickname=self._name
)
- def _post_launch(self):
+ def _post_launch(self) -> None:
if self._qmp:
self._qmp.accept()
@@ -330,7 +334,7 @@ def _post_shutdown(self) -> None:
self._killed = False
self._launched = False
- def launch(self):
+ def launch(self) -> None:
"""
Launch the VM and make sure we cleanup and expose the
command line/output in case of exception
@@ -354,7 +358,7 @@ def launch(self):
LOG.debug('Output: %r', self._iolog)
raise
- def _launch(self):
+ def _launch(self) -> None:
"""
Launch the VM and establish a QMP connection
"""
@@ -375,7 +379,7 @@ def _launch(self):
close_fds=False)
self._post_launch()
- def wait(self):
+ def wait(self) -> None:
"""
Wait for the VM to power off
"""
@@ -446,13 +450,13 @@ def shutdown(self, has_quit: bool = False, hard: bool = False) -> None:
finally:
self._post_shutdown()
- def kill(self):
+ def kill(self) -> None:
"""
Terminate the VM forcefully and perform cleanup.
"""
self.shutdown(hard=True)
- def set_qmp_monitor(self, enabled=True):
+ def set_qmp_monitor(self, enabled: bool = True) -> None:
"""
Set the QMP monitor.
@@ -492,7 +496,9 @@ def qmp(self, cmd: str,
qmp_args = self._qmp_args(conv_keys, **args)
return self._qmp.cmd(cmd, args=qmp_args)
- def command(self, cmd, conv_keys=True, **args):
+ def command(self, cmd: str,
+ conv_keys: bool = True,
+ **args: Any) -> QMPReturnValue:
"""
Invoke a QMP command.
On success return the response dict.
@@ -501,7 +507,7 @@ def command(self, cmd, conv_keys=True, **args):
qmp_args = self._qmp_args(conv_keys, **args)
return self._qmp.command(cmd, **qmp_args)
- def get_qmp_event(self, wait=False):
+ def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:
"""
Poll for one queued QMP events and return it
"""
@@ -509,7 +515,7 @@ def get_qmp_event(self, wait=False):
return self._events.pop(0)
return self._qmp.pull_event(wait=wait)
- def get_qmp_events(self, wait=False):
+ def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
"""
Poll for queued QMP events and return a list of dicts
"""
@@ -520,7 +526,7 @@ def get_qmp_events(self, wait=False):
return events
@staticmethod
- def event_match(event, match=None):
+ def event_match(event: Any, match: Optional[Any]) -> bool:
"""
Check if an event matches optional match criteria.
@@ -550,9 +556,11 @@ def event_match(event, match=None):
return True
except TypeError:
# either match or event wasn't iterable (not a dict)
- return match == event
+ return bool(match == event)
- def event_wait(self, name, timeout=60.0, match=None):
+ def event_wait(self, name: str,
+ timeout: float = 60.0,
+ match: Optional[QMPMessage] = None) -> Optional[QMPMessage]:
"""
event_wait waits for and returns a named event from QMP with a timeout.
@@ -562,7 +570,9 @@ def event_wait(self, name, timeout=60.0, match=None):
"""
return self.events_wait([(name, match)], timeout)
- def events_wait(self, events, timeout=60.0):
+ def events_wait(self,
+ events: Sequence[Tuple[str, Any]],
+ timeout: float = 60.0) -> Optional[QMPMessage]:
"""
events_wait waits for and returns a single named event from QMP.
In the case of multiple qualifying events, this function returns the
@@ -579,7 +589,7 @@ def events_wait(self, events, timeout=60.0):
:return: A QMP event matching the filter criteria.
If timeout was 0 and no event matched, None.
"""
- def _match(event):
+ def _match(event: QMPMessage) -> bool:
for name, match in events:
if event['event'] == name and self.event_match(event, match):
return True
@@ -606,20 +616,20 @@ def _match(event):
return None
- def get_log(self):
+ def get_log(self) -> Optional[str]:
"""
After self.shutdown or failed qemu execution, this returns the output
of the qemu process.
"""
return self._iolog
- def add_args(self, *args):
+ def add_args(self, *args: str) -> None:
"""
Adds to the list of extra arguments to be given to the QEMU binary
"""
self._args.extend(args)
- def set_machine(self, machine_type):
+ def set_machine(self, machine_type: str) -> None:
"""
Sets the machine type
@@ -628,7 +638,9 @@ def set_machine(self, machine_type):
"""
self._machine = machine_type
- def set_console(self, device_type=None, console_index=0):
+ def set_console(self,
+ device_type: Optional[str] = None,
+ console_index: int = 0) -> None:
"""
Sets the device type for a console device
@@ -659,7 +671,7 @@ def set_console(self, device_type=None, console_index=0):
self._console_index = console_index
@property
- def console_socket(self):
+ def console_socket(self) -> socket.socket:
"""
Returns a socket connected to the console
"""
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 7935dababbf..303e82ee6b4 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -15,6 +15,7 @@
Any,
cast,
Dict,
+ List,
Optional,
TextIO,
Type,
@@ -90,7 +91,9 @@ class QEMUMonitorProtocol:
#: Logger object for debugging messages
logger = logging.getLogger('QMP')
- def __init__(self, address, server=False, nickname=None):
+ def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
"""
Create a QEMUMonitorProtocol class.
@@ -102,7 +105,7 @@ def __init__(self, address, server=False, nickname=None):
@note No connection is established, this is done by the connect() or
accept() methods
"""
- self.__events = []
+ self.__events: List[QMPMessage] = []
self.__address = address
self.__sock = self.__get_sock()
self.__sockfile: Optional[TextIO] = None
@@ -114,14 +117,14 @@ def __init__(self, address, server=False, nickname=None):
self.__sock.bind(self.__address)
self.__sock.listen(1)
- def __get_sock(self):
+ def __get_sock(self) -> socket.socket:
if isinstance(self.__address, tuple):
family = socket.AF_INET
else:
family = socket.AF_UNIX
return socket.socket(family, socket.SOCK_STREAM)
- def __negotiate_capabilities(self):
+ def __negotiate_capabilities(self) -> QMPMessage:
greeting = self.__json_read()
if greeting is None or "QMP" not in greeting:
raise QMPConnectError
@@ -131,7 +134,7 @@ def __negotiate_capabilities(self):
return greeting
raise QMPCapabilitiesError
- def __json_read(self, only_event=False):
+ def __json_read(self, only_event: bool = False) -> Optional[QMPMessage]:
assert self.__sockfile is not None
while True:
data = self.__sockfile.readline()
@@ -148,7 +151,7 @@ def __json_read(self, only_event=False):
continue
return resp
- def __get_events(self, wait=False):
+ def __get_events(self, wait: Union[bool, float] = False) -> None:
"""
Check for new events in the stream and cache them in __events.
@@ -186,7 +189,7 @@ def __get_events(self, wait=False):
raise QMPConnectError("Error while reading from socket")
self.__sock.settimeout(None)
- def __enter__(self):
+ def __enter__(self) -> 'QEMUMonitorProtocol':
# Implement context manager enter function.
return self
@@ -199,7 +202,7 @@ def __exit__(self,
# Implement context manager exit function.
self.close()
- def connect(self, negotiate=True):
+ def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
"""
Connect to the QMP Monitor and perform capabilities negotiation.
@@ -214,7 +217,7 @@ def connect(self, negotiate=True):
return self.__negotiate_capabilities()
return None
- def accept(self, timeout=15.0):
+ def accept(self, timeout: float = 15.0) -> QMPMessage:
"""
Await connection from QMP Monitor and perform capabilities negotiation.
@@ -250,7 +253,9 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
self.logger.debug("<<< %s", resp)
return resp
- def cmd(self, name, args=None, cmd_id=None):
+ def cmd(self, name: str,
+ args: Optional[Dict[str, Any]] = None,
+ cmd_id: Optional[Any] = None) -> QMPMessage:
"""
Build a QMP command and send it to the QMP Monitor.
@@ -258,14 +263,14 @@ def cmd(self, name, args=None, cmd_id=None):
@param args: command arguments (dict)
@param cmd_id: command id (dict, list, string or int)
"""
- qmp_cmd = {'execute': name}
+ qmp_cmd: QMPMessage = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if cmd_id:
qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
- def command(self, cmd, **kwds):
+ def command(self, cmd: str, **kwds: Any) -> QMPReturnValue:
"""
Build and send a QMP command to the monitor, report errors if any
"""
@@ -278,7 +283,8 @@ def command(self, cmd, **kwds):
)
return cast(QMPReturnValue, ret['return'])
- def pull_event(self, wait=False):
+ def pull_event(self,
+ wait: Union[bool, float] = False) -> Optional[QMPMessage]:
"""
Pulls a single event.
@@ -298,7 +304,7 @@ def pull_event(self, wait=False):
return self.__events.pop(0)
return None
- def get_events(self, wait=False):
+ def get_events(self, wait: bool = False) -> List[QMPMessage]:
"""
Get a list of available QMP events.
@@ -315,13 +321,13 @@ def get_events(self, wait=False):
self.__get_events(wait)
return self.__events
- def clear_events(self):
+ def clear_events(self) -> None:
"""
Clear current list of pending events.
"""
self.__events = []
- def close(self):
+ def close(self) -> None:
"""
Close the socket and socket file.
"""
@@ -330,7 +336,7 @@ def close(self):
if self.__sockfile:
self.__sockfile.close()
- def settimeout(self, timeout):
+ def settimeout(self, timeout: float) -> None:
"""
Set the socket timeout.
@@ -339,7 +345,7 @@ def settimeout(self, timeout):
"""
self.__sock.settimeout(timeout)
- def get_sock_fd(self):
+ def get_sock_fd(self) -> int:
"""
Get the socket file descriptor.
@@ -347,7 +353,7 @@ def get_sock_fd(self):
"""
return self.__sock.fileno()
- def is_scm_available(self):
+ def is_scm_available(self) -> bool:
"""
Check if the socket allows for SCM_RIGHTS.
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index ae4661d4d3e..49c04bc98d9 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -27,6 +27,7 @@
)
from .machine import QEMUMachine
+from .qmp import SocketAddrT
class QEMUQtestProtocol:
@@ -43,7 +44,8 @@ class QEMUQtestProtocol:
No conection is estabalished by __init__(), this is done
by the connect() or accept() methods.
"""
- def __init__(self, address, server=False):
+ def __init__(self, address: SocketAddrT,
+ server: bool = False):
self._address = address
self._sock = self._get_sock()
self._sockfile: Optional[TextIO] = None
@@ -51,14 +53,14 @@ def __init__(self, address, server=False):
self._sock.bind(self._address)
self._sock.listen(1)
- def _get_sock(self):
+ def _get_sock(self) -> socket.socket:
if isinstance(self._address, tuple):
family = socket.AF_INET
else:
family = socket.AF_UNIX
return socket.socket(family, socket.SOCK_STREAM)
- def connect(self):
+ def connect(self) -> None:
"""
Connect to the qtest socket.
@@ -67,7 +69,7 @@ def connect(self):
self._sock.connect(self._address)
self._sockfile = self._sock.makefile(mode='r')
- def accept(self):
+ def accept(self) -> None:
"""
Await connection from QEMU.
@@ -76,7 +78,7 @@ def accept(self):
self._sock, _ = self._sock.accept()
self._sockfile = self._sock.makefile(mode='r')
- def cmd(self, qtest_cmd):
+ def cmd(self, qtest_cmd: str) -> str:
"""
Send a qtest command on the wire.
@@ -87,14 +89,16 @@ def cmd(self, qtest_cmd):
resp = self._sockfile.readline()
return resp
- def close(self):
- """Close this socket."""
+ def close(self) -> None:
+ """
+ Close this socket.
+ """
self._sock.close()
if self._sockfile:
self._sockfile.close()
self._sockfile = None
- def settimeout(self, timeout):
+ def settimeout(self, timeout: Optional[float]) -> None:
"""Set a timeout, in seconds."""
self._sock.settimeout(timeout)
@@ -118,7 +122,7 @@ def __init__(self,
super().__init__(binary, args, name=name, test_dir=test_dir,
socket_scm_helper=socket_scm_helper,
sock_dir=sock_dir)
- self._qtest = None
+ self._qtest: Optional[QEMUQtestProtocol] = None
self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
@property
@@ -130,7 +134,7 @@ def _base_args(self) -> List[str]:
])
return args
- def _pre_launch(self):
+ def _pre_launch(self) -> None:
super()._pre_launch()
self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
@@ -139,7 +143,7 @@ def _post_launch(self) -> None:
super()._post_launch()
self._qtest.accept()
- def _post_shutdown(self):
+ def _post_shutdown(self) -> None:
super()._post_shutdown()
self._remove_if_exists(self._qtest_path)
--
2.21.3
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (15 preceding siblings ...)
2020-06-04 20:22 ` [PATCH v3 16/16] python/qemu: Add mypy type annotations John Snow
@ 2020-06-05 9:26 ` Kevin Wolf
2020-06-08 15:19 ` John Snow
2020-06-17 17:18 ` [PATCH v3 00/16] python: add mypy support to python/qemu Philippe Mathieu-Daudé
17 siblings, 1 reply; 34+ messages in thread
From: Kevin Wolf @ 2020-06-05 9:26 UTC (permalink / raw)
To: John Snow
Cc: Eduardo Habkost, qemu-block, qemu-devel, Max Reitz, Cleber Rosa,
philmd
Am 04.06.2020 um 22:22 hat John Snow geschrieben:
> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>
> This series is extracted from my larger series that attempted to bundle
> our python module as an installable module. These fixes don't require that,
> so they are being sent first as I think there's less up for debate in here.
>
> This requires my "refactor shutdown" patch as a pre-requisite.
You didn't like my previous R-b? Here's a new one. :-)
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-05 9:26 ` [PATCH v3 00/16] python: add mypy support to python/qemu Kevin Wolf
@ 2020-06-08 15:19 ` John Snow
2020-06-08 15:33 ` Kevin Wolf
0 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-06-08 15:19 UTC (permalink / raw)
To: Kevin Wolf
Cc: Eduardo Habkost, qemu-block, qemu-devel, Max Reitz, Cleber Rosa,
philmd
On 6/5/20 5:26 AM, Kevin Wolf wrote:
> Am 04.06.2020 um 22:22 hat John Snow geschrieben:
>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>
>> This series is extracted from my larger series that attempted to bundle
>> our python module as an installable module. These fixes don't require that,
>> so they are being sent first as I think there's less up for debate in here.
>>
>> This requires my "refactor shutdown" patch as a pre-requisite.
>
> You didn't like my previous R-b? Here's a new one. :-)
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
I felt like I should address the feedback, and though I could have
applied the R-B to patches I didn't change, it was ... faster to just
re-send it.
Serious question: How do you apply people's R-Bs to your patches? At the
moment, it's pretty manually intensive for me. I use stgit and I pop all
of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
stg edit` and copy-paste the R-B into it.
It's easier when I'm just pulling other people's patches from the ML,
the patches tool handles it for me. It's updating my own patches that's
a drag and prone to error.
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-08 15:19 ` John Snow
@ 2020-06-08 15:33 ` Kevin Wolf
2020-06-08 17:41 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 34+ messages in thread
From: Kevin Wolf @ 2020-06-08 15:33 UTC (permalink / raw)
To: John Snow
Cc: Eduardo Habkost, qemu-block, qemu-devel, Max Reitz, Cleber Rosa,
philmd
Am 08.06.2020 um 17:19 hat John Snow geschrieben:
>
>
> On 6/5/20 5:26 AM, Kevin Wolf wrote:
> > Am 04.06.2020 um 22:22 hat John Snow geschrieben:
> >> Based-on: 20200604195252.20739-1-jsnow@redhat.com
> >>
> >> This series is extracted from my larger series that attempted to bundle
> >> our python module as an installable module. These fixes don't require that,
> >> so they are being sent first as I think there's less up for debate in here.
> >>
> >> This requires my "refactor shutdown" patch as a pre-requisite.
> >
> > You didn't like my previous R-b? Here's a new one. :-)
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
>
> I felt like I should address the feedback, and though I could have
> applied the R-B to patches I didn't change, it was ... faster to just
> re-send it.
>
> Serious question: How do you apply people's R-Bs to your patches? At the
> moment, it's pretty manually intensive for me. I use stgit and I pop all
> of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
> stg edit` and copy-paste the R-B into it.
>
> It's easier when I'm just pulling other people's patches from the ML,
> the patches tool handles it for me. It's updating my own patches that's
> a drag and prone to error.
It's a manual process for me, too. Just that I don't use stgit, but
'git rebase -i'.
Kevin
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-08 15:33 ` Kevin Wolf
@ 2020-06-08 17:41 ` Philippe Mathieu-Daudé
2020-06-09 8:58 ` Markus Armbruster
0 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-08 17:41 UTC (permalink / raw)
To: Kevin Wolf, John Snow
Cc: Eduardo Habkost, Cleber Rosa, qemu-devel, qemu-block, Max Reitz
On 6/8/20 5:33 PM, Kevin Wolf wrote:
> Am 08.06.2020 um 17:19 hat John Snow geschrieben:
>>
>>
>> On 6/5/20 5:26 AM, Kevin Wolf wrote:
>>> Am 04.06.2020 um 22:22 hat John Snow geschrieben:
>>>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>>>
>>>> This series is extracted from my larger series that attempted to bundle
>>>> our python module as an installable module. These fixes don't require that,
>>>> so they are being sent first as I think there's less up for debate in here.
>>>>
>>>> This requires my "refactor shutdown" patch as a pre-requisite.
>>>
>>> You didn't like my previous R-b? Here's a new one. :-)
>>>
>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>
>>
>> I felt like I should address the feedback, and though I could have
>> applied the R-B to patches I didn't change, it was ... faster to just
>> re-send it.
>>
>> Serious question: How do you apply people's R-Bs to your patches? At the
>> moment, it's pretty manually intensive for me. I use stgit and I pop all
>> of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
>> stg edit` and copy-paste the R-B into it.
wget https://patchew.org/QEMU/${MSG_ID}/mbox
git am mbox
Where ${MSG_ID} is the Message-Id of the series cover letter.
>>
>> It's easier when I'm just pulling other people's patches from the ML,
>> the patches tool handles it for me. It's updating my own patches that's
>> a drag and prone to error.
>
> It's a manual process for me, too. Just that I don't use stgit, but
> 'git rebase -i'.
>
> Kevin
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-08 17:41 ` Philippe Mathieu-Daudé
@ 2020-06-09 8:58 ` Markus Armbruster
2020-06-16 17:58 ` applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu) John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2020-06-09 8:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, qemu-devel, Max Reitz,
Cleber Rosa, John Snow
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 6/8/20 5:33 PM, Kevin Wolf wrote:
>> Am 08.06.2020 um 17:19 hat John Snow geschrieben:
>>>
>>>
>>> On 6/5/20 5:26 AM, Kevin Wolf wrote:
>>>> Am 04.06.2020 um 22:22 hat John Snow geschrieben:
>>>>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>>>>
>>>>> This series is extracted from my larger series that attempted to bundle
>>>>> our python module as an installable module. These fixes don't require that,
>>>>> so they are being sent first as I think there's less up for debate in here.
>>>>>
>>>>> This requires my "refactor shutdown" patch as a pre-requisite.
>>>>
>>>> You didn't like my previous R-b? Here's a new one. :-)
>>>>
>>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>>
>>>
>>> I felt like I should address the feedback, and though I could have
>>> applied the R-B to patches I didn't change, it was ... faster to just
>>> re-send it.
>>>
>>> Serious question: How do you apply people's R-Bs to your patches? At the
>>> moment, it's pretty manually intensive for me. I use stgit and I pop all
>>> of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
>>> stg edit` and copy-paste the R-B into it.
>
> wget https://patchew.org/QEMU/${MSG_ID}/mbox
> git am mbox
>
> Where ${MSG_ID} is the Message-Id of the series cover letter.
Patchew's awesomeness is still under-appreciated.
^ permalink raw reply [flat|nested] 34+ messages in thread* applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu)
2020-06-09 8:58 ` Markus Armbruster
@ 2020-06-16 17:58 ` John Snow
2020-06-17 3:33 ` Philippe Mathieu-Daudé
2020-06-17 15:04 ` applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu) Paolo Bonzini
0 siblings, 2 replies; 34+ messages in thread
From: John Snow @ 2020-06-16 17:58 UTC (permalink / raw)
To: Markus Armbruster, Philippe Mathieu-Daudé
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, qemu-devel, Max Reitz,
Cleber Rosa
On 6/9/20 4:58 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 6/8/20 5:33 PM, Kevin Wolf wrote:
>>> Am 08.06.2020 um 17:19 hat John Snow geschrieben:
>>>>
>>>>
>>>> On 6/5/20 5:26 AM, Kevin Wolf wrote:
>>>>> Am 04.06.2020 um 22:22 hat John Snow geschrieben:
>>>>>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>>>>>
>>>>>> This series is extracted from my larger series that attempted to bundle
>>>>>> our python module as an installable module. These fixes don't require that,
>>>>>> so they are being sent first as I think there's less up for debate in here.
>>>>>>
>>>>>> This requires my "refactor shutdown" patch as a pre-requisite.
>>>>>
>>>>> You didn't like my previous R-b? Here's a new one. :-)
>>>>>
>>>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>>>
>>>>
>>>> I felt like I should address the feedback, and though I could have
>>>> applied the R-B to patches I didn't change, it was ... faster to just
>>>> re-send it.
>>>>
>>>> Serious question: How do you apply people's R-Bs to your patches? At the
>>>> moment, it's pretty manually intensive for me. I use stgit and I pop all
>>>> of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
>>>> stg edit` and copy-paste the R-B into it.
>>
>> wget https://patchew.org/QEMU/${MSG_ID}/mbox
>> git am mbox
>>
>> Where ${MSG_ID} is the Message-Id of the series cover letter.
>
> Patchew's awesomeness is still under-appreciated.
>
Not for lack of appreciating patchew, but the problem with this workflow
is if I have already made modifications to my patches locally, I can't
use this to apply tags from upstream.
It looks like I will continue to do this manually for the time being;
but scripting the ability to "merge tags" from the list would be a cool
trick.
I'm not sure how to do it with git, though. Let's say I've got 16
patches and I've made modifications to some, but not all; so I have a
branch with 16 patches ahead of origin/master.
Does anyone have any cool tricks for being able to script:
1. Correlating a mailing list patch from e.g. patchew to a commit in my
history, even if it's changed a little bit?
(git-backport-diff uses patch names, that might be sufficient... Could
use that as a starting point, at least.)
2. Obtaining the commit message of that patch?
`git show -s --format=%B $SHA` ought to do it...
3. Editing that commit message? This I'm not sure about. I'd need to
understand the tags on the upstream and downstream versions, merge them,
and then re-write the message. Some magic with `git rebase -i` ?
--js
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu)
2020-06-16 17:58 ` applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu) John Snow
@ 2020-06-17 3:33 ` Philippe Mathieu-Daudé
2020-06-17 15:25 ` [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
2020-06-17 15:04 ` applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu) Paolo Bonzini
1 sibling, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-17 3:33 UTC (permalink / raw)
To: John Snow, Markus Armbruster
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, qemu-devel, Max Reitz,
Cleber Rosa
On 6/16/20 7:58 PM, John Snow wrote:
>
>
> On 6/9/20 4:58 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 6/8/20 5:33 PM, Kevin Wolf wrote:
>>>> Am 08.06.2020 um 17:19 hat John Snow geschrieben:
>>>>>
>>>>>
>>>>> On 6/5/20 5:26 AM, Kevin Wolf wrote:
>>>>>> Am 04.06.2020 um 22:22 hat John Snow geschrieben:
>>>>>>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>>>>>>
>>>>>>> This series is extracted from my larger series that attempted to bundle
>>>>>>> our python module as an installable module. These fixes don't require that,
>>>>>>> so they are being sent first as I think there's less up for debate in here.
>>>>>>>
>>>>>>> This requires my "refactor shutdown" patch as a pre-requisite.
>>>>>>
>>>>>> You didn't like my previous R-b? Here's a new one. :-)
>>>>>>
>>>>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>>>>
>>>>>
>>>>> I felt like I should address the feedback, and though I could have
>>>>> applied the R-B to patches I didn't change, it was ... faster to just
>>>>> re-send it.
>>>>>
>>>>> Serious question: How do you apply people's R-Bs to your patches? At the
>>>>> moment, it's pretty manually intensive for me. I use stgit and I pop all
>>>>> of the patches off (stg pop -n 100), and then one-at-a-time I `stg push;
>>>>> stg edit` and copy-paste the R-B into it.
>>>
>>> wget https://patchew.org/QEMU/${MSG_ID}/mbox
>>> git am mbox
>>>
>>> Where ${MSG_ID} is the Message-Id of the series cover letter.
>>
>> Patchew's awesomeness is still under-appreciated.
>>
>
> Not for lack of appreciating patchew, but the problem with this workflow
> is if I have already made modifications to my patches locally, I can't
> use this to apply tags from upstream.
Does that mean you want to respin this series?
Else you can consider it applied on python-next.
>
> It looks like I will continue to do this manually for the time being;
> but scripting the ability to "merge tags" from the list would be a cool
> trick.
>
> I'm not sure how to do it with git, though. Let's say I've got 16
> patches and I've made modifications to some, but not all; so I have a
> branch with 16 patches ahead of origin/master.
>
> Does anyone have any cool tricks for being able to script:
>
> 1. Correlating a mailing list patch from e.g. patchew to a commit in my
> history, even if it's changed a little bit?
>
> (git-backport-diff uses patch names, that might be sufficient... Could
> use that as a starting point, at least.)
>
> 2. Obtaining the commit message of that patch?
> `git show -s --format=%B $SHA` ought to do it...
>
> 3. Editing that commit message? This I'm not sure about. I'd need to
> understand the tags on the upstream and downstream versions, merge them,
> and then re-write the message. Some magic with `git rebase -i` ?
>
> --js
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-17 3:33 ` Philippe Mathieu-Daudé
@ 2020-06-17 15:25 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-17 15:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Markus Armbruster
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, qemu-devel, Max Reitz,
Cleber Rosa
On 6/16/20 11:33 PM, Philippe Mathieu-Daudé wrote:
> Does that mean you want to respin this series?
> Else you can consider it applied on python-next.
No no - you're all set. Just taking the opportunity to discuss tooling
that I find cumbersome at the moment.
Thank you Philippe :)
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu)
2020-06-16 17:58 ` applying mailing list review tags (was: Re: [PATCH v3 00/16] python: add mypy support to python/qemu) John Snow
2020-06-17 3:33 ` Philippe Mathieu-Daudé
@ 2020-06-17 15:04 ` Paolo Bonzini
1 sibling, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2020-06-17 15:04 UTC (permalink / raw)
To: John Snow, Markus Armbruster, Philippe Mathieu-Daudé
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, qemu-devel, Max Reitz,
Cleber Rosa
On 16/06/20 19:58, John Snow wrote:
> 1. Correlating a mailing list patch from e.g. patchew to a commit in my
> history, even if it's changed a little bit?
Use "git am --message-id"?
> (git-backport-diff uses patch names, that might be sufficient... Could
> use that as a starting point, at least.)
>
> 2. Obtaining the commit message of that patch?
> `git show -s --format=%B $SHA` ought to do it...
>
> 3. Editing that commit message? This I'm not sure about. I'd need to
> understand the tags on the upstream and downstream versions, merge them,
> and then re-write the message. Some magic with `git rebase -i` ?
"patchew apply" actually uses "git filter-branch --msg-filter" to add the
tags after a successful "git am", so you can do something similar yourself.
(Actually I have pending patches to patchew that switch it to server-side
application of tags using the "mbox" URL that Philippe mentioned earlier, but
they've been pending for quite some time now).
To get the upstream tags you can use the Patchew REST API:
$ MSGID=20200521153616.307100-1-stefanha@redhat.com
$ curl -L https://patchew.org/api/v1/projects/by-name/QEMU/messages/$MSGID/ | jq -r '.tags[]'
Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
So you'd have to take a commit, look for a Message-Id header, fetch the
tags from above mentioned Patchew API URL and add the tags to the commit
message.
The commit message can be either emitted to stdout (and the script
used with "git filter-branch)" or, for the faint of heart, the script
could do a "git commit --amend" and you can use "git rebase -i --exec"
to execute the script on all commits in a range.
This script is for the latter option:
#! /bin/bash
BODY=$(git show -s --format=%B)
MSGID=$(git interpret-trailers --parse <<< $BODY | sed -n 's/^Message-Id: <\(.*\)>/\1/ip')
USER="$(git config user.name) <$(git config user.email)>"
BODY=$(curl -L https://patchew.org/api/v1/projects/by-name/QEMU/messages/$MSGID/ | \
jq -r '.tags[]' | ( \
args=()
while read x; do
args+=(--trailer "$x")
done
git interpret-trailers \
--if-exists doNothing "${args[@]}" \
--if-exists replace --if-missing doNothing --trailer "Signed-off-by: $USER" <<< $BODY
))
git commit --amend -m"$BODY"
The script will also move your Signed-off-by line at the end of the commit
message, this might be a problem if there is more than one such line and
you want to keep them all.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-04 20:22 [PATCH v3 00/16] python: add mypy support to python/qemu John Snow
` (16 preceding siblings ...)
2020-06-05 9:26 ` [PATCH v3 00/16] python: add mypy support to python/qemu Kevin Wolf
@ 2020-06-17 17:18 ` Philippe Mathieu-Daudé
2020-06-17 17:18 ` John Snow
17 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-17 17:18 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 6/4/20 10:22 PM, John Snow wrote:
> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>
> This series is extracted from my larger series that attempted to bundle
> our python module as an installable module. These fixes don't require that,
> so they are being sent first as I think there's less up for debate in here.
>
[...]
>
> John Snow (16):
> 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/machine.py: Fix monitor address typing
> python/machine.py: reorder __init__
> python/machine.py: Don't modify state in _base_args()
> python/machine.py: Handle None events in events_wait
> python/machine.py: use qmp.command
> python/machine.py: Add _qmp access shim
> python/machine.py: fix _popen access
> python/qemu: make 'args' style arguments immutable
> iotests.py: Adjust HMP kwargs typing
> python/qemu: Add mypy type annotations
>
> python/qemu/accel.py | 8 +-
> python/qemu/machine.py | 286 ++++++++++++++++++++--------------
> python/qemu/qmp.py | 111 +++++++++----
> python/qemu/qtest.py | 53 ++++---
> scripts/render_block_graph.py | 7 +-
> tests/qemu-iotests/iotests.py | 11 +-
> 6 files changed, 298 insertions(+), 178 deletions(-)
>
Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/16] python: add mypy support to python/qemu
2020-06-17 17:18 ` [PATCH v3 00/16] python: add mypy support to python/qemu Philippe Mathieu-Daudé
@ 2020-06-17 17:18 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-06-17 17:18 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 6/17/20 1:18 PM, Philippe Mathieu-Daudé wrote:
> On 6/4/20 10:22 PM, John Snow wrote:
>> Based-on: 20200604195252.20739-1-jsnow@redhat.com
>>
>> This series is extracted from my larger series that attempted to bundle
>> our python module as an installable module. These fixes don't require that,
>> so they are being sent first as I think there's less up for debate in here.
>>
> [...]
>>
>> John Snow (16):
>> 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/machine.py: Fix monitor address typing
>> python/machine.py: reorder __init__
>> python/machine.py: Don't modify state in _base_args()
>> python/machine.py: Handle None events in events_wait
>> python/machine.py: use qmp.command
>> python/machine.py: Add _qmp access shim
>> python/machine.py: fix _popen access
>> python/qemu: make 'args' style arguments immutable
>> iotests.py: Adjust HMP kwargs typing
>> python/qemu: Add mypy type annotations
>>
>> python/qemu/accel.py | 8 +-
>> python/qemu/machine.py | 286 ++++++++++++++++++++--------------
>> python/qemu/qmp.py | 111 +++++++++----
>> python/qemu/qtest.py | 53 ++++---
>> scripts/render_block_graph.py | 7 +-
>> tests/qemu-iotests/iotests.py | 11 +-
>> 6 files changed, 298 insertions(+), 178 deletions(-)
>>
>
> Thanks, applied to my python-next tree:
> https://gitlab.com/philmd/qemu/commits/python-next
>
Awesome, thanks!
^ permalink raw reply [flat|nested] 34+ messages in thread