* [PATCH 01/20] python/qemu: use isort to lay out imports
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
@ 2020-10-06 23:57 ` John Snow
2020-10-07 4:53 ` Philippe Mathieu-Daudé
2020-10-07 9:45 ` Kevin Wolf
2020-10-06 23:57 ` [PATCH 02/20] python/machine.py: Fix monitor address typing John Snow
` (19 subsequent siblings)
20 siblings, 2 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:57 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Borrowed from the QAPI cleanup series, use the same configuration to
standardize the way we write and sort imports.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/.isort.cfg | 7 +++++++
python/qemu/accel.py | 1 +
python/qemu/console_socket.py | 2 +-
python/qemu/machine.py | 8 ++++----
python/qemu/qmp.py | 10 +++++-----
python/qemu/qtest.py | 2 +-
6 files changed, 19 insertions(+), 11 deletions(-)
create mode 100644 python/qemu/.isort.cfg
diff --git a/python/qemu/.isort.cfg b/python/qemu/.isort.cfg
new file mode 100644
index 00000000000..6d0fd6cc0d3
--- /dev/null
+++ b/python/qemu/.isort.cfg
@@ -0,0 +1,7 @@
+[settings]
+force_grid_wrap=4
+force_sort_within_sections=True
+include_trailing_comma=True
+line_length=72
+lines_after_imports=2
+multi_line_output=3
\ No newline at end of file
diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 7fabe629208..3ec6bdcfdb5 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -18,6 +18,7 @@
import os
import subprocess
+
LOG = logging.getLogger(__name__)
# Mapping host architecture to any additional architectures it can
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 70869fbbdc4..69f604c77fe 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -13,9 +13,9 @@
# the COPYING file in the top-level directory.
#
+from collections import deque
import socket
import threading
-from collections import deque
import time
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 82f3731fc3f..bc83f399c1b 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -20,15 +20,15 @@
import errno
import logging
import os
-import subprocess
import shutil
import signal
+import subprocess
import tempfile
-from typing import Optional, Type
from types import TracebackType
-from . import console_socket
+from typing import Optional, Type
+
+from . import console_socket, qmp
-from . import qmp
LOG = logging.getLogger(__name__)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 7935dababbf..ddf8347ac12 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -7,21 +7,21 @@
# This work is licensed under the terms of the GNU GPL, version 2. See
# the COPYING file in the top-level directory.
-import json
import errno
-import socket
+import json
import logging
+import socket
+from types import TracebackType
from typing import (
Any,
- cast,
Dict,
Optional,
TextIO,
- Type,
Tuple,
+ Type,
Union,
+ cast,
)
-from types import TracebackType
# QMPMessage is a QMP Message of any kind.
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 888c8bd2f60..7700c0b09b6 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -17,8 +17,8 @@
# Based on qmp.py.
#
-import socket
import os
+import socket
from typing import Optional, TextIO
from .machine import QEMUMachine
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 01/20] python/qemu: use isort to lay out imports
2020-10-06 23:57 ` [PATCH 01/20] python/qemu: use isort to lay out imports John Snow
@ 2020-10-07 4:53 ` Philippe Mathieu-Daudé
2020-10-07 9:45 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:53 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:57 AM, John Snow wrote:
> Borrowed from the QAPI cleanup series, use the same configuration to
> standardize the way we write and sort imports.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/.isort.cfg | 7 +++++++
> python/qemu/accel.py | 1 +
> python/qemu/console_socket.py | 2 +-
> python/qemu/machine.py | 8 ++++----
> python/qemu/qmp.py | 10 +++++-----
> python/qemu/qtest.py | 2 +-
> 6 files changed, 19 insertions(+), 11 deletions(-)
> create mode 100644 python/qemu/.isort.cfg
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/20] python/qemu: use isort to lay out imports
2020-10-06 23:57 ` [PATCH 01/20] python/qemu: use isort to lay out imports John Snow
2020-10-07 4:53 ` Philippe Mathieu-Daudé
@ 2020-10-07 9:45 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 9:45 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:57 hat John Snow geschrieben:
> Borrowed from the QAPI cleanup series, use the same configuration to
> standardize the way we write and sort imports.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 02/20] python/machine.py: Fix monitor address typing
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
2020-10-06 23:57 ` [PATCH 01/20] python/qemu: use isort to lay out imports John Snow
@ 2020-10-06 23:57 ` John Snow
2020-10-06 23:58 ` [PATCH 03/20] python/machine.py: reorder __init__ John Snow
` (18 subsequent siblings)
20 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:57 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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 simplify 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
python/qemu/machine.py | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index bc83f399c1b..3017ec072df 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -28,6 +28,7 @@
from typing import Optional, Type
from . import console_socket, qmp
+from .qmp import SocketAddrT
LOG = logging.getLogger(__name__)
@@ -68,7 +69,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,
drain_console=False, console_log=None):
'''
@@ -95,8 +97,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
@@ -241,15 +249,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):
@@ -274,14 +284,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 03/20] python/machine.py: reorder __init__
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
2020-10-06 23:57 ` [PATCH 01/20] python/qemu: use isort to lay out imports John Snow
2020-10-06 23:57 ` [PATCH 02/20] python/machine.py: Fix monitor address typing John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 4:53 ` Philippe Mathieu-Daudé
2020-10-07 9:43 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 04/20] python/machine.py: Don't modify state in _base_args() John Snow
` (17 subsequent siblings)
20 siblings, 2 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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>
---
python/qemu/machine.py | 44 ++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 19 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 3017ec072df..71fe58be050 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -84,42 +84,54 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
@param monitor_address: address for QMP monitor
@param socket_scm_helper: helper program, required for send_fd_scm()
@param sock_dir: where to create socket (overrides test_dir for sock)
- @param console_log: (optional) path to console log file
@param drain_console: (optional) True to drain console socket to buffer
+ @param console_log: (optional) path to console log file
@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
+
+ self._console_log_path = console_log
+ if self._console_log_path:
+ # In order to log the console, buffering needs to be enabled.
+ self._drain_console = True
+ else:
+ self._drain_console = drain_console
+
+ # 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
@@ -129,12 +141,6 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
self._console_socket = None
self._remove_files = []
self._user_killed = False
- self._console_log_path = console_log
- if self._console_log_path:
- # In order to log the console, buffering needs to be enabled.
- self._drain_console = True
- else:
- self._drain_console = drain_console
def __enter__(self):
return self
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 03/20] python/machine.py: reorder __init__
2020-10-06 23:58 ` [PATCH 03/20] python/machine.py: reorder __init__ John Snow
@ 2020-10-07 4:53 ` Philippe Mathieu-Daudé
2020-10-07 9:43 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:53 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:58 AM, John Snow wrote:
> 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>
> ---
> python/qemu/machine.py | 44 ++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 03/20] python/machine.py: reorder __init__
2020-10-06 23:58 ` [PATCH 03/20] python/machine.py: reorder __init__ John Snow
2020-10-07 4:53 ` Philippe Mathieu-Daudé
@ 2020-10-07 9:43 ` Kevin Wolf
2020-10-07 18:16 ` John Snow
1 sibling, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 9:43 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> 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>
> ---
> python/qemu/machine.py | 44 ++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 3017ec072df..71fe58be050 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -84,42 +84,54 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
> @param monitor_address: address for QMP monitor
> @param socket_scm_helper: helper program, required for send_fd_scm()
> @param sock_dir: where to create socket (overrides test_dir for sock)
> - @param console_log: (optional) path to console log file
> @param drain_console: (optional) True to drain console socket to buffer
> + @param console_log: (optional) path to console log file
> @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
Interesting that you use a shortcut with 'or' for name and sock_dir,
but you don't have 'wraper or []' and 'args or []' above.
It's not wrong, of course, but if you have to respin for another reason,
maybe an inconsistency to address.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 03/20] python/machine.py: reorder __init__
2020-10-07 9:43 ` Kevin Wolf
@ 2020-10-07 18:16 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 18:16 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 5:43 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> 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>
>> ---
>> python/qemu/machine.py | 44 ++++++++++++++++++++++++------------------
>> 1 file changed, 25 insertions(+), 19 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 3017ec072df..71fe58be050 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -84,42 +84,54 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>> @param monitor_address: address for QMP monitor
>> @param socket_scm_helper: helper program, required for send_fd_scm()
>> @param sock_dir: where to create socket (overrides test_dir for sock)
>> - @param console_log: (optional) path to console log file
>> @param drain_console: (optional) True to drain console socket to buffer
>> + @param console_log: (optional) path to console log file
>> @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
>
> Interesting that you use a shortcut with 'or' for name and sock_dir,
> but you don't have 'wraper or []' and 'args or []' above.
>
> It's not wrong, of course, but if you have to respin for another reason,
> maybe an inconsistency to address.
>
This winds up being because ... I delete those lines of code later. I
very often have this problem where I clean up a bunch of stuff, and then
split out that giant commit into a series.
Sometimes that causes stuff like this.
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Thanks!
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 04/20] python/machine.py: Don't modify state in _base_args()
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (2 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 03/20] python/machine.py: reorder __init__ John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-06 23:58 ` [PATCH 05/20] python/machine.py: Handle None events in events_wait John Snow
` (16 subsequent siblings)
20 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
python/qemu/machine.py | 17 ++++++++++-------
python/qemu/qtest.py | 7 ++++---
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 71fe58be050..812ca7d3497 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -25,7 +25,7 @@
import subprocess
import tempfile
from types import TracebackType
-from typing import Optional, Type
+from typing import List, Optional, Type
from . import console_socket, qmp
from .qmp import SocketAddrT
@@ -137,7 +137,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._user_killed = False
@@ -253,7 +255,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:
@@ -271,9 +274,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])
@@ -289,6 +289,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)
@@ -374,7 +377,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 7700c0b09b6..7fde2565a0a 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -19,7 +19,7 @@
import os
import socket
-from typing import Optional, TextIO
+from typing import List, Optional, TextIO
from .machine import QEMUMachine
@@ -111,8 +111,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 05/20] python/machine.py: Handle None events in events_wait
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (3 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 04/20] python/machine.py: Don't modify state in _base_args() John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-06 23:58 ` [PATCH 06/20] python/machine.py: use qmp.command John Snow
` (15 subsequent siblings)
20 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
If the timeout is 0, we can get None back. Handle this explicitly.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@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 812ca7d3497..aebfa09e9de 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -28,7 +28,7 @@
from typing import List, Optional, Type
from . import console_socket, qmp
-from .qmp import SocketAddrT
+from .qmp import QMPMessage, SocketAddrT
LOG = logging.getLogger(__name__)
@@ -604,13 +604,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:
@@ -618,6 +625,8 @@ def _match(event):
return True
return False
+ event: Optional[QMPMessage]
+
# Search cached events
for event in self._events:
if _match(event):
@@ -627,6 +636,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 06/20] python/machine.py: use qmp.command
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (4 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 05/20] python/machine.py: Handle None events in events_wait John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 4:54 ` Philippe Mathieu-Daudé
2020-10-06 23:58 ` [PATCH 07/20] python/machine.py: Add _qmp access shim John Snow
` (14 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
python/qemu/machine.py | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index aebfa09e9de..d788e8aba8c 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -25,7 +25,13 @@
import subprocess
import tempfile
from types import TracebackType
-from typing import List, Optional, Type
+from typing import (
+ Any,
+ Dict,
+ List,
+ Optional,
+ Type,
+)
from . import console_socket, qmp
from .qmp import QMPMessage, SocketAddrT
@@ -515,17 +521,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):
@@ -534,12 +546,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 06/20] python/machine.py: use qmp.command
2020-10-06 23:58 ` [PATCH 06/20] python/machine.py: use qmp.command John Snow
@ 2020-10-07 4:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:54 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:58 AM, John Snow wrote:
> 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>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> python/qemu/machine.py | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 07/20] python/machine.py: Add _qmp access shim
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (5 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 06/20] python/machine.py: use qmp.command John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 9:53 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 08/20] python/machine.py: fix _popen access John Snow
` (13 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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 | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index d788e8aba8c..3e9cf09fd2d 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -135,7 +135,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
@@ -302,14 +302,14 @@ 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
)
def _post_launch(self):
- if self._qmp:
+ if self._qmp_connection:
self._qmp.accept()
def _post_shutdown(self):
@@ -320,9 +320,9 @@ def _post_shutdown(self):
# Comprehensive reset for the failed launch case:
self._early_cleanup()
- if self._qmp:
+ if self._qmp_connection:
self._qmp.close()
- self._qmp = None
+ self._qmp_connection = None
self._load_io_log()
@@ -434,7 +434,7 @@ def _soft_shutdown(self, timeout: Optional[int],
"""
self._early_cleanup()
- if self._qmp is not None:
+ if self._qmp_connection:
if not has_quit:
# Might raise ConnectionReset
self._qmp.cmd('quit')
@@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True):
line. Default is True.
@note: call this function before launch().
"""
- if enabled:
- self._qmp_set = True
- else:
- self._qmp_set = False
- self._qmp = None
+ self._qmp_set = enabled
+
+ @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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 07/20] python/machine.py: Add _qmp access shim
2020-10-06 23:58 ` [PATCH 07/20] python/machine.py: Add _qmp access shim John Snow
@ 2020-10-07 9:53 ` Kevin Wolf
2020-10-07 18:21 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 9:53 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> 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>
> @@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True):
> line. Default is True.
> @note: call this function before launch().
> """
> - if enabled:
> - self._qmp_set = True
> - else:
> - self._qmp_set = False
> - self._qmp = None
> + self._qmp_set = enabled
This change seems unrelated to wrapping the connection in a property.
Intuitively, it makes sense that the connection of a running instance
doesn't go away just because I disable QMP in the command line for the
next launch.
If this is the reasoning behind the change, maybe mention it in the
commit message.
With this:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 07/20] python/machine.py: Add _qmp access shim
2020-10-07 9:53 ` Kevin Wolf
@ 2020-10-07 18:21 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 18:21 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 5:53 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> 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>
>
>> @@ -515,11 +515,13 @@ def set_qmp_monitor(self, enabled=True):
>> line. Default is True.
>> @note: call this function before launch().
>> """
>> - if enabled:
>> - self._qmp_set = True
>> - else:
>> - self._qmp_set = False
>> - self._qmp = None
>> + self._qmp_set = enabled
>
> This change seems unrelated to wrapping the connection in a property.
> Intuitively, it makes sense that the connection of a running instance
> doesn't go away just because I disable QMP in the command line for the
> next launch.
>
> If this is the reasoning behind the change, maybe mention it in the
> commit message.
>
> With this:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Oh, yes. That's what happened here -- and it got folded in here
specifically to make that access check consistent.
I'll update the commit message.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 08/20] python/machine.py: fix _popen access
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (6 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 07/20] python/machine.py: Add _qmp access shim John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 10:07 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 09/20] python/qemu: make 'args' style arguments immutable John Snow
` (12 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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 | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 3e9cf09fd2d..4e762fcd529 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -131,7 +131,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.
@@ -244,6 +244,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:
@@ -254,7 +260,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:
@@ -415,8 +421,8 @@ def _hard_shutdown(self) -> None:
waiting for the QEMU process to terminate.
"""
self._early_cleanup()
- self._popen.kill()
- self._popen.wait(timeout=60)
+ self._subp.kill()
+ self._subp.wait(timeout=60)
def _soft_shutdown(self, timeout: Optional[int],
has_quit: bool = False) -> None:
@@ -440,7 +446,7 @@ def _soft_shutdown(self, timeout: Optional[int],
self._qmp.cmd('quit')
# May raise subprocess.TimeoutExpired
- self._popen.wait(timeout=timeout)
+ self._subp.wait(timeout=timeout)
def _do_shutdown(self, timeout: Optional[int],
has_quit: bool = False) -> None:
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 08/20] python/machine.py: fix _popen access
2020-10-06 23:58 ` [PATCH 08/20] python/machine.py: fix _popen access John Snow
@ 2020-10-07 10:07 ` Kevin Wolf
2020-10-07 18:44 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 10:07 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> 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>
First a question about the preexisting state: I see that after
initialising self._popen once, we never reset it to None. Should we do
so on shutdown?
> python/qemu/machine.py | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 3e9cf09fd2d..4e762fcd529 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -131,7 +131,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
Another option that we have, especially if it's an attribute that is
never reset, would be to set the attribute only when it first gets a
value other than None. Accessing it while it hasn't been set yet
automatically results in an AttributeError. I don't think that's much
worse than the exception raised explicitly in a property wrapper.
In this case, you would only declare the type in __init__, but not
assign a value to it:
self._popen: Optional['subprocess.Popen[bytes]']
Maybe a nicer alternative in some cases than adding properties around
everything.
Instead of checking for None, you would then have to use hasattr(),
which is a bit uglier, so I guess it's mainly for attributes where you
can assume that you will always have a value (if the caller isn't buggy)
and therefore don't even have a check in most places.
> self._events = []
> self._iolog = None
> self._qmp_set = True # Enable QMP monitor by default.
> @@ -244,6 +244,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:
Of course, even if an alternative is possible, what you have is still
correct.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 08/20] python/machine.py: fix _popen access
2020-10-07 10:07 ` Kevin Wolf
@ 2020-10-07 18:44 ` John Snow
2020-10-08 7:04 ` Kevin Wolf
0 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-07 18:44 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 6:07 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> 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>
>
> First a question about the preexisting state: I see that after
> initialising self._popen once, we never reset it to None. Should we do
> so on shutdown?
>
Yup, we should.
>> python/qemu/machine.py | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 3e9cf09fd2d..4e762fcd529 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -131,7 +131,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
>
> Another option that we have, especially if it's an attribute that is
> never reset, would be to set the attribute only when it first gets a
> value other than None. Accessing it while it hasn't been set yet
> automatically results in an AttributeError. I don't think that's much
> worse than the exception raised explicitly in a property wrapper.
>
> In this case, you would only declare the type in __init__, but not
> assign a value to it:
>
> self._popen: Optional['subprocess.Popen[bytes]']
>
If you do this, you can just declare it as non-Optional. Whenever it
exists, it is definitely a subprocess.Popen[bytes].
> Maybe a nicer alternative in some cases than adding properties around
> everything.
>
> Instead of checking for None, you would then have to use hasattr(),
> which is a bit uglier, so I guess it's mainly for attributes where you
> can assume that you will always have a value (if the caller isn't buggy)
> and therefore don't even have a check in most places.
>
As long as the style checkers are OK with that sort of thing. After a
very quick test, it seems like they might be.
Generally, we run into trouble because pylint et al want variables to be
declared in __init__, but doing so requires Optional[T] most of the time
to allow something to be initialized later.
A lot of our stateful objects have this kind of pattern. QAPIGen has a
ton of it. machine.py has a ton of it too.
You can basically imply the stateful check by just foregoing the actual
initialization, which trades the explicit check for the implicit one
when you get the AttributeError.
This is maybe more convenient -- less code to write, certainly. The
error message you get I think is going to be a little worse, though.
I think I have been leaning towards the cute little @property shims
because it follows a familiar OO model where a specific class always has
a finite set of properties that does not grow or shrink. You can also
use the shim to give a meaningful error that might be nicer to read than
the AttributeError.
I'm open to suggestions on better patterns. I had considered at one
point that it might be nice to split Machine out into a version with and
without the console to make stronger typing guarantees. It has
implications for how shutdown and cleanup and so on is handled, too.
(I had some WIP patches to do this, but I think I got a little stuck
making the code pretty, and then the release, and then I got busy, and...)
>> self._events = []
>> self._iolog = None
>> self._qmp_set = True # Enable QMP monitor by default.
>> @@ -244,6 +244,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:
>
> Of course, even if an alternative is possible, what you have is still
> correct.
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Thanks; I'll continue with this for now, but I really am open to talking
about better ways to model the common pattern of "Optional sub-feature
for a class that can be engaged post-initialization".
It's an interesting typing problem. If we were using semantic types,
what we are describing is an f(x) such that:
f(object-without-feature) -> object-with-feature
It's a kind of semantic cast where we are doing something akin to an
in-place transformation of a base type to a subtype. I'm not sure I have
encountered any language that actually intentionally supports such a
paradigm.
(Maybe haskell? I just assume haskell can do everything if you learn to
lie to computers well enough.)
--js
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 08/20] python/machine.py: fix _popen access
2020-10-07 18:44 ` John Snow
@ 2020-10-08 7:04 ` Kevin Wolf
2020-10-08 15:29 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-08 7:04 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 20:44 hat John Snow geschrieben:
> On 10/7/20 6:07 AM, Kevin Wolf wrote:
> > Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> > > 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>
> >
> > First a question about the preexisting state: I see that after
> > initialising self._popen once, we never reset it to None. Should we do
> > so on shutdown?
> >
>
> Yup, we should.
>
> > > python/qemu/machine.py | 16 +++++++++++-----
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > > index 3e9cf09fd2d..4e762fcd529 100644
> > > --- a/python/qemu/machine.py
> > > +++ b/python/qemu/machine.py
> > > @@ -131,7 +131,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
> >
> > Another option that we have, especially if it's an attribute that is
> > never reset, would be to set the attribute only when it first gets a
> > value other than None. Accessing it while it hasn't been set yet
> > automatically results in an AttributeError. I don't think that's much
> > worse than the exception raised explicitly in a property wrapper.
> >
> > In this case, you would only declare the type in __init__, but not
> > assign a value to it:
> >
> > self._popen: Optional['subprocess.Popen[bytes]']
> >
>
> If you do this, you can just declare it as non-Optional. Whenever it exists,
> it is definitely a subprocess.Popen[bytes].
Sorry, yes, copied too much while thinking too little.
Getting rid of Optional was the whole point of the suggestion.
> > Maybe a nicer alternative in some cases than adding properties around
> > everything.
> >
> > Instead of checking for None, you would then have to use hasattr(),
> > which is a bit uglier, so I guess it's mainly for attributes where you
> > can assume that you will always have a value (if the caller isn't buggy)
> > and therefore don't even have a check in most places.
> >
>
> As long as the style checkers are OK with that sort of thing. After a very
> quick test, it seems like they might be.
>
> Generally, we run into trouble because pylint et al want variables to be
> declared in __init__, but doing so requires Optional[T] most of the time to
> allow something to be initialized later.
>
> A lot of our stateful objects have this kind of pattern. QAPIGen has a ton
> of it. machine.py has a ton of it too.
>
> You can basically imply the stateful check by just foregoing the actual
> initialization, which trades the explicit check for the implicit one when
> you get the AttributeError.
>
> This is maybe more convenient -- less code to write, certainly. The error
> message you get I think is going to be a little worse, though.
Whether this matters depends on the meaning of the individual attribute.
There can be attributes that can legitimately be None during most of
the lifetime of the object. These should clearly be Optional.
In many cases, however, the contract say that you must first call method
A that initialises the attribute and then you can call method B which
uses it. Calling B without A would be a bug, so it's not an error
message that users should ever see. For developers who will then look at
the stack trace anyway, I don't think it should make a big difference.
Here, it's usually expected that the attribute is not None except during
phases where the object is mostly inactive anyway (like VMs before
launch or after shutdown). Then you can just not add the attribute yet
and access it without checks (which would only throw an exception
anyway) elsewhere.
> I think I have been leaning towards the cute little @property shims because
> it follows a familiar OO model where a specific class always has a finite
> set of properties that does not grow or shrink. You can also use the shim to
> give a meaningful error that might be nicer to read than the AttributeError.
>
> I'm open to suggestions on better patterns. I had considered at one point
> that it might be nice to split Machine out into a version with and without
> the console to make stronger typing guarantees. It has implications for how
> shutdown and cleanup and so on is handled, too.
>
> (I had some WIP patches to do this, but I think I got a little stuck making
> the code pretty, and then the release, and then I got busy, and...)
I guess the way to have everything static would be splitting QEMUMachine
into QEMUVMConfig (which exists without a running QEMU instance) and
QEMUVMInstance (which gets a QEMUVMConfig passed to its constructor and
is directly tied to a QEMU process).
Not sure if it would be worth such a major change.
> > > self._events = []
> > > self._iolog = None
> > > self._qmp_set = True # Enable QMP monitor by default.
> > > @@ -244,6 +244,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
The major downside that I saw while reviewing this patch (besides having
extra code just for making the error message of what essentially a
failed assertion nicer) is that we have two names for the same thing, we
have both names in active use in the other methods, and I'll never be
able to remember which of _subp and _popen is the real attribute and
which is the property (or that they are related at all and changing one
will actually change the other, too) without looking it up.
I mean, I guess tools will tell me after getting it wrong, but still...
Properties can make a nice external interface, but I feel using them
internally while you don't avoid accessing the real attribute in methods
other than the property implementation is more confusing than helpful.
> > > def exitcode(self):
> > > """Returns the exit code if possible, or None."""
> > > if self._popen is None:
> >
> > Of course, even if an alternative is possible, what you have is still
> > correct.
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
>
> Thanks; I'll continue with this for now, but I really am open to talking
> about better ways to model the common pattern of "Optional sub-feature for a
> class that can be engaged post-initialization".
>
> It's an interesting typing problem. If we were using semantic types, what we
> are describing is an f(x) such that:
>
> f(object-without-feature) -> object-with-feature
>
> It's a kind of semantic cast where we are doing something akin to an
> in-place transformation of a base type to a subtype. I'm not sure I have
> encountered any language that actually intentionally supports such a
> paradigm.
>
> (Maybe haskell? I just assume haskell can do everything if you learn to lie
> to computers well enough.)
You can always express this kind of thing as object-with-feature
containing an object-without-feature.
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 08/20] python/machine.py: fix _popen access
2020-10-08 7:04 ` Kevin Wolf
@ 2020-10-08 15:29 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-08 15:29 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/8/20 3:04 AM, Kevin Wolf wrote:
> The major downside that I saw while reviewing this patch (besides having
> extra code just for making the error message of what essentially a
> failed assertion nicer) is that we have two names for the same thing, we
> have both names in active use in the other methods, and I'll never be
> able to remember which of _subp and _popen is the real attribute and
> which is the property (or that they are related at all and changing one
> will actually change the other, too) without looking it up.
>
> I mean, I guess tools will tell me after getting it wrong, but still...
>
> Properties can make a nice external interface, but I feel using them
> internally while you don't avoid accessing the real attribute in methods
> other than the property implementation is more confusing than helpful.
Good point. I'll see if I can find a nicer cleanup soon. For now I will
suggest relying on the type checker to spot if we get it wrong.
I do think the little property wrappers are kind of distracting, but
seemed like the quickest means to an end at the time. With type checking
fully in place, refactors can be a little more fearless going forward, I
think.
--js
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 09/20] python/qemu: make 'args' style arguments immutable
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (7 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 08/20] python/machine.py: fix _popen access John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 4:55 ` Philippe Mathieu-Daudé
2020-10-06 23:58 ` [PATCH 10/20] iotests.py: Adjust HMP kwargs typing John Snow
` (11 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
python/qemu/machine.py | 30 +++++++++++++++++-------------
python/qemu/qtest.py | 22 +++++++++++++++++-----
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 4e762fcd529..e599cb7439b 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 shutil
@@ -30,6 +31,8 @@
Dict,
List,
Optional,
+ Sequence,
+ Tuple,
Type,
)
@@ -74,8 +77,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,
drain_console=False, console_log=None):
@@ -97,14 +104,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()
@@ -136,7 +136,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
@@ -368,7 +368,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
@@ -388,8 +388,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 7fde2565a0a..675310d8dfe 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -19,7 +19,12 @@
import os
import socket
-from typing import List, Optional, TextIO
+from typing import (
+ List,
+ Optional,
+ Sequence,
+ TextIO,
+)
from .machine import QEMUMachine
@@ -99,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:
@@ -114,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 09/20] python/qemu: make 'args' style arguments immutable
2020-10-06 23:58 ` [PATCH 09/20] python/qemu: make 'args' style arguments immutable John Snow
@ 2020-10-07 4:55 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:55 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:58 AM, John Snow wrote:
> 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>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> python/qemu/machine.py | 30 +++++++++++++++++-------------
> python/qemu/qtest.py | 22 +++++++++++++++++-----
> 2 files changed, 34 insertions(+), 18 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 10/20] iotests.py: Adjust HMP kwargs typing
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (8 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 09/20] python/qemu: make 'args' style arguments immutable John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-06 23:58 ` [PATCH 11/20] python/qemu: Add mypy type annotations John Snow
` (10 subsequent siblings)
20 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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>
Reviewed-by: Kevin Wolf <kwolf@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 f212cec446d..63d2ace93c0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -605,7 +605,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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 11/20] python/qemu: Add mypy type annotations
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (9 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 10/20] iotests.py: Adjust HMP kwargs typing John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 10:46 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv() John Snow
` (9 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
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 | 98 ++++++++++++++++++++++++------------------
python/qemu/qmp.py | 44 +++++++++++--------
python/qemu/qtest.py | 26 ++++++-----
4 files changed, 101 insertions(+), 75 deletions(-)
diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 3ec6bdcfdb5..297933df2a3 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__)
@@ -30,7 +31,7 @@
}
-def list_accel(qemu_bin):
+def list_accel(qemu_bin: str) -> List[str]:
"""
List accelerators enabled in the QEMU binary.
@@ -50,7 +51,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;
@@ -73,7 +75,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 e599cb7439b..6420f01bed4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -23,11 +23,13 @@
import os
import shutil
import signal
+import socket
import subprocess
import tempfile
from types import TracebackType
from typing import (
Any,
+ BinaryIO,
Dict,
List,
Optional,
@@ -37,7 +39,7 @@
)
from . import console_socket, qmp
-from .qmp import QMPMessage, SocketAddrT
+from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
LOG = logging.getLogger(__name__)
@@ -67,7 +69,7 @@ class AbnormalShutdown(QEMUMachineError):
class QEMUMachine:
"""
- A QEMU VM
+ A QEMU VM.
Use this object as a context manager to ensure
the QEMU process terminates::
@@ -84,8 +86,10 @@ def __init__(self,
name: Optional[str] = None,
test_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper=None, sock_dir=None,
- drain_console=False, console_log=None):
+ socket_scm_helper: Optional[str] = None,
+ sock_dir: Optional[str] = None,
+ drain_console: bool = False,
+ console_log: Optional[str] = None):
'''
Initialize a QEMUMachine
@@ -129,28 +133,28 @@ def __init__(self,
self._drain_console = drain_console
# 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._user_killed = False
- def __enter__(self):
+ def __enter__(self) -> 'QEMUMachine':
return self
def __exit__(self,
@@ -159,14 +163,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
"""
@@ -185,7 +190,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.
@@ -229,7 +235,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
"""
@@ -240,7 +246,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
@@ -250,19 +256,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()
@@ -296,7 +302,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')
@@ -314,11 +320,11 @@ def _pre_launch(self):
nickname=self._name
)
- def _post_launch(self):
+ def _post_launch(self) -> None:
if self._qmp_connection:
self._qmp.accept()
- def _post_shutdown(self):
+ def _post_shutdown(self) -> None:
"""
Called to cleanup the VM instance after the process has exited.
May also be called after a failed launch.
@@ -358,7 +364,7 @@ def _post_shutdown(self):
self._user_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
@@ -382,7 +388,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
"""
@@ -501,7 +507,7 @@ def shutdown(self, has_quit: bool = False,
finally:
self._post_shutdown()
- def kill(self):
+ def kill(self) -> None:
"""
Terminate the VM forcefully, wait for it to exit, and perform cleanup.
"""
@@ -516,7 +522,7 @@ def wait(self, timeout: Optional[int] = 30) -> None:
"""
self.shutdown(has_quit=True, timeout=timeout)
- def set_qmp_monitor(self, enabled=True):
+ def set_qmp_monitor(self, enabled: bool = True) -> None:
"""
Set the QMP monitor.
@@ -552,7 +558,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.
@@ -561,7 +569,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
"""
@@ -569,7 +577,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
"""
@@ -580,7 +588,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.
@@ -610,9 +618,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.
@@ -622,7 +632,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
@@ -639,7 +651,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
@@ -666,20 +678,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
@@ -688,7 +700,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
@@ -719,7 +733,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 ddf8347ac12..9223307ed81 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -15,6 +15,7 @@
from typing import (
Any,
Dict,
+ List,
Optional,
TextIO,
Tuple,
@@ -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 675310d8dfe..39a0cf62fe9 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.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 11/20] python/qemu: Add mypy type annotations
2020-10-06 23:58 ` [PATCH 11/20] python/qemu: Add mypy type annotations John Snow
@ 2020-10-07 10:46 ` Kevin Wolf
2020-10-07 18:48 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 10:46 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> 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>
'mypy --strict qemu' doesn't have clean output after this patch because
ConsoleSocket isn't converted yet. But then, technically the commit
message doesn't make this claim, and you can indeed check the
self-consistency when you ignore the ConsoleSocket related errors, so
probably not a problem.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 11/20] python/qemu: Add mypy type annotations
2020-10-07 10:46 ` Kevin Wolf
@ 2020-10-07 18:48 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 18:48 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 6:46 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> 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>
>
> 'mypy --strict qemu' doesn't have clean output after this patch because
> ConsoleSocket isn't converted yet. But then, technically the commit
> message doesn't make this claim, and you can indeed check the
> self-consistency when you ignore the ConsoleSocket related errors, so
> probably not a problem.
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Whoops, old commit message.
I decided against folding in the new changes because they are newer and
have been through the review wringer a lot less.
I'll fix this commit message up.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv()
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (10 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 11/20] python/qemu: Add mypy type annotations John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 10:59 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 13/20] python/qemu/console_socket.py: fix typing of settimeout John Snow
` (8 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
The type and parameter names of recv() should match socket.socket().
OK, easy enough, but in the cases we don't pass straight through to the
real socket implementation, we probably can't accept such flags. OK, for
now, assert that we don't receive flags in such cases.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 69f604c77fe..cb3400a0385 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -92,13 +92,14 @@ def _drain_socket(self):
for c in string:
self._buffer.extend(c)
- def recv(self, bufsize=1):
+ def recv(self, bufsize: int = 1, flags: int = 0) -> bytes:
"""Return chars from in memory buffer.
Maintains the same API as socket.socket.recv.
"""
if self._drain_thread is None:
# Not buffering the socket, pass thru to socket.
- return socket.socket.recv(self, bufsize)
+ return socket.socket.recv(self, bufsize, flags)
+ assert not flags, "Cannot pass flags to recv() in drained mode"
start_time = time.time()
while len(self._buffer) < bufsize:
time.sleep(self._sleep_time)
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv()
2020-10-06 23:58 ` [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv() John Snow
@ 2020-10-07 10:59 ` Kevin Wolf
2020-10-07 18:49 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 10:59 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> The type and parameter names of recv() should match socket.socket().
Should this be socket.socket without parentheses (the class name)?
socket.socket() is the constructor and it takes very different
parameters.
> OK, easy enough, but in the cases we don't pass straight through to the
> real socket implementation, we probably can't accept such flags. OK, for
> now, assert that we don't receive flags in such cases.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv()
2020-10-07 10:59 ` Kevin Wolf
@ 2020-10-07 18:49 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 18:49 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 6:59 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> The type and parameter names of recv() should match socket.socket().
>
> Should this be socket.socket without parentheses (the class name)?
> socket.socket() is the constructor and it takes very different
> parameters.
>
You're right.
>> OK, easy enough, but in the cases we don't pass straight through to the
>> real socket implementation, we probably can't accept such flags. OK, for
>> now, assert that we don't receive flags in such cases.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Thanks!
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 13/20] python/qemu/console_socket.py: fix typing of settimeout
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (11 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 12/20] python/qemu/console_socket.py: Correct type of recv() John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 10:59 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 14/20] python/qemu/console_socket.py: Clarify type of drain_thread John Snow
` (7 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
The types and names of the parameters must match the socket.socket interface.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index cb3400a0385..39456825064 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -17,6 +17,7 @@
import socket
import threading
import time
+from typing import Optional
class ConsoleSocket(socket.socket):
@@ -31,6 +32,7 @@ class ConsoleSocket(socket.socket):
"""
def __init__(self, address, file=None, drain=False):
self._recv_timeout_sec = 300
+ self._recv_timeout_sec = 300.0
self._sleep_time = 0.5
self._buffer = deque()
socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
@@ -120,11 +122,11 @@ def setblocking(self, value):
if self._drain_thread is None:
socket.socket.setblocking(self, value)
- def settimeout(self, seconds):
+ def settimeout(self, value: Optional[float]) -> None:
"""When not draining we pass thru to the socket,
since when draining we control the timeout.
"""
- if seconds is not None:
- self._recv_timeout_sec = seconds
+ if value is not None:
+ self._recv_timeout_sec = value
if self._drain_thread is None:
- socket.socket.settimeout(self, seconds)
+ socket.socket.settimeout(self, value)
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 14/20] python/qemu/console_socket.py: Clarify type of drain_thread
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (12 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 13/20] python/qemu/console_socket.py: fix typing of settimeout John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 10:59 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 15/20] python/qemu/console_socket.py: Add type hint annotations John Snow
` (6 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Mypy needs just a little help to guess the type here.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 39456825064..d4669c441d0 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -41,10 +41,9 @@ def __init__(self, address, file=None, drain=False):
if file:
self._logfile = open(file, "w")
self._open = True
+ self._drain_thread = None
if drain:
self._drain_thread = self._thread_start()
- else:
- self._drain_thread = None
def _drain_fn(self):
"""Drains the socket and runs while the socket is open."""
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 15/20] python/qemu/console_socket.py: Add type hint annotations
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (13 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 14/20] python/qemu/console_socket.py: Clarify type of drain_thread John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 11:01 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 16/20] python/console_socket: avoid encoding to/from string John Snow
` (5 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Finish the typing of console_socket.py with annotations and no code
changes.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index d4669c441d0..57e6eee0176 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -17,7 +17,7 @@
import socket
import threading
import time
-from typing import Optional
+from typing import Deque, Optional
class ConsoleSocket(socket.socket):
@@ -30,11 +30,11 @@ class ConsoleSocket(socket.socket):
Optionally a file path can be passed in and we will also
dump the characters to this file for debugging purposes.
"""
- def __init__(self, address, file=None, drain=False):
- self._recv_timeout_sec = 300
+ def __init__(self, address: str, file: Optional[str] = None,
+ drain: bool = False):
self._recv_timeout_sec = 300.0
self._sleep_time = 0.5
- self._buffer = deque()
+ self._buffer: Deque[str] = deque()
socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
self.connect(address)
self._logfile = None
@@ -45,7 +45,7 @@ def __init__(self, address, file=None, drain=False):
if drain:
self._drain_thread = self._thread_start()
- def _drain_fn(self):
+ def _drain_fn(self) -> None:
"""Drains the socket and runs while the socket is open."""
while self._open:
try:
@@ -56,7 +56,7 @@ def _drain_fn(self):
# self._open is set to False.
time.sleep(self._sleep_time)
- def _thread_start(self):
+ def _thread_start(self) -> threading.Thread:
"""Kick off a thread to drain the socket."""
# Configure socket to not block and timeout.
# This allows our drain thread to not block
@@ -68,7 +68,7 @@ def _thread_start(self):
drain_thread.start()
return drain_thread
- def close(self):
+ def close(self) -> None:
"""Close the base object and wait for the thread to terminate"""
if self._open:
self._open = False
@@ -80,7 +80,7 @@ def close(self):
self._logfile.close()
self._logfile = None
- def _drain_socket(self):
+ def _drain_socket(self) -> None:
"""process arriving characters into in memory _buffer"""
data = socket.socket.recv(self, 1)
# latin1 is needed since there are some chars
@@ -114,7 +114,7 @@ def recv(self, bufsize: int = 1, flags: int = 0) -> bytes:
# socket w/o our intervention.
return chars.encode("latin1")
- def setblocking(self, value):
+ def setblocking(self, value: bool) -> None:
"""When not draining we pass thru to the socket,
since when draining we control socket blocking.
"""
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 16/20] python/console_socket: avoid encoding to/from string
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (14 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 15/20] python/qemu/console_socket.py: Add type hint annotations John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 11:10 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise John Snow
` (4 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
We can work directly in bytes instead of translating back and forth to
string, which removes the question of which encodings to use.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 57e6eee0176..f060d79e06d 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -34,12 +34,12 @@ def __init__(self, address: str, file: Optional[str] = None,
drain: bool = False):
self._recv_timeout_sec = 300.0
self._sleep_time = 0.5
- self._buffer: Deque[str] = deque()
+ self._buffer: Deque[int] = deque()
socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
self.connect(address)
self._logfile = None
if file:
- self._logfile = open(file, "w")
+ self._logfile = open(file, "bw")
self._open = True
self._drain_thread = None
if drain:
@@ -83,15 +83,10 @@ def close(self) -> None:
def _drain_socket(self) -> None:
"""process arriving characters into in memory _buffer"""
data = socket.socket.recv(self, 1)
- # latin1 is needed since there are some chars
- # we are receiving that cannot be encoded to utf-8
- # such as 0xe2, 0x80, 0xA6.
- string = data.decode("latin1")
if self._logfile:
- self._logfile.write("{}".format(string))
+ self._logfile.write(data)
self._logfile.flush()
- for c in string:
- self._buffer.extend(c)
+ self._buffer.extend(data)
def recv(self, bufsize: int = 1, flags: int = 0) -> bytes:
"""Return chars from in memory buffer.
@@ -107,12 +102,7 @@ def recv(self, bufsize: int = 1, flags: int = 0) -> bytes:
elapsed_sec = time.time() - start_time
if elapsed_sec > self._recv_timeout_sec:
raise socket.timeout
- chars = ''.join([self._buffer.popleft() for i in range(bufsize)])
- # We choose to use latin1 to remain consistent with
- # handle_read() and give back the same data as the user would
- # receive if they were reading directly from the
- # socket w/o our intervention.
- return chars.encode("latin1")
+ return bytes((self._buffer.popleft() for i in range(bufsize)))
def setblocking(self, value: bool) -> None:
"""When not draining we pass thru to the socket,
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (15 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 16/20] python/console_socket: avoid encoding to/from string John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 4:58 ` Philippe Mathieu-Daudé
2020-10-07 11:21 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered John Snow
` (3 subsequent siblings)
20 siblings, 2 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Use the "from ..." phrasing when re-raising errors to preserve their
initial context, to help aid debugging when things go wrong.
This also silences a pylint 2.6.0+ error.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/qmp.py | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 9223307ed81..d911999da1f 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -181,10 +181,11 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
self.__sock.settimeout(wait)
try:
ret = self.__json_read(only_event=True)
- except socket.timeout:
- raise QMPTimeoutError("Timeout waiting for event")
- except:
- raise QMPConnectError("Error while reading from socket")
+ except socket.timeout as err:
+ raise QMPTimeoutError("Timeout waiting for event") from err
+ except Exception as err:
+ msg = "Error while reading from socket"
+ raise QMPConnectError(msg) from err
if ret is None:
raise QMPConnectError("Error while reading from socket")
self.__sock.settimeout(None)
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise
2020-10-06 23:58 ` [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise John Snow
@ 2020-10-07 4:58 ` Philippe Mathieu-Daudé
2020-10-07 11:21 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:58 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:58 AM, John Snow wrote:
> Use the "from ..." phrasing when re-raising errors to preserve their
> initial context, to help aid debugging when things go wrong.
>
> This also silences a pylint 2.6.0+ error.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/qmp.py | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index 9223307ed81..d911999da1f 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -181,10 +181,11 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
> self.__sock.settimeout(wait)
> try:
> ret = self.__json_read(only_event=True)
> - except socket.timeout:
> - raise QMPTimeoutError("Timeout waiting for event")
> - except:
> - raise QMPConnectError("Error while reading from socket")
> + except socket.timeout as err:
> + raise QMPTimeoutError("Timeout waiting for event") from err
> + except Exception as err:
> + msg = "Error while reading from socket"
> + raise QMPConnectError(msg) from err
> if ret is None:
> raise QMPConnectError("Error while reading from socket")
> self.__sock.settimeout(None)
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise
2020-10-06 23:58 ` [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise John Snow
2020-10-07 4:58 ` Philippe Mathieu-Daudé
@ 2020-10-07 11:21 ` Kevin Wolf
2020-10-07 19:03 ` John Snow
1 sibling, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 11:21 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> Use the "from ..." phrasing when re-raising errors to preserve their
> initial context, to help aid debugging when things go wrong.
>
> This also silences a pylint 2.6.0+ error.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
I don't really understand what this improves compared to the implicit
chaining we got before, but if pylint says so...
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise
2020-10-07 11:21 ` Kevin Wolf
@ 2020-10-07 19:03 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 19:03 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 7:21 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> Use the "from ..." phrasing when re-raising errors to preserve their
>> initial context, to help aid debugging when things go wrong.
>>
>> This also silences a pylint 2.6.0+ error.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> I don't really understand what this improves compared to the implicit
> chaining we got before, but if pylint says so...
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Yeah, it's a pretty minimal change. Depending on the context, I think it
makes a bigger difference depending on how far away you are from the
error you are re-raising, but I couldn't find a great real-world example
for you right now.
In summary, it changes this line:
"During handling of the above exception, another exception occurred:"
to this one:
"The above exception was the direct cause of the following exception:"
Which disambiguates between wrapping an exception with a more
semantically meaningful exception class, vs. your handler code itself
faulted.
Minor change, I know. You are also allowed to use "from None" to
suppress the chain. I use this in the QAPI series at one point because I
felt the underlying error was not useful to see in the traceback.
I see the pylint change as forcing you not to rely on the implicit
chaining. Eh, fine.
--js
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (16 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 17/20] python/qemu/qmp.py: Preserve error context on re-raise John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 4:59 ` Philippe Mathieu-Daudé
2020-10-07 11:30 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 19/20] python/qemu/qmp.py: Straighten out exception hierarchy John Snow
` (2 subsequent siblings)
20 siblings, 2 replies; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Nested if conditions don't change when the exception block fires; we
need to explicitly re-raise the error if we didn't intend to capture and
suppress it.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/qmp.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index d911999da1f..bdbd1e9bdbb 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
try:
self.__json_read()
except OSError as err:
- if err.errno == errno.EAGAIN:
- # No data available
- pass
+ # EAGAIN: No data available; not critical
+ if err.errno != errno.EAGAIN:
+ raise
self.__sock.setblocking(True)
# Wait for new events, if needed.
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered
2020-10-06 23:58 ` [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered John Snow
@ 2020-10-07 4:59 ` Philippe Mathieu-Daudé
2020-10-07 11:30 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-07 4:59 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 10/7/20 1:58 AM, John Snow wrote:
> Nested if conditions don't change when the exception block fires; we
> need to explicitly re-raise the error if we didn't intend to capture and
> suppress it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/qmp.py | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index d911999da1f..bdbd1e9bdbb 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
> try:
> self.__json_read()
> except OSError as err:
> - if err.errno == errno.EAGAIN:
> - # No data available
> - pass
> + # EAGAIN: No data available; not critical
> + if err.errno != errno.EAGAIN:
> + raise
> self.__sock.setblocking(True)
>
> # Wait for new events, if needed.
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered
2020-10-06 23:58 ` [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered John Snow
2020-10-07 4:59 ` Philippe Mathieu-Daudé
@ 2020-10-07 11:30 ` Kevin Wolf
2020-10-07 19:17 ` John Snow
1 sibling, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 11:30 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> Nested if conditions don't change when the exception block fires; we
> need to explicitly re-raise the error if we didn't intend to capture and
> suppress it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/qmp.py | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index d911999da1f..bdbd1e9bdbb 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
> try:
> self.__json_read()
> except OSError as err:
> - if err.errno == errno.EAGAIN:
> - # No data available
> - pass
> + # EAGAIN: No data available; not critical
> + if err.errno != errno.EAGAIN:
> + raise
Hm, if we re-raise the exception here, the socket remains non-blocking.
I think we intended to have it like that only temporarily.
The same kind of exception would raise QMPConnectError below instead of
directly OSError. Should we try to make this consistent?
> self.__sock.setblocking(True)
>
> # Wait for new events, if needed.
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered
2020-10-07 11:30 ` Kevin Wolf
@ 2020-10-07 19:17 ` John Snow
2020-10-08 23:41 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-07 19:17 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 7:30 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> Nested if conditions don't change when the exception block fires; we
>> need to explicitly re-raise the error if we didn't intend to capture and
>> suppress it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> python/qemu/qmp.py | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
>> index d911999da1f..bdbd1e9bdbb 100644
>> --- a/python/qemu/qmp.py
>> +++ b/python/qemu/qmp.py
>> @@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
>> try:
>> self.__json_read()
>> except OSError as err:
>> - if err.errno == errno.EAGAIN:
>> - # No data available
>> - pass
>> + # EAGAIN: No data available; not critical
>> + if err.errno != errno.EAGAIN:
>> + raise
>
> Hm, if we re-raise the exception here, the socket remains non-blocking.
> I think we intended to have it like that only temporarily.
>
Whoops. Yep, no good to go from one kind of broken to a different kind
of broken.
> The same kind of exception would raise QMPConnectError below instead of
> directly OSError. Should we try to make this consistent?
>
Yeah, honestly I'm a bit confused about the error plumbing myself. We
like to return "None" a lot, and I have been trying to remove that
whenever possible, because the nature of what None can mean semantically
is ambiguous.
I need to sit down with this code and learn all of the different ways it
can actually and genuinely fail, and what each failure actually
semantically means.
I suspect it would probably be best to always catch socket errors and
wrap them in QMPConnectError just to be consistent about that.
I also need to revise the docstrings to be clear about what errors get
raised where, when, and why. I almost included that for this series, but
decided against it because I need to also adjust the docstring
formatting and so on -- and pending discussion in the qapi series --
felt it would be best to tackle that just a little bit later.
Here's a docstring convention question:
I think that any method that directly raises an exception should always
mention that with :raise X:. How far up the call chain, however, should
anticipated exceptions be documented with :raise:?
My gut feeling is that it should stop at the first public call boundary,
so accept() should repeat any :raise: comments that appear in private
helpers.
>> self.__sock.setblocking(True)
>>
>> # Wait for new events, if needed.
>
> Kevin
>
Thanks for the review! Things seem like they're looking good.
--js
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered
2020-10-07 19:17 ` John Snow
@ 2020-10-08 23:41 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-08 23:41 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 3:17 PM, John Snow wrote:
> On 10/7/20 7:30 AM, Kevin Wolf wrote:
>> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>>> Nested if conditions don't change when the exception block fires; we
>>> need to explicitly re-raise the error if we didn't intend to capture and
>>> suppress it.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> python/qemu/qmp.py | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
>>> index d911999da1f..bdbd1e9bdbb 100644
>>> --- a/python/qemu/qmp.py
>>> +++ b/python/qemu/qmp.py
>>> @@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] =
>>> False) -> None:
>>> try:
>>> self.__json_read()
>>> except OSError as err:
>>> - if err.errno == errno.EAGAIN:
>>> - # No data available
>>> - pass
>>> + # EAGAIN: No data available; not critical
>>> + if err.errno != errno.EAGAIN:
>>> + raise
>>
>> Hm, if we re-raise the exception here, the socket remains non-blocking.
>> I think we intended to have it like that only temporarily.
>>
>
> Whoops. Yep, no good to go from one kind of broken to a different kind
> of broken.
>
Actually, wanna know a funny story?
I think the reason this never broke anything is because sockfiles aren't
suppose to be used in nonblocking mode -- their behavior is not
documented in this case.
In practice, what actually seems to happen when you set non-blocking
mode and then call sockfile.readline() is that you get an empty string.
This means you get 'None' from __json_read, and so on.
Why doesn't this bite us more often? Well, we don't actually check the
return value when we're using this in non-blocking mode, so I suppose
that's the primary reason.
I don't know if the behavior of Python changed at some point, I can try
to patch this up for how Python *seems* to work, but we should probably
do a more meaningful fix to get away from undefined behavior sometime soon.
I had some async prototypes hanging around, maybe it's time to try and
give that a more solid effort ...
Related note:
Even in blocking mode, you might get an empty reply from readline which
means EOF and not just "try again."
You'll see this in the case where you already have QEMU running from a
previous failed test, and you start a new iotest up. When QMP calls
accept(), the QMP capabilities handshake fails because it gets "None"
from __json_read.
Confusing error for what's actually going on there. It's actually that
the socket is at EOF.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 19/20] python/qemu/qmp.py: Straighten out exception hierarchy
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (17 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 12:53 ` Kevin Wolf
2020-10-06 23:58 ` [PATCH 20/20] python: add mypy config John Snow
2020-10-08 15:29 ` [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Be a little more rigorous about which exception we use, and when.
Primarily, this makes QMPCapabilitiesError an extension of
QMPprotocolError.
The family of errors:
QMPError (generic base)
QMPConnectError (For connection issues)
QMPTimeoutError (when waiting for an event expires)
QMPProtocolError (unexpected/garbled responses)
QMPCapabilitiesError (handshake problems)
QMPResponseError (For in-band QMP error returns)
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/qmp.py | 45 ++++++++++++++++++++++++++++++---------------
1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index bdbd1e9bdbb..1349632c101 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -41,7 +41,7 @@
class QMPError(Exception):
"""
- QMP base exception
+ QMP base exception; all QMP errors derive from this.
"""
@@ -51,7 +51,13 @@ class QMPConnectError(QMPError):
"""
-class QMPCapabilitiesError(QMPError):
+class QMPProtocolError(QMPError):
+ """
+ QMP protocol error; unexpected response
+ """
+
+
+class QMPCapabilitiesError(QMPProtocolError):
"""
QMP negotiate capabilities exception
"""
@@ -63,15 +69,9 @@ class QMPTimeoutError(QMPError):
"""
-class QMPProtocolError(QMPError):
- """
- QMP protocol error; unexpected response
- """
-
-
class QMPResponseError(QMPError):
"""
- Represents erroneous QMP monitor reply
+ Represents in-band QMP replies indicating command failure
"""
def __init__(self, reply: QMPMessage):
try:
@@ -125,14 +125,23 @@ def __get_sock(self) -> socket.socket:
return socket.socket(family, socket.SOCK_STREAM)
def __negotiate_capabilities(self) -> QMPMessage:
+ """
+ @raise QMPConnectError on failure to receive the greeting.
+ @raise QMPCapabilitiesError on malformed greeting, or malformed
+ capabilities handshake response.
+ """
greeting = self.__json_read()
- if greeting is None or "QMP" not in greeting:
- raise QMPConnectError
+ if greeting is None:
+ raise QMPConnectError("Did not receive QMP greeting")
+ if 'QMP' not in greeting:
+ msg = f"Did not understand greeting: '{str(greeting)}'"
+ raise QMPCapabilitiesError(msg)
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
if resp and "return" in resp:
return greeting
- raise QMPCapabilitiesError
+ msg = "Did not understand capabilities reply"
+ raise QMPCapabilitiesError(f"{msg}: {str(resp)}")
def __json_read(self, only_event: bool = False) -> Optional[QMPMessage]:
assert self.__sockfile is not None
@@ -158,10 +167,12 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
+ @raise OSError: For backing socket connection errors
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
@raise QMPConnectError: If wait is True but no events could be
retrieved or if some other error occurred.
+ @raise QMPProtocolError: If a garbled message is received.
"""
# Check for new events regardless and pull them into the cache:
@@ -187,7 +198,7 @@ def __get_events(self, wait: Union[bool, float] = False) -> None:
msg = "Error while reading from socket"
raise QMPConnectError(msg) from err
if ret is None:
- raise QMPConnectError("Error while reading from socket")
+ raise QMPProtocolError("Unexpected empty message from server")
self.__sock.settimeout(None)
def __enter__(self) -> 'QEMUMonitorProtocol':
@@ -245,12 +256,13 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
@param qmp_cmd: QMP command to be sent as a Python dict
@return QMP response as a Python dict
+ @raise QMPProtocolError on unexpected empty messages.
"""
self.logger.debug(">>> %s", qmp_cmd)
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")
+ raise QMPProtocolError("Unexpected empty reply from server")
self.logger.debug("<<< %s", resp)
return resp
@@ -274,13 +286,16 @@ def cmd(self, name: str,
def command(self, cmd: str, **kwds: Any) -> QMPReturnValue:
"""
Build and send a QMP command to the monitor, report errors if any
+
+ @raise QMPResponseError if the server returns an in-band error reply.
+ @raise QMPProtocolError if the server reply is not understood.
"""
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))
+ f"'return' key not found in QMP response '{str(ret)}'"
)
return cast(QMPReturnValue, ret['return'])
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 19/20] python/qemu/qmp.py: Straighten out exception hierarchy
2020-10-06 23:58 ` [PATCH 19/20] python/qemu/qmp.py: Straighten out exception hierarchy John Snow
@ 2020-10-07 12:53 ` Kevin Wolf
0 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 12:53 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> Be a little more rigorous about which exception we use, and when.
> Primarily, this makes QMPCapabilitiesError an extension of
> QMPprotocolError.
>
> The family of errors:
>
> QMPError (generic base)
> QMPConnectError (For connection issues)
> QMPTimeoutError (when waiting for an event expires)
> QMPProtocolError (unexpected/garbled responses)
> QMPCapabilitiesError (handshake problems)
> QMPResponseError (For in-band QMP error returns)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 20/20] python: add mypy config
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (18 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 19/20] python/qemu/qmp.py: Straighten out exception hierarchy John Snow
@ 2020-10-06 23:58 ` John Snow
2020-10-07 11:35 ` Kevin Wolf
2020-10-08 15:29 ` [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
20 siblings, 1 reply; 53+ messages in thread
From: John Snow @ 2020-10-06 23:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Formalize the options used for checking the python library. You can run
mypy from the directory that mypy.ini is in by typing `mypy qemu/`.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/mypy.ini | 4 ++++
1 file changed, 4 insertions(+)
create mode 100644 python/mypy.ini
diff --git a/python/mypy.ini b/python/mypy.ini
new file mode 100644
index 00000000000..7a70eca47c6
--- /dev/null
+++ b/python/mypy.ini
@@ -0,0 +1,4 @@
+[mypy]
+strict = True
+python_version = 3.6
+warn_unused_configs = True
\ No newline at end of file
--
2.26.2
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 20/20] python: add mypy config
2020-10-06 23:58 ` [PATCH 20/20] python: add mypy config John Snow
@ 2020-10-07 11:35 ` Kevin Wolf
2020-10-07 19:08 ` John Snow
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-10-07 11:35 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> Formalize the options used for checking the python library. You can run
> mypy from the directory that mypy.ini is in by typing `mypy qemu/`.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/mypy.ini | 4 ++++
> 1 file changed, 4 insertions(+)
> create mode 100644 python/mypy.ini
>
> diff --git a/python/mypy.ini b/python/mypy.ini
> new file mode 100644
> index 00000000000..7a70eca47c6
> --- /dev/null
> +++ b/python/mypy.ini
> @@ -0,0 +1,4 @@
> +[mypy]
> +strict = True
$ mypy --strict qemu
mypy.ini: [mypy]: Strict mode is not supported in configuration files: specify individual flags instead (see 'mypy -h' for the list of flags enabled in strict mode)
Success: no issues found in 6 source files
$ mypy --version
mypy 0.740
Did this change in newer mypy versions? I guess it's time that I get the
new laptop which will involve installing a newer Fedora release. :-)
> +python_version = 3.6
> +warn_unused_configs = True
> \ No newline at end of file
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 20/20] python: add mypy config
2020-10-07 11:35 ` Kevin Wolf
@ 2020-10-07 19:08 ` John Snow
0 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-07 19:08 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Max Reitz, qemu-devel, Eduardo Habkost, Cleber Rosa
On 10/7/20 7:35 AM, Kevin Wolf wrote:
> Am 07.10.2020 um 01:58 hat John Snow geschrieben:
>> Formalize the options used for checking the python library. You can run
>> mypy from the directory that mypy.ini is in by typing `mypy qemu/`.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> python/mypy.ini | 4 ++++
>> 1 file changed, 4 insertions(+)
>> create mode 100644 python/mypy.ini
>>
>> diff --git a/python/mypy.ini b/python/mypy.ini
>> new file mode 100644
>> index 00000000000..7a70eca47c6
>> --- /dev/null
>> +++ b/python/mypy.ini
>> @@ -0,0 +1,4 @@
>> +[mypy]
>> +strict = True
>
> $ mypy --strict qemu
> mypy.ini: [mypy]: Strict mode is not supported in configuration files: specify individual flags instead (see 'mypy -h' for the list of flags enabled in strict mode)
> Success: no issues found in 6 source files
> $ mypy --version
> mypy 0.740
>
> Did this change in newer mypy versions? I guess it's time that I get the
> new laptop which will involve installing a newer Fedora release. :-)
>
>> +python_version = 3.6
>> +warn_unused_configs = True
>> \ No newline at end of file
>
> Kevin
>
0.770 lets you use strict in the config file. Fairly modern. I intend to
use this version in the CI venv that I am cooking up to check these, so
no need to hurry and update your fedora.
'pip3 install --user mypy>=0.770' should work out just fine until then.
Maybe I should drop back down to >=0.730, but I liked being able to
force the stricter options in the conf file directly. I also liked the
idea that if new strict options got added in the future, we'd acquire
them automatically.
I felt like anything we disabled should be a conscious and explicit
choice, instead of the opposite.
--js
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2
2020-10-06 23:57 [PATCH 00/20] python/qemu: strictly typed mypy conversion, pt2 John Snow
` (19 preceding siblings ...)
2020-10-06 23:58 ` [PATCH 20/20] python: add mypy config John Snow
@ 2020-10-08 15:29 ` John Snow
20 siblings, 0 replies; 53+ messages in thread
From: John Snow @ 2020-10-08 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Max Reitz, Eduardo Habkost, qemu-block, Cleber Rosa
On 10/6/20 7:57 PM, John Snow wrote:
> Continuing where I left off prior to the 5.1 release, this series
> touches up a few odds and ends and introduces mypy hints.
>
> What's new:
>
> - Using isort to solidify import order
> - Patches adding small corrections and typing for console_socket
> - A few error class changes for qmp.py
>
> Like my QAPI series, this requires:
>
> - pylint >= 2.6.0
> - flake8 >= 3.8.0
> - mypy >= 0.770
> - isort >= 4.3.0 (Presumably...)
>
> What this series doesn't do:
>
> - Create a proper python package
> - Establish a CI test to prevent regressions
> - Fix the docstring conventions in the library
>
> Those are coming soon! (and in the order mentioned.)
>
> Changes against the last version of this series that was sent prior to
> 5.1:
>
> 001/20:[down] 'python/qemu: use isort to lay out imports'
> 002/20:[0005] [FC] 'python/machine.py: Fix monitor address typing'
> 003/20:[0015] [FC] 'python/machine.py: reorder __init__'
> 004/20:[0009] [FC] 'python/machine.py: Don't modify state in _base_args()'
> 005/20:[0002] [FC] 'python/machine.py: Handle None events in events_wait'
> 006/20:[0006] [FC] 'python/machine.py: use qmp.command'
> 007/20:[----] [-C] 'python/machine.py: Add _qmp access shim'
> 008/20:[----] [-C] 'python/machine.py: fix _popen access'
> 009/20:[0006] [FC] 'python/qemu: make 'args' style arguments immutable'
> 010/20:[----] [--] 'iotests.py: Adjust HMP kwargs typing'
> 011/20:[0010] [FC] 'python/qemu: Add mypy type annotations'
> 012/20:[down] 'python/qemu/console_socket.py: Correct type of recv()'
> 013/20:[down] 'python/qemu/console_socket.py: fix typing of settimeout'
> 014/20:[down] 'python/qemu/console_socket.py: Clarify type of drain_thread'
> 015/20:[down] 'python/qemu/console_socket.py: Add type hint annotations'
> 016/20:[down] 'python/console_socket: avoid encoding to/from string'
> 017/20:[down] 'python/qemu/qmp.py: Preserve error context on re-raise'
> 018/20:[down] 'python/qemu/qmp.py: re-raise OSError when encountered'
> 019/20:[down] 'python/qemu/qmp.py: Straighten out exception hierarchy'
> 020/20:[down] 'python: add mypy config'
>
> 02: import order differences, context changes from console_socket.py
> 03: (minor) changes for console_socket, RB-s dropped just in case
> 04: import order differences
> 05: import order differences
> 06: import order differences
> 09: import order differences
> 11: import order differences, small changes for console_socket
>
> John Snow (20):
> python/qemu: use isort to lay out imports
> 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/console_socket.py: Correct type of recv()
> python/qemu/console_socket.py: fix typing of settimeout
> python/qemu/console_socket.py: Clarify type of drain_thread
> python/qemu/console_socket.py: Add type hint annotations
> python/console_socket: avoid encoding to/from string
> python/qemu/qmp.py: Preserve error context on re-raise
> python/qemu/qmp.py: re-raise OSError when encountered
> python/qemu/qmp.py: Straighten out exception hierarchy
> python: add mypy config
>
> python/mypy.ini | 4 +
> python/qemu/.isort.cfg | 7 +
> python/qemu/accel.py | 9 +-
> python/qemu/console_socket.py | 54 +++---
> python/qemu/machine.py | 306 +++++++++++++++++++++-------------
> python/qemu/qmp.py | 114 ++++++++-----
> python/qemu/qtest.py | 55 +++---
> tests/qemu-iotests/iotests.py | 2 +-
> 8 files changed, 331 insertions(+), 220 deletions(-)
> create mode 100644 python/mypy.ini
> create mode 100644 python/qemu/.isort.cfg
>
Thanks, I am staging patches 1-17 -- alongside the Python maintainers
patch I have been kicking around -- to my python branch.
Cleaning up the error classes in 18-19 is looking more fiddly, so I'm
spinning it out into a new small series for better review.
https://gitlab.com/jsnow/qemu.git python
https://gitlab.com/jsnow/qemu/-/tree/python
--js
^ permalink raw reply [flat|nested] 53+ messages in thread