[PULL 2/7] python/machine: remove _remove_monitor_sockfile property
It doesn't matter if it was the user or the class itself that specified
where the sockfile should be created; the fact is that if we are using a
sockfile here, we created it and we can clean it up.
Signed-off-by: John Snow
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-3-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index b1dd77b538..ea9e07805d 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -141,12 +141,10 @@ def __init__(self,
if monitor_address is not None:
self._monitor_address = monitor_address
-self._remove_monitor_sockfile = False
else:
self._monitor_address = os.path.join(
self.sock_dir, f"{self._name}-monitor.sock"
)
-self._remove_monitor_sockfile = True
self._console_log_path = console_log
if self._console_log_path:
@@ -315,8 +313,7 @@ def _pre_launch(self) -> None:
self._remove_files.append(self._console_address)
if self._qmp_set:
-if self._remove_monitor_sockfile:
-assert isinstance(self._monitor_address, str)
+if isinstance(self._monitor_address, str):
self._remove_files.append(self._monitor_address)
self._qmp_connection = QEMUMonitorProtocol(
self._monitor_address,
--
2.31.1
[PULL 7/7] python/aqmp: fix send_fd_scm for python 3.6.x
3.6 doesn't play keepaway with the socket object, so we don't need to go
fishing for it on this version. In fact, so long as 'sendmsg' is still
available, it's probably preferable to just use that method and only go
fishing for forbidden details when we absolutely have to.
Reported-by: Thomas Huth
Signed-off-by: John Snow
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-8-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/qmp_client.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index f987da02eb..8105e29fa8 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -639,9 +639,12 @@ def send_fd_scm(self, fd: int) -> None:
if sock.family != socket.AF_UNIX:
raise AQMPError("Sending file descriptors requires a UNIX socket.")
-# Void the warranty sticker.
-# Access to sendmsg in asyncio is scheduled for removal in Python 3.11.
-sock = sock._sock # pylint: disable=protected-access
+if not hasattr(sock, 'sendmsg'):
+# We need to void the warranty sticker.
+# Access to sendmsg is scheduled for removal in Python 3.11.
+# Find the real backing socket to use it anyway.
+sock = sock._sock # pylint: disable=protected-access
+
sock.sendmsg(
[b' '],
[(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
--
2.31.1
[PULL 6/7] scripts/device-crash-test: Use a QMP timeout
Despite all the previous fixes, it's still possible for
device-crash-test to wedge itself in the case that QEMU terminates *so
quickly* that it doesn't even begin a connection attempt to our QMP
client. Python will just joyfully wait ad infinitum for a connection
that will now never arrive.
The real fix is to use asyncio to simultaneously poll both the health of
the launched process AND the connection attempt. That's quite a bit more
invasive than just setting a connection timeout, though.
Do the very simplest thing for now.
Signed-off-by: John Snow
Message-id: 2028204620.1897674-7-js...@redhat.com
Signed-off-by: John Snow
---
scripts/device-crash-test | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 1c73dac93e..7fbd99158b 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -353,7 +353,7 @@ def checkOneCase(args, testcase):
'-device', qemuOptsEscape(device)]
cmdline = ' '.join([binary] + args)
dbg("will launch QEMU: %s", cmdline)
-vm = QEMUMachine(binary=binary, args=args)
+vm = QEMUMachine(binary=binary, args=args, qmp_timer=15)
exc = None
exc_traceback = None
--
2.31.1
[PULL 4/7] python/machine: move more variable initializations to _pre_launch
No need to clear them only to set them later.
Signed-off-by: John Snow
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-5-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ad529fd92a..f92e73de40 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -327,6 +327,14 @@ def _pre_launch(self) -> None:
self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
self._qemu_log_file = open(self._qemu_log_path, 'wb')
+self._iolog = None
+self._qemu_full_args = tuple(chain(
+self._wrapper,
+[self._binary],
+self._base_args,
+self._args
+))
+
def _post_launch(self) -> None:
if self._qmp_connection:
self._qmp.accept(self._qmp_timer)
@@ -390,8 +398,6 @@ def launch(self) -> None:
if self._launched:
raise QEMUMachineError('VM already launched')
-self._iolog = None
-self._qemu_full_args = ()
try:
self._launch()
self._launched = True
@@ -410,12 +416,6 @@ def _launch(self) -> None:
Launch the VM and establish a QMP connection
"""
self._pre_launch()
-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))
# Cleaning up of this subprocess is guaranteed by _do_shutdown.
--
2.31.1
[PULL 5/7] python/machine: handle "fast" QEMU terminations
In the case that the QEMU process actually launches -- but then dies so
quickly that we can't establish a QMP connection to it -- QEMUMachine
currently calls _post_shutdown() assuming that it never launched the VM
process.
This isn't true, though: it "merely" may have failed to establish a QMP
connection and the process is in the middle of its own exit path.
If we don't wait for the subprocess, the caller may get a bogus `None`
return for .exitcode(). This behavior was observed from
device-crash-test; after the switch to Async QMP, the timings were
changed such that it was now seemingly possible to witness the failure
of "vm.launch()" *prior* to the exitcode becoming available.
The semantic of the `_launched` property is changed in this
patch. Instead of representing the condition "launch() executed
successfully", it will now represent "has forked a child process
successfully". This way, wait() when called in the exit path won't
become a no-op.
Signed-off-by: John Snow
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-6-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index f92e73de40..67ab06ca2b 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -349,9 +349,6 @@ def _post_shutdown(self) -> None:
Called to cleanup the VM instance after the process has exited.
May also be called after a failed launch.
"""
-# Comprehensive reset for the failed launch case:
-self._early_cleanup()
-
try:
self._close_qmp_connection()
except Exception as err: # pylint: disable=broad-except
@@ -400,9 +397,16 @@ def launch(self) -> None:
try:
self._launch()
-self._launched = True
except:
-self._post_shutdown()
+# We may have launched the process but it may
+# have exited before we could connect via QMP.
+# Assume the VM didn't launch or is exiting.
+# If we don't wait for the process, exitcode() may still be
+# 'None' by the time control is ceded back to the caller.
+if self._launched:
+self.wait()
+else:
+self._post_shutdown()
LOG.debug('Error launching VM')
if self._qemu_full_args:
@@ -426,6 +430,7 @@ def _launch(self) -> None:
stderr=subprocess.STDOUT,
shell=False,
close_fds=False)
+self._launched = True
self._post_launch()
def _close_qmp_connection(self) -> None:
@@ -457,8 +462,8 @@ def _early_cleanup(self) -> None:
"""
Perform any cleanup that needs to happen before the VM exits.
-May be invoked by both soft and hard shutdown in failover scenarios.
-Called additionally by _post_shutdown for comprehensive cleanup.
+This method may be called twice upon shutdown, once each by soft
+and hard shutdown in failover scenarios.
"""
# If we keep the console socket open, we may deadlock waiting
# for QEMU to exit, while QEMU is waiting for the socket to
--
2.31.1
[PULL 3/7] python/machine: add instance disambiguator to default nickname
If you create two instances of QEMUMachine(), they'll both create the
same nickname by default -- which is not that helpful.
Luckily, they'll both create unique temporary directories ... but due to
user configuration, they may share logging and sockfile directories,
meaning two instances can collide. The Python logging will also be quite
confusing, with no differentiation between the two instances.
Add an instance disambiguator (The memory address of the instance) to
the default nickname to foolproof this in all cases.
Signed-off-by: John Snow
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-4-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ea9e07805d..ad529fd92a 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -133,7 +133,7 @@ def __init__(self,
self._wrapper = wrapper
self._qmp_timer = qmp_timer
-self._name = name or "qemu-%d" % os.getpid()
+self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
self._temp_dir: Optional[str] = None
self._base_temp_dir = base_temp_dir
self._sock_dir = sock_dir
--
2.31.1
[PULL 1/7] python/machine: add @sock_dir property
Analogous to temp_dir and log_dir, add a sock_dir property that defaults
to @temp_dir -- instead of base_temp_dir -- when the user hasn't
overridden the sock dir value in the initializer.
This gives us a much more unique directory to put sockfiles in by default.
Signed-off-by: John Snow
Reviewed-by: Willian Rampazzo
Message-id: 2028204620.1897674-2-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a487c39745..b1dd77b538 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -134,8 +134,9 @@ def __init__(self,
self._qmp_timer = qmp_timer
self._name = name or "qemu-%d" % os.getpid()
+self._temp_dir: Optional[str] = None
self._base_temp_dir = base_temp_dir
-self._sock_dir = sock_dir or self._base_temp_dir
+self._sock_dir = sock_dir
self._log_dir = log_dir
if monitor_address is not None:
@@ -143,7 +144,7 @@ def __init__(self,
self._remove_monitor_sockfile = False
else:
self._monitor_address = os.path.join(
-self._sock_dir, f"{self._name}-monitor.sock"
+self.sock_dir, f"{self._name}-monitor.sock"
)
self._remove_monitor_sockfile = True
@@ -163,14 +164,13 @@ def __init__(self,
self._qmp_set = True # Enable QMP monitor by default.
self._qmp_connection: Optional[QEMUMonitorProtocol] = None
self._qemu_full_args: Tuple[str, ...] = ()
-self._temp_dir: Optional[str] = None
self._launched = False
self._machine: Optional[str] = None
self._console_index = 0
self._console_set = False
self._console_device_type: Optional[str] = None
self._console_address = os.path.join(
-self._sock_dir, f"{self._name}-console.sock"
+self.sock_dir, f"{self._name}-console.sock"
)
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
@@ -816,6 +816,15 @@ def temp_dir(self) -> str:
dir=self._base_temp_dir)
return self._temp_dir
+@property
+def sock_dir(self) -> str:
+"""
+Returns the directory used for sockfiles by this machine.
+"""
+if self._sock_dir:
+return self._sock_dir
+return self.temp_dir
+
@property
def log_dir(self) -> str:
"""
--
2.31.1
[PULL 0/7] Python patches
The following changes since commit 89d2f9e4c63799f7f03e9180c63b7dc45fc2a04a: Merge tag 'pull-target-arm-20211122' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2021-11-22 16:35:54 +0100) are available in the Git repository at: https://gitlab.com/jsnow/qemu.git tags/python-pull-request for you to fetch changes up to a57cb3e23d5ac918a69d0aab918470ff0b429ff9: python/aqmp: fix send_fd_scm for python 3.6.x (2021-11-22 18:41:21 -0500) Python testing fixes for 6.2 A few more fixes to help eliminate race conditions from device-crash-test, along with a fix that allows the SCM_RIGHTS functionality to work on hosts that only have Python 3.6. If this is too much this late in the RC process, I'd advocate for at least patch 7/7 by itself. John Snow (7): python/machine: add @sock_dir property python/machine: remove _remove_monitor_sockfile property python/machine: add instance disambiguator to default nickname python/machine: move more variable initializations to _pre_launch python/machine: handle "fast" QEMU terminations scripts/device-crash-test: Use a QMP timeout python/aqmp: fix send_fd_scm for python 3.6.x python/qemu/aqmp/qmp_client.py | 9 -- python/qemu/machine/machine.py | 59 -- scripts/device-crash-test | 2 +- 3 files changed, 42 insertions(+), 28 deletions(-) -- 2.31.1
Re: [PULL 0/5] Python patches
On Thu, Nov 18, 2021 at 1:46 AM Gerd Hoffmann wrote: > Hi, > > > - Split python/qemu/qmp out into its own repository and begin uploading > it > > to PyPI, as a test. (Do not delete python/qemu/qmp yet at this phase.) > > I think you can do that as two separate steps. > > pip can install from vcs too, i.e. when splitted to a separate repo but > not yet uploaded to pypi you can simply drop something like ... > > git+https://gitlab.com/qemu/qemu-python.git@master > > ... into pip-requirements.txt. That way you can easily test things > before actually uploading to pypi. > > Indeed - a limitation here however is that pip will not install from this source unless explicitly asked to, so you couldn't use this package as a requirement for another one, for example -- but it works as a testing step. but that's the rough outline of where I am headed and what I think needs to be done to get there. It's just taking me a while to get everything put in order exactly the right way to be able to flip the switch. Hopefully soon, though. I realized when re-reading my mails last night that I said I wouldn't be able to do it until "next release" but what I really meant was "until the next development window". --js
Re: Failing QEMU iotests
On Wed, Nov 17, 2021 at 4:33 PM Thomas Huth wrote: > On 17/11/2021 20.59, John Snow wrote: > > > > > > On Wed, Nov 17, 2021 at 2:45 PM Thomas Huth > <mailto:th...@redhat.com>> wrote: > > > > On 17/11/2021 19.13, John Snow wrote: > > > > > > > > > On Wed, Nov 17, 2021 at 5:07 AM Thomas Huth > <mailto:th...@redhat.com> > > > <mailto:th...@redhat.com <mailto:th...@redhat.com>>> wrote: > > > > > > > > >Hi! > > > > > > I think it has been working fine for me a couple of weeks ago, > > > but when I now run: > > > > > >make check SPEED=slow > > > > > > I'm getting a couple of failing iotests... not sure whether > > > these are known issues already, so I thought I'd summarize > them > > > here: > > > > > > *** First one is 045 in raw mode: *** > > > > > >TEST iotest-raw: 045 [fail] > > > QEMU -- > > > > > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-system-x86_64" > > > -nodefaults -display none -accel qtest > > > QEMU_IMG -- > > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-img" > > > QEMU_IO -- > > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-io" > --cache > > > writeback --aio threads -f raw > > > QEMU_NBD -- > > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-nbd" > > > IMGFMT-- raw > > > IMGPROTO -- file > > > PLATFORM -- Linux/x86_64 thuth > 4.18.0-305.19.1.el8_4.x86_64 > > > TEST_DIR -- > > /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch > > > SOCK_DIR -- /tmp/tmphlexdrlt > > > GDB_OPTIONS -- > > > VALGRIND_QEMU -- > > > PRINT_QEMU_OUTPUT -- > > > > > > --- /home/thuth/devel/qemu/tests/qemu-iotests/045.out > > > +++ 045.out.bad > > > @@ -1,5 +1,77 @@ > > > -... > > > +..EE.EE <http://EE.EE> <http://EE.EE <http://EE.EE>> > > > > > > +== > > > +ERROR: test_add_fd (__main__.TestSCMFd) > > > > > > +-- > > > +Traceback (most recent call last): > > > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line > 148, in > > > test_add_fd > > > +self._send_fd_by_SCM() > > > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line > 144, in > > > _send_fd_by_SCM > > > +ret = self.vm.send_fd_scm(file_path=image0) > > > + File > "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line > > > 229, in send_fd_scm > > > +self._qmp.send_fd_scm(fd) > > > + File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", > line > > 138, > > > in send_fd_scm > > > +self._aqmp.send_fd_scm(fd) > > > + File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", > > line 149, > > > in _wrapper > > > +return func(proto, *args, **kwargs) > > > + File > "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line > > > 644, in send_fd_scm > > > +sock = sock._sock # pylint: disable=protected-access > > > +AttributeError: 'socket' object has no attribute '_sock' > > > > > > > > > Well, that's not good. > > > > > > Can you tell me some details about what system produced this > failure? > > > The python version used to run the test would be good, as well as > distro > > > release, kernel version, etc. > > > > > > If you can reproduce it, I might want to give you a test branch > of the > > > python code to produce some extra debugging information to help me > > > und
Re: Failing QEMU iotests
On Wed, Nov 17, 2021 at 2:45 PM Thomas Huth wrote: > On 17/11/2021 19.13, John Snow wrote: > > > > > > On Wed, Nov 17, 2021 at 5:07 AM Thomas Huth > <mailto:th...@redhat.com>> wrote: > > > > > >Hi! > > > > I think it has been working fine for me a couple of weeks ago, > > but when I now run: > > > >make check SPEED=slow > > > > I'm getting a couple of failing iotests... not sure whether > > these are known issues already, so I thought I'd summarize them > > here: > > > > *** First one is 045 in raw mode: *** > > > >TEST iotest-raw: 045 [fail] > > QEMU -- > > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-system-x86_64" > > -nodefaults -display none -accel qtest > > QEMU_IMG -- > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-img" > > QEMU_IO -- > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-io" --cache > > writeback --aio threads -f raw > > QEMU_NBD -- > > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-nbd" > > IMGFMT-- raw > > IMGPROTO -- file > > PLATFORM -- Linux/x86_64 thuth 4.18.0-305.19.1.el8_4.x86_64 > > TEST_DIR -- > /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch > > SOCK_DIR -- /tmp/tmphlexdrlt > > GDB_OPTIONS -- > > VALGRIND_QEMU -- > > PRINT_QEMU_OUTPUT -- > > > > --- /home/thuth/devel/qemu/tests/qemu-iotests/045.out > > +++ 045.out.bad > > @@ -1,5 +1,77 @@ > > -... > > +..EE.EE <http://EE.EE> > > > +== > > +ERROR: test_add_fd (__main__.TestSCMFd) > > > +-- > > +Traceback (most recent call last): > > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 148, in > > test_add_fd > > +self._send_fd_by_SCM() > > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in > > _send_fd_by_SCM > > +ret = self.vm.send_fd_scm(file_path=image0) > > + File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line > > 229, in send_fd_scm > > +self._qmp.send_fd_scm(fd) > > + File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line > 138, > > in send_fd_scm > > +self._aqmp.send_fd_scm(fd) > > + File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line > 149, > > in _wrapper > > +return func(proto, *args, **kwargs) > > + File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line > > 644, in send_fd_scm > > +sock = sock._sock # pylint: disable=protected-access > > +AttributeError: 'socket' object has no attribute '_sock' > > > > > > Well, that's not good. > > > > Can you tell me some details about what system produced this failure? > > The python version used to run the test would be good, as well as distro > > release, kernel version, etc. > > > > If you can reproduce it, I might want to give you a test branch of the > > python code to produce some extra debugging information to help me > > understand what's gone wrong here. Get in touch on IRC when you have > some > > spare time if you'd like to interactively debug it. > > As you likely saw in Hanna's mail a little bit later, the problem was the > old version of pylint. I did still have version 2.2 installed - after > upgrading, the problem went away. > > upgrading pylint made *this* problem in *045* go away and not just the failure in *297*, are you positive?
Re: [PULL 0/5] Python patches
On Wed, Nov 17, 2021 at 1:20 PM Vladimir Sementsov-Ogievskiy < vsement...@virtuozzo.com> wrote: > 17.11.2021 20:56, John Snow wrote: > > > > On Wed, Nov 17, 2021 at 4:42 AM Gerd Hoffmann <mailto:kra...@redhat.com>> wrote: > > > >Hi, > > > > > https://gitlab.com/jsnow/qemu.git < > https://gitlab.com/jsnow/qemu.git> tags/python-pull-request > > > > What is the status of the plan to upload this to pypi eventually? > > > > > > Thanks for asking! > > > > The honest answer is "I'm not exactly sure", but there are a few things > to work out still. Let me use this as an opportunity to try and give you an > honest answer. > > We've got four packages right now: qmp, aqmp, machine and utils. > > > > - I don't intend to *ever* upload utils, I created that one specifically > as an in-tree package for "low quality" code that we just need as glue. > > - aqmp is brand new. It was moved as the default provider for the QMP > protocol in the tree (being used by machine.py) only two weeks ago. I am > using this current RC testing phase to find any problems with it. > > - qmp is something I want to deprecate, I don't intend to upload it to > PyPI. I intend to rename aqmp -> qmp and have just the one qmp package. I > can't do this until next release, and only after we are confident and happy > that aqmp is stable enough. > > - machine has a few problems with it. I am reluctant to upload it in its > current form. I am actively developing a new version of it that uses the > new Async QMP module. However, this might take a bit of time, I fear. > > > > So, I think I have this timeline for myself: > > > > - Fix bugs in AQMP package revealed during RC testing > > - Introduce sync wrapper for AQMP that resembles the native AQMP > interface more than it resembles the "legacy QMP" interface. > > - Remove all QEMU source tree uses of qemu.qmp and qemu.aqmp.legacy. > > - Delete qemu.qmp and rename qemu.aqmp to qemu.qmp. > > - Split python/qemu/qmp out into its own repository and begin uploading > it to PyPI, as a test. (Do not delete python/qemu/qmp yet at this phase.) > > - Transition any users of the Python packages in the QEMU source tree to > installing the QMP dependency from PyPI instead of grabbing it from the > tree. > > - Delete python/qemu/qmp from the QEMU source tree at this moment; > "re-fork" the package if necessary to collect any commits since the "test > split" procedure. > > > > That all sounds great! > > > > > Some questions to work out: > > - What tools should be uploaded with qemu.qmp? a version of qmp-shell is > high on the list for me. qom, qom-set, qom-get, qom-list, qom-tree, > qom-fuse etc I am suspecting might be better left behind in qemu.utils > instead, though. I am not sure I want to support those more broadly. They > weren't designed for "external consumption". > > - qemu-ga-client should be moved over into utils, or possibly even > deleted -- it hasn't seen a lot of love and I doubt there are any users. I > don't have the bandwidth to refurbish it for no users. Maybe if there's a > demand in the future ... > > > > > > ... This might be being overcautious, though. Perhaps I can upload a > version of "qemu.aqmp" even this week just as a demonstration of how it > would work. > > > > Why do we need wait for next release for renaming aqmp -> qmp? Or what > next release do you mean? I think you can rename it as soon as 6.3 > development phase is open. > > I might be confused in my thinking because there's a ton of little tasks to do, and I won't pretend I have thought extremely carefully about the precise order in which they *have* to be done, only the order in which that it occurs to me to do them. :) I suppose I could do something like rename "qmp" to "legacy_qmp" in the tree as an intermediate step and accomplish the "aqmp -> qmp" rename sooner, but there's a lot of churn inherent to that. Since there's a lot of churn inherent to moving users off of the old interface anyway, I figured I'd just tackle it all at once... which I can't do until the tree re-opens again. I can certainly work on it in the meantime, though. I'm not sure that's a good idea to upload qemu.aqmp to public and than > rename it to qemu.qmp.. Maybe, you can upload it now as qemu.qmp? So, > first, create a separate repo with aqmp (already renamed to qmp), upload it > to PyPI (as qmp) - this all as a first step. And then gradually move Qemu > to use this new repo instead its own qmp/aqmp. > I'm afraid of doing this because I don't want to p
Re: Failing QEMU iotests
On Wed, Nov 17, 2021 at 5:07 AM Thomas Huth wrote: > > Hi! > > I think it has been working fine for me a couple of weeks ago, > but when I now run: > > make check SPEED=slow > > I'm getting a couple of failing iotests... not sure whether > these are known issues already, so I thought I'd summarize them > here: > > *** First one is 045 in raw mode: *** > > TEST iotest-raw: 045 [fail] > QEMU -- > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-system-x86_64" > -nodefaults -display none -accel qtest > QEMU_IMG -- > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-img" > QEMU_IO -- > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-io" --cache > writeback --aio threads -f raw > QEMU_NBD -- > "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-nbd" > IMGFMT-- raw > IMGPROTO -- file > PLATFORM -- Linux/x86_64 thuth 4.18.0-305.19.1.el8_4.x86_64 > TEST_DIR -- /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch > SOCK_DIR -- /tmp/tmphlexdrlt > GDB_OPTIONS -- > VALGRIND_QEMU -- > PRINT_QEMU_OUTPUT -- > > --- /home/thuth/devel/qemu/tests/qemu-iotests/045.out > +++ 045.out.bad > @@ -1,5 +1,77 @@ > -... > +..EE.EE > +== > +ERROR: test_add_fd (__main__.TestSCMFd) > +-- > +Traceback (most recent call last): > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 148, in > test_add_fd > +self._send_fd_by_SCM() > + File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in > _send_fd_by_SCM > +ret = self.vm.send_fd_scm(file_path=image0) > + File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 229, > in send_fd_scm > +self._qmp.send_fd_scm(fd) > + File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, in > send_fd_scm > +self._aqmp.send_fd_scm(fd) > + File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 149, > in _wrapper > +return func(proto, *args, **kwargs) > + File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 644, > in send_fd_scm > +sock = sock._sock # pylint: disable=protected-access > +AttributeError: 'socket' object has no attribute '_sock' > Well, that's not good. Can you tell me some details about what system produced this failure? The python version used to run the test would be good, as well as distro release, kernel version, etc. If you can reproduce it, I might want to give you a test branch of the python code to produce some extra debugging information to help me understand what's gone wrong here. Get in touch on IRC when you have some spare time if you'd like to interactively debug it. --js
Re: Failing QEMU iotests
On Wed, Nov 17, 2021 at 7:50 AM Thomas Huth wrote:
> On 17/11/2021 11.59, Hanna Reitz wrote:
> > On 17.11.21 11:07, Thomas Huth wrote:
>
> >> +++ 297.out.bad
> >> @@ -1,2 +1,21 @@
> >> === pylint ===
> >> +* Module image-fleecing
> >> +tests/image-fleecing:34:24: C0326: Exactly one space required after
> comma
> >> +patterns = [('0x5d', '0', '64k'),
> >> +^ (bad-whitespace)
> >> +tests/image-fleecing:35:25: C0326: Exactly one space required after
> comma
> >> +('0xd5', '1M','64k'),
> >> + ^ (bad-whitespace)
> >> +tests/image-fleecing:36:26: C0326: Exactly one space required after
> comma
> >> +('0xdc', '32M', '64k'),
> >> + ^ (bad-whitespace)
> >> +tests/image-fleecing:39:25: C0326: Exactly one space required after
> comma
> >> +overwrite = [('0xab', '0', '64k'), # Full overwrite
> >> + ^ (bad-whitespace)
> >> +tests/image-fleecing:48:32: C0326: Exactly one space required after
> comma
> >> +remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left
> [1]
> >> +^ (bad-whitespace)
> >> +tests/image-fleecing:49:27: C0326: Exactly one space required after
> comma
> >> + ('0xdc', '32M', '32k'), # Left-end of partial-right
> [2]
> >> + ^ (bad-whitespace)
> >
> > This could be because your pylint is too old. At least for the python/
> > tests we at least require 2.8.0
> > (https://lists.nongnu.org/archive/html/qemu-block/2021-10/msg00768.html)
> and
> > bad-whitespace was removed in 2.6.
>
> Thanks, updating pylint fixed this problem, indeed!
>
> But maybe the iotests should check the pylint version before using it?
>
>
Ideally, yes ... sorry, it's been a lot of work to try and get the python
testing for this stuff in order.
FWIW, the GitLab CI jobs for check-python-pipenv and check-python-tox now
basically run "iotest 297", and those jobs will use virtual environments to
force a supportable version of pylint/mypy/etc. These targets are the ones
I put the most effort into, and those are the ones that "just work".
It's on my list to, one way or another, drop 297 and use the python testing
infra to cover this instead, but I have some ground to cover for
usability/convenience before I can pitch it.
(At the risk of sounding like I am task offloading, if you send a patch to
add version checking to 297, I can review it.)
--js
Re: [PULL 0/5] Python patches
On Wed, Nov 17, 2021 at 4:42 AM Gerd Hoffmann wrote: > Hi, > > > https://gitlab.com/jsnow/qemu.git tags/python-pull-request > > What is the status of the plan to upload this to pypi eventually? > > Thanks for asking! The honest answer is "I'm not exactly sure", but there are a few things to work out still. Let me use this as an opportunity to try and give you an honest answer. We've got four packages right now: qmp, aqmp, machine and utils. - I don't intend to *ever* upload utils, I created that one specifically as an in-tree package for "low quality" code that we just need as glue. - aqmp is brand new. It was moved as the default provider for the QMP protocol in the tree (being used by machine.py) only two weeks ago. I am using this current RC testing phase to find any problems with it. - qmp is something I want to deprecate, I don't intend to upload it to PyPI. I intend to rename aqmp -> qmp and have just the one qmp package. I can't do this until next release, and only after we are confident and happy that aqmp is stable enough. - machine has a few problems with it. I am reluctant to upload it in its current form. I am actively developing a new version of it that uses the new Async QMP module. However, this might take a bit of time, I fear. So, I think I have this timeline for myself: - Fix bugs in AQMP package revealed during RC testing - Introduce sync wrapper for AQMP that resembles the native AQMP interface more than it resembles the "legacy QMP" interface. - Remove all QEMU source tree uses of qemu.qmp and qemu.aqmp.legacy. - Delete qemu.qmp and rename qemu.aqmp to qemu.qmp. - Split python/qemu/qmp out into its own repository and begin uploading it to PyPI, as a test. (Do not delete python/qemu/qmp yet at this phase.) - Transition any users of the Python packages in the QEMU source tree to installing the QMP dependency from PyPI instead of grabbing it from the tree. - Delete python/qemu/qmp from the QEMU source tree at this moment; "re-fork" the package if necessary to collect any commits since the "test split" procedure. Some questions to work out: - What tools should be uploaded with qemu.qmp? a version of qmp-shell is high on the list for me. qom, qom-set, qom-get, qom-list, qom-tree, qom-fuse etc I am suspecting might be better left behind in qemu.utils instead, though. I am not sure I want to support those more broadly. They weren't designed for "external consumption". - qemu-ga-client should be moved over into utils, or possibly even deleted -- it hasn't seen a lot of love and I doubt there are any users. I don't have the bandwidth to refurbish it for no users. Maybe if there's a demand in the future ... ... This might be being overcautious, though. Perhaps I can upload a version of "qemu.aqmp" even this week just as a demonstration of how it would work. Happy to take suggestions/feedback on this process. --js
[PULL 5/5] scripts/device-crash-test: hide tracebacks for QMP connect errors
Generally, the traceback for a connection failure is uninteresting and
all we need to know is that the connection attempt failed.
Reduce the verbosity in these cases, except when debugging.
Signed-off-by: John Snow
Reported-by: Thomas Huth
Tested-by: Thomas Huth
Message-id: 2021143719.2162525-6-js...@redhat.com
Signed-off-by: John Snow
---
scripts/device-crash-test | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 49bcd61b4f..3db0ffe5b8 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -36,6 +36,7 @@ from itertools import chain
sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
from qemu.machine import QEMUMachine
+from qemu.aqmp import ConnectError
logger = logging.getLogger('device-crash-test')
dbg = logger.debug
@@ -355,10 +356,12 @@ def checkOneCase(args, testcase):
dbg("will launch QEMU: %s", cmdline)
vm = QEMUMachine(binary=binary, args=args)
+exc = None
exc_traceback = None
try:
vm.launch()
-except Exception:
+except Exception as this_exc:
+exc = this_exc
exc_traceback = traceback.format_exc()
dbg("Exception while running test case")
finally:
@@ -366,8 +369,9 @@ def checkOneCase(args, testcase):
ec = vm.exitcode()
log = vm.get_log()
-if exc_traceback is not None or ec != 0:
-return {'exc_traceback':exc_traceback,
+if exc is not None or ec != 0:
+return {'exc': exc,
+'exc_traceback':exc_traceback,
'exitcode':ec,
'log':log,
'testcase':testcase,
@@ -455,6 +459,17 @@ def logFailure(f, level):
for l in f['log'].strip().split('\n'):
logger.log(level, "log: %s", l)
logger.log(level, "exit code: %r", f['exitcode'])
+
+# If the Exception is merely a QMP connect error,
+# reduce the logging level for its traceback to
+# improve visual clarity.
+if isinstance(f.get('exc'), ConnectError):
+logger.log(level, "%s.%s: %s",
+ type(f['exc']).__module__,
+ type(f['exc']).__qualname__,
+ str(f['exc']))
+level = logging.DEBUG
+
if f['exc_traceback']:
logger.log(level, "exception:")
for l in f['exc_traceback'].split('\n'):
--
2.31.1
[PULL 3/5] scripts/device-crash-test: simplify Exception handling
We don't need to handle KeyboardInterruptError specifically; we can
instead tighten the scope of the broad Exception handlers to only catch
"Exception", which has the effect of allowing all BaseException classes
that do not inherit from Exception to be raised through.
KeyboardInterruptError and a few other important ones are
BaseExceptions, so this does the same thing with less code.
Signed-off-by: John Snow
Reported-by: Thomas Huth
Tested-by: Thomas Huth
Message-id: 2021143719.2162525-4-js...@redhat.com
Signed-off-by: John Snow
---
scripts/device-crash-test | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 8331c057b8..d91e8616ef 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -317,9 +317,7 @@ class QemuBinaryInfo(object):
try:
vm.launch()
mi['runnable'] = True
-except KeyboardInterrupt:
-raise
-except:
+except Exception:
dbg("exception trying to run binary=%s machine=%s", self.binary,
machine, exc_info=sys.exc_info())
dbg("log: %r", vm.get_log())
mi['runnable'] = False
@@ -360,9 +358,7 @@ def checkOneCase(args, testcase):
exc_traceback = None
try:
vm.launch()
-except KeyboardInterrupt:
-raise
-except:
+except Exception:
exc_traceback = traceback.format_exc()
dbg("Exception while running test case")
finally:
--
2.31.1
[PULL 1/5] python/aqmp: Fix disconnect during capabilities negotiation
If we receive ConnectionResetError (ECONNRESET) while attempting to
perform capabilities negotiation -- prior to the establishment of the
async reader/writer tasks -- the disconnect function is not aware that
we are in an error pathway.
As a result, when attempting to close the StreamWriter, we'll see the
same ConnectionResetError that caused us to initiate a disconnect in the
first place, which will cause the disconnect task itself to fail, which
emits a CRITICAL logging event.
I still don't know if there's a smarter way to check to see if an
exception received at this point is "the same" exception as the one that
caused the initial disconnect, but for now the problem can be avoided by
improving the error pathway detection in the exit path.
Reported-by: Thomas Huth
Signed-off-by: John Snow
Tested-by: Thomas Huth
Message-id: 2021143719.2162525-2-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index ae1df24026..860b79512d 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -623,13 +623,21 @@ async def _bh_disconnect(self) -> None:
def _done(task: Optional['asyncio.Future[Any]']) -> bool:
return task is not None and task.done()
-# NB: We can't rely on _bh_tasks being done() here, it may not
-# yet have had a chance to run and gather itself.
+# Are we already in an error pathway? If either of the tasks are
+# already done, or if we have no tasks but a reader/writer; we
+# must be.
+#
+# NB: We can't use _bh_tasks to check for premature task
+# completion, because it may not yet have had a chance to run
+# and gather itself.
tasks = tuple(filter(None, (self._writer_task, self._reader_task)))
error_pathway = _done(self._reader_task) or _done(self._writer_task)
+if not tasks:
+error_pathway |= bool(self._reader) or bool(self._writer)
try:
-# Try to flush the writer, if possible:
+# Try to flush the writer, if possible.
+# This *may* cause an error and force us over into the error path.
if not error_pathway:
await self._bh_flush_writer()
except BaseException as err:
@@ -639,7 +647,7 @@ def _done(task: Optional['asyncio.Future[Any]']) -> bool:
self.logger.debug("%s:\n%s\n", emsg, pretty_traceback())
raise
finally:
-# Cancel any still-running tasks:
+# Cancel any still-running tasks (Won't raise):
if self._writer_task is not None and not self._writer_task.done():
self.logger.debug("Cancelling writer task.")
self._writer_task.cancel()
@@ -652,7 +660,7 @@ def _done(task: Optional['asyncio.Future[Any]']) -> bool:
self.logger.debug("Waiting for tasks to complete ...")
await asyncio.wait(tasks)
-# Lastly, close the stream itself. (May raise):
+# Lastly, close the stream itself. (*May raise*!):
await self._bh_close_stream(error_pathway)
self.logger.debug("Disconnected.")
--
2.31.1
[PULL 2/5] python/aqmp: fix ConnectError string method
When ConnectError is used to wrap an Exception that was initialized
without an error message, we are treated to a traceback with a rubbish
line like this:
... ConnectError: Failed to establish session:
Correct this to use the name of an exception as a fallback message:
... ConnectError: Failed to establish session: EOFError
Better!
Signed-off-by: John Snow
Reported-by: Thomas Huth
Tested-by: Thomas Huth
Message-id: 2021143719.2162525-3-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 860b79512d..5190b33b13 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -79,7 +79,11 @@ def __init__(self, error_message: str, exc: Exception):
self.exc: Exception = exc
def __str__(self) -> str:
-return f"{self.error_message}: {self.exc!s}"
+cause = str(self.exc)
+if not cause:
+# If there's no error string, use the exception name.
+cause = exception_summary(self.exc)
+return f"{self.error_message}: {cause}"
class StateError(AQMPError):
--
2.31.1
[PULL 4/5] scripts/device-crash-test: don't emit AQMP connection errors to stdout
These errors are expected, so they shouldn't clog up terminal output. In
the event that they're *not* expected, we'll be seeing an awful lot more
output concerning the nature of the failure.
Reported-by: Thomas Huth
Signed-off-by: John Snow
Tested-by: Thomas Huth
Message-id: 2021143719.2162525-5-js...@redhat.com
Signed-off-by: John Snow
---
scripts/device-crash-test | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index d91e8616ef..49bcd61b4f 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -499,6 +499,12 @@ def main():
lvl = logging.WARN
logging.basicConfig(stream=sys.stdout, level=lvl, format='%(levelname)s:
%(message)s')
+if not args.debug:
+# Async QMP, when in use, is chatty about connection failures.
+# This script knowingly generates a ton of connection errors.
+# Silence this logger.
+logging.getLogger('qemu.aqmp.qmp_client').setLevel(logging.CRITICAL)
+
fatal_failures = []
wl_stats = {}
skipped = 0
--
2.31.1
[PULL 0/5] Python patches
The following changes since commit 2b22e7540d6ab4efe82d442363e3fc900cea6584: Merge tag 'm68k-for-6.2-pull-request' of git://github.com/vivier/qemu-m68k into staging (2021-11-09 13:16:56 +0100) are available in the Git repository at: https://gitlab.com/jsnow/qemu.git tags/python-pull-request for you to fetch changes up to c398a241ec4138e0b995a0215dea84ca93b0384f: scripts/device-crash-test: hide tracebacks for QMP connect errors (2021-11-16 14:26:36 -0500) Pull request John Snow (5): python/aqmp: Fix disconnect during capabilities negotiation python/aqmp: fix ConnectError string method scripts/device-crash-test: simplify Exception handling scripts/device-crash-test: don't emit AQMP connection errors to stdout scripts/device-crash-test: hide tracebacks for QMP connect errors python/qemu/aqmp/protocol.py | 24 ++-- scripts/device-crash-test| 33 + 2 files changed, 43 insertions(+), 14 deletions(-) -- 2.31.1
Re: [PULL 22/22] python, iotests: replace qmp with aqmp
On Tue, Nov 9, 2021 at 9:07 AM Thomas Huth wrote: > On 01/11/2021 18.30, John Snow wrote: > > Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync > > wrapper from qemu.aqmp instead. > > > > Add an escape hatch in the form of the environment variable > > QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using > > the old implementation, proving that both implementations work > > concurrently. > > Hi John, > > seems like this patch broke our device-crash-test script. If I now run > "scripts/device-crash-test -q" I get lots of error messages... could you > please have a look? > > Thomas > I'm on it, thanks for the report and sorry for the inconvenience. --js
[PULL 22/22] python, iotests: replace qmp with aqmp
Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.
Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementation, proving that both implementations work
concurrently.
Signed-off-by: John Snow
Reviewed-by: Kevin Wolf
Reviewed-by: Hanna Reitz
Message-id: 20211026175612.4127598-9-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
)
from qemu.qmp import ( # pylint: disable=import-error
-QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
SocketAddrT,
@@ -50,6 +49,12 @@
from . import console_socket
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
LOG = logging.getLogger(__name__)
--
2.31.1
[PULL 21/22] python/aqmp: Create sync QMP wrapper for iotests
This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.
It does not, however, attempt to mimic Exception compatibility.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Reviewed-by: Hanna Reitz
Message-id: 20211026175612.4127598-8-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 138 +
1 file changed, 138 insertions(+)
create mode 100644 python/qemu/aqmp/legacy.py
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+ wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
--
2.31.1
[PULL 16/22] python/machine: Handle QMP errors on close more meticulously
To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.
(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)
Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20211026175612.4127598-3-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 48 +-
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
# Comprehensive reset for the failed launch case:
self._early_cleanup()
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err: # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
self._close_qemu_log_file()
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
self._post_launch()
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
def _early_cleanup(self) -> None:
"""
Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
self._early_cleanup()
if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
--
2.31.1
[PULL 15/22] python/machine: remove has_quit argument
If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.
The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.
In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20211026175612.4127598-2-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 34 +++---
tests/qemu-iotests/040 | 7 +--
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/255 | 2 +-
4 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
self._user_killed = False
+self._quit_issued = False
def __enter__(self: _T) -> _T:
return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
command = ''
LOG.warning(msg, -int(exitcode), command)
+self._quit_issued = False
self._user_killed = False
self._launched = False
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
self._subp.kill()
self._subp.wait(timeout=60)
-def _soft_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
"""
Perform early cleanup, attempt to gracefully shut down the VM, and wait
for it to terminate.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise ConnectionReset: On QMP communication errors
:raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
self._early_cleanup()
if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
# Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
"""
Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise AbnormalShutdown: When the VM could not be shut down gracefully.
The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
may result in its own exceptions, likely subprocess.TimeoutExpired.
"""
try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
except Exception as exc:
self._hard_shutdown()
raise AbnormalShutdown("Could not perform graceful shutdown") \
from exc
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
hard: bool = False,
timeout: Optional[int] = 30) -> None:
"""
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
If the VM has not yet been launched, or shutdown(), wait(), or kill()
have already been called, this method does nothing.
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
:param hard: When true, do not attempt graceful shutdown, and
suppress the SIGKILL warning log message.
:param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
self._user_killed = True
self._hard_shutdown()
else:
-self._do_shutdown(timeout, has_quit)
+self._do_s
[PULL 13/22] iotests/linters: Add workaround for mypy bug #9852
This one is insidious: if you write an import as "from {namespace}
import {subpackage}" as mirror-top-perms (now) does, mypy will fail on
every-other invocation *if* the package being imported is a typed,
installed, namespace-scoped package.
Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in
the context of Python CI tests.
Now, I could just edit mirror-top-perms to avoid this invocation, but
since I tripped on a landmine, I might as well head it off at the pass
and make sure nobody else trips on that same landmine.
It seems to have something to do with the order in which files are
checked as well, meaning the random order in which set(os.listdir())
produces the list of files to test will cause problems intermittently
and not just strictly "every other run".
This will be fixed in mypy >= 0.920, which is not released yet. The
workaround for now is to disable incremental checking, which avoids the
issue.
Note: This workaround is not applied when running iotest 297 directly,
because the bug does not surface there! Given the nature of CI jobs not
starting with any stale cache to begin with, this really only has a
half-second impact on manual runs of the Python test suite when executed
directly by a developer on their local machine. The workaround may be
removed when the Python package requirements can stipulate mypy 0.920 or
higher, which can happen as soon as it is released. (Barring any
unforseen compatibility issues that 0.920 may bring with it.)
See also:
https://github.com/python/mypy/issues/11010
https://github.com/python/mypy/issues/9852
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-14-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/linters.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 46c28fdcda0..65c4c4e8272 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -93,7 +93,9 @@ def show_usage() -> None:
if sys.argv[1] == '--pylint':
run_linter('pylint', files)
elif sys.argv[1] == '--mypy':
-run_linter('mypy', files)
+# mypy bug #9852; disable incremental checking as a workaround.
+args = ['--no-incremental'] + files
+run_linter('mypy', args)
else:
print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
show_usage()
--
2.31.1
[PULL 09/22] iotests/297: update tool availability checks
As mentioned in 'iotests/297: Don't rely on distro-specific linter
binaries', these checks are overly strict. Update them to be in-line
with how we actually invoke the linters themselves.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-10-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 76d6a23f531..b2ad8d1cbe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -18,7 +18,6 @@
import os
import re
-import shutil
import subprocess
import sys
from typing import List, Mapping, Optional
@@ -84,9 +83,11 @@ def run_linter(
def main() -> None:
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+for linter in ('pylint', 'mypy'):
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.notrun(f"'{linter}' not found")
files = get_test_files()
--
2.31.1
[PULL 20/22] iotests/300: avoid abnormal shutdown race condition
Wait for the destination VM to close itself instead of racing to shut it
down first, which produces different error log messages from AQMP
depending on precisely when we tried to shut it down.
(For example: We may try to issue 'quit' immediately prior to the target
VM closing its QMP socket, which will cause an ECONNRESET error to be
logged. Waiting for the VM to exit itself avoids the race on shutdown
behavior.)
Reported-by: Hanna Reitz
Signed-off-by: John Snow
Reviewed-by: Kevin Wolf
Reviewed-by: Hanna Reitz
Message-id: 20211026175612.4127598-7-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/300 | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 10f9f2a8da6..dbd28384ec3 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,8 +24,6 @@ import random
import re
from typing import Dict, List, Optional
-from qemu.machine import machine
-
import iotests
@@ -461,12 +459,11 @@ class
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
f"'{self.src_node_name}': Name is longer than 255 bytes",
log)
-# Expect abnormal shutdown of the destination VM because of
-# the failed migration
-try:
-self.vm_b.shutdown()
-except machine.AbnormalShutdown:
-pass
+# Destination VM will terminate w/ error of its own accord
+# due to the failed migration.
+self.vm_b.wait()
+rc = self.vm_b.exitcode()
+assert rc is not None and rc > 0
def test_aliased_bitmap_name_too_long(self) -> None:
# Longer than the maximum for bitmap names
--
2.31.1
[PULL 19/22] iotests: Conditionally silence certain AQMP errors
AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.
For example, during a failed QMP connection attempt, we might see:
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError
This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.
For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.
(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20211026175612.4127598-6-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/iotests.py | 20 +++-
tests/qemu-iotests/tests/mirror-top-perms | 12
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
import subprocess
import sys
import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
import unittest
@@ -114,6 +114,24 @@
sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
def unarchive_sample_image(sample, fname):
sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
from qemu.qmp import QMPConnectError
import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
- 'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+ 'this should not have happened')
except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
--
2.31.1
[PULL 18/22] iotests: Accommodate async QMP Exception classes
(But continue to support the old ones for now, too.)
There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:
- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20211026175612.4127598-5-js...@redhat.com
Signed-off-by: John Snow
---
scripts/simplebench/bench_block_job.py| 3 ++-
tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
import os
-from qemu import qmp
+from qemu.aqmp import ConnectError
from qemu.machine import machine
+from qemu.qmp import QMPConnectError
import iotests
from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PULL 14/22] python: Add iotest linters to test suite
Run mypy and pylint on the iotests files directly from the Python CI test infrastructure. This ensures that any accidental breakages to the qemu.[qmp|aqmp|machine|utils] packages will be caught by that test suite. It also ensures that these linters are run with well-known versions and test against a wide variety of python versions, which helps to find accidental cross-version python compatibility issues. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-15-js...@redhat.com Signed-off-by: John Snow --- python/tests/iotests-mypy.sh | 4 python/tests/iotests-pylint.sh | 4 2 files changed, 8 insertions(+) create mode 100755 python/tests/iotests-mypy.sh create mode 100755 python/tests/iotests-pylint.sh diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh new file mode 100755 index 000..ee764708199 --- /dev/null +++ b/python/tests/iotests-mypy.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --mypy diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh new file mode 100755 index 000..4cae03424b4 --- /dev/null +++ b/python/tests/iotests-pylint.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --pylint -- 2.31.1
[PULL 07/22] iotests/297: refactor run_[mypy|pylint] as generic execution shim
There's virtually nothing special here anymore; we can combine these
into a single, rather generic function.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-8-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 42 ++
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 189bcaf5f94..d21673a2929 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,27 +61,29 @@ def get_test_files() -> List[str]:
return list(filter(is_python_file, check_tests))
-def run_pylint(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
+def run_linter(
+tool: str,
+args: List[str],
+env: Optional[Mapping[str, str]] = None,
+suppress_output: bool = False,
) -> None:
+"""
+Run a python-based linting tool.
-subprocess.run(('python3', '-m', 'pylint', *files),
- env=env, check=False)
+If suppress_output is True, capture stdout/stderr of the child
+process and only print that information back to stdout if the child
+process's return code was non-zero.
+"""
+p = subprocess.run(
+('python3', '-m', tool, *args),
+env=env,
+check=False,
+stdout=subprocess.PIPE if suppress_output else None,
+stderr=subprocess.STDOUT if suppress_output else None,
+universal_newlines=True,
+)
-
-def run_mypy(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
-) -> None:
-p = subprocess.run(('python3', '-m', 'mypy', *files),
- env=env,
- check=False,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True)
-
-if p.returncode != 0:
+if suppress_output and p.returncode != 0:
print(p.stdout)
@@ -100,11 +102,11 @@ def main() -> None:
print('=== pylint ===')
sys.stdout.flush()
-run_pylint(files, env=env)
+run_linter('pylint', files, env=env)
print('=== mypy ===')
sys.stdout.flush()
-run_mypy(files, env=env)
+run_linter('mypy', files, env=env, suppress_output=True)
iotests.script_main(main)
--
2.31.1
[PULL 05/22] iotests/297: Don't rely on distro-specific linter binaries
'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.
Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing. This is addressed by a
commit later in this series;
'iotests/297: update tool availability checks'.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-6-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 163ebc8ebfd..c1bddb9ce0e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,14 +71,14 @@ def run_linters():
sys.stdout.flush()
env = os.environ.copy()
-subprocess.run(('pylint-3', *files),
+subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
print('=== mypy ===')
sys.stdout.flush()
env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy', *files),
+p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
--
2.31.1
[PULL 12/22] iotests/linters: Add entry point for linting via Python CI
We need at least a tiny little shim here to join test file discovery
with test invocation. This logic could conceivably be hosted somewhere
in python/, but I felt it was strictly the least-rude thing to keep the
test logic here in iotests/, even if this small function isn't itself an
iotest.
Note that we don't actually even need the executable bit here, we'll be
relying on the ability to run this module as a script using Python CLI
arguments. No chance it gets misunderstood as an actual iotest that way.
(It's named, not in tests/, doesn't have the execute bit, and doesn't
have an execution shebang.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-13-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/linters.py | 27 +++
1 file changed, 27 insertions(+)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index c515c7afe36..46c28fdcda0 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -16,6 +16,7 @@
import os
import re
import subprocess
+import sys
from typing import List, Mapping, Optional
@@ -74,3 +75,29 @@ def run_linter(
stderr=subprocess.STDOUT if suppress_output else None,
universal_newlines=True,
)
+
+
+def main() -> None:
+"""
+Used by the Python CI system as an entry point to run these linters.
+"""
+def show_usage() -> None:
+print(f"Usage: {sys.argv[0]} < --mypy | --pylint >", file=sys.stderr)
+sys.exit(1)
+
+if len(sys.argv) != 2:
+show_usage()
+
+files = get_test_files()
+
+if sys.argv[1] == '--pylint':
+run_linter('pylint', files)
+elif sys.argv[1] == '--mypy':
+run_linter('mypy', files)
+else:
+print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
+show_usage()
+
+
+if __name__ == '__main__':
+main()
--
2.31.1
[PULL 17/22] python/aqmp: Remove scary message
The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Reviewed-by: Hanna Reitz
Message-id: 20211026175612.4127598-4-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 12
1 file changed, 12 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
# the COPYING file in the top-level directory.
import logging
-import warnings
from .error import AQMPError
from .events import EventListener
@@ -31,17 +30,6 @@
from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
# Suppress logging unless an application engages it.
logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
--
2.31.1
[PULL 11/22] iotests: split linters.py out from 297
Now, 297 is just the iotests-specific incantations and linters.py is as
minimal as I can think to make it. The only remaining element in here
that ought to be configuration and not code is the list of skip files,
but they're still numerous enough that repeating them for mypy and
pylint configurations both would be ... a hassle.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-12-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297| 72 +
tests/qemu-iotests/linters.py | 76 +++
2 files changed, 87 insertions(+), 61 deletions(-)
create mode 100644 tests/qemu-iotests/linters.py
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b7d9d6077b3..ee78a627359 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,74 +17,24 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import os
-import re
import subprocess
import sys
-from typing import List, Mapping, Optional
+from typing import List
import iotests
+import linters
-# TODO: Empty this list!
-SKIP_FILES = (
-'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-'096', '118', '124', '132', '136', '139', '147', '148', '149',
-'151', '152', '155', '163', '165', '194', '196', '202',
-'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '224', '228', '234', '235', '236', '237', '238',
-'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '302', '303', '304', '307',
-'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename):
-if not os.path.isfile(filename):
-return False
-
-if filename.endswith('.py'):
-return True
-
-with open(filename, encoding='utf-8') as f:
-try:
-first_line = f.readline()
-return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError: # Ignore binary files
-return False
-
-
-def get_test_files() -> List[str]:
-named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-return list(filter(is_python_file, check_tests))
-
-
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> None:
-"""
-Run a python-based linting tool.
-
-:param suppress_output: If True, suppress all stdout/stderr output.
-:raise CalledProcessError: If the linter process exits with failure.
-"""
-subprocess.run(
-('python3', '-m', tool, *args),
-env=env,
-check=True,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
-)
+# Looking for something?
+#
+# List of files to exclude from linting: linters.py
+# mypy configuration:mypy.ini
+# pylint configuration: pylintrc
def check_linter(linter: str) -> bool:
try:
-run_linter(linter, ['--version'], suppress_output=True)
+linters.run_linter(linter, ['--version'], suppress_output=True)
except subprocess.CalledProcessError:
iotests.case_notrun(f"'{linter}' not found")
return False
@@ -98,7 +48,7 @@ def test_pylint(files: List[str]) -> None:
if not check_linter('pylint'):
return
-run_linter('pylint', files)
+linters.run_linter('pylint', files)
def test_mypy(files: List[str]) -> None:
@@ -111,11 +61,11 @@ def test_mypy(files: List[str]) -> None:
env = os.environ.copy()
env['MYPYPATH'] = env['PYTHONPATH']
-run_linter('mypy', files, env=env, suppress_output=True)
+linters.run_linter('mypy', files, env=env, suppress_output=True)
def main() -> None:
-files = get_test_files()
+files = linters.get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 000..c515c7afe36
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,76 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICUL
[PULL 10/22] iotests/297: split test into sub-cases
Take iotest 297's main() test function and split it into two sub-cases
that can be skipped individually. We can also drop custom environment
setup from the pylint test as it isn't needed.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-11-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 63 ++
1 file changed, 39 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b2ad8d1cbe0..b7d9d6077b3 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,36 +82,51 @@ def run_linter(
)
+def check_linter(linter: str) -> bool:
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.case_notrun(f"'{linter}' not found")
+return False
+return True
+
+
+def test_pylint(files: List[str]) -> None:
+print('=== pylint ===')
+sys.stdout.flush()
+
+if not check_linter('pylint'):
+return
+
+run_linter('pylint', files)
+
+
+def test_mypy(files: List[str]) -> None:
+print('=== mypy ===')
+sys.stdout.flush()
+
+if not check_linter('mypy'):
+return
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+run_linter('mypy', files, env=env, suppress_output=True)
+
+
def main() -> None:
-for linter in ('pylint', 'mypy'):
-try:
-run_linter(linter, ['--version'], suppress_output=True)
-except subprocess.CalledProcessError:
-iotests.notrun(f"'{linter}' not found")
-
files = get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
-env = os.environ.copy()
-env['MYPYPATH'] = env['PYTHONPATH']
-
-print('=== pylint ===')
-sys.stdout.flush()
-try:
-run_linter('pylint', files, env=env)
-except subprocess.CalledProcessError:
-# pylint failure will be caught by diffing the IO.
-pass
-
-print('=== mypy ===')
-sys.stdout.flush()
-try:
-run_linter('mypy', files, env=env, suppress_output=True)
-except subprocess.CalledProcessError as exc:
-if exc.output:
-print(exc.output)
+for test in (test_pylint, test_mypy):
+try:
+test(files)
+except subprocess.CalledProcessError as exc:
+# Linter failure will be caught by diffing the IO.
+if exc.output:
+print(exc.output)
iotests.script_main(main)
--
2.31.1
[PULL 08/22] iotests/297: Change run_linter() to raise an exception on failure
Instead of using a process return code as the python function return
value (or just not returning anything at all), allow run_linter() to
raise an exception instead.
The responsibility for printing output on error shifts from the function
itself to the caller, who will know best how to present/format that
information. (Also, "suppress_output" is now a lot more accurate of a
parameter name.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-9-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d21673a2929..76d6a23f531 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -70,22 +70,18 @@ def run_linter(
"""
Run a python-based linting tool.
-If suppress_output is True, capture stdout/stderr of the child
-process and only print that information back to stdout if the child
-process's return code was non-zero.
+:param suppress_output: If True, suppress all stdout/stderr output.
+:raise CalledProcessError: If the linter process exits with failure.
"""
-p = subprocess.run(
+subprocess.run(
('python3', '-m', tool, *args),
env=env,
-check=False,
+check=True,
stdout=subprocess.PIPE if suppress_output else None,
stderr=subprocess.STDOUT if suppress_output else None,
universal_newlines=True,
)
-if suppress_output and p.returncode != 0:
-print(p.stdout)
-
def main() -> None:
for linter in ('pylint-3', 'mypy'):
@@ -102,11 +98,19 @@ def main() -> None:
print('=== pylint ===')
sys.stdout.flush()
-run_linter('pylint', files, env=env)
+try:
+run_linter('pylint', files, env=env)
+except subprocess.CalledProcessError:
+# pylint failure will be caught by diffing the IO.
+pass
print('=== mypy ===')
sys.stdout.flush()
-run_linter('mypy', files, env=env, suppress_output=True)
+try:
+run_linter('mypy', files, env=env, suppress_output=True)
+except subprocess.CalledProcessError as exc:
+if exc.output:
+print(exc.output)
iotests.script_main(main)
--
2.31.1
[PULL 04/22] iotests/297: Create main() function
Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.
(That environment setup will be moved over in forthcoming commits.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-5-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 15b54594c11..163ebc8ebfd 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -89,8 +89,12 @@ def run_linters():
print(p.stdout)
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+def main() -> None:
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
-iotests.script_main(run_linters)
+run_linters()
+
+
+iotests.script_main(main)
--
2.31.1
[PULL 06/22] iotests/297: Split run_linters apart into run_pylint and run_mypy
Move environment setup into main(), and split the actual linter
execution into run_pylint and run_mypy, respectively.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-7-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c1bddb9ce0e..189bcaf5f94 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
import shutil
import subprocess
import sys
-from typing import List
+from typing import List, Mapping, Optional
import iotests
@@ -61,23 +61,19 @@ def get_test_files() -> List[str]:
return list(filter(is_python_file, check_tests))
-def run_linters():
-files = get_test_files()
+def run_pylint(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-print('=== pylint ===')
-sys.stdout.flush()
-
-env = os.environ.copy()
subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
-print('=== mypy ===')
-sys.stdout.flush()
-env['MYPYPATH'] = env['PYTHONPATH']
+def run_mypy(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
@@ -94,7 +90,21 @@ def main() -> None:
if shutil.which(linter) is None:
iotests.notrun(f'{linter} not found')
-run_linters()
+files = get_test_files()
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+print('=== pylint ===')
+sys.stdout.flush()
+run_pylint(files, env=env)
+
+print('=== mypy ===')
+sys.stdout.flush()
+run_mypy(files, env=env)
iotests.script_main(main)
--
2.31.1
[PULL 03/22] iotests/297: Add get_files() function
Split out file discovery into its own method to begin separating out
configuration/setup and test execution.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-4-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b8101e6024a..15b54594c11 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
import shutil
import subprocess
import sys
+from typing import List
import iotests
@@ -54,10 +55,14 @@ def is_python_file(filename):
return False
-def run_linters():
+def get_test_files() -> List[str]:
named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-files = [filename for filename in check_tests if is_python_file(filename)]
+return list(filter(is_python_file, check_tests))
+
+
+def run_linters():
+files = get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
--
2.31.1
[PULL 02/22] iotests/297: Split mypy configuration out into mypy.ini
More separation of code and configuration.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-3-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 14 +-
tests/qemu-iotests/mypy.ini | 12
2 files changed, 13 insertions(+), 13 deletions(-)
create mode 100644 tests/qemu-iotests/mypy.ini
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bc3a0ceb2aa..b8101e6024a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,7 @@ def run_linters():
sys.stdout.flush()
env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-'--scripts-are-modules',
-*files),
+p = subprocess.run(('mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
new file mode 100644
index 000..4c0339f5589
--- /dev/null
+++ b/tests/qemu-iotests/mypy.ini
@@ -0,0 +1,12 @@
+[mypy]
+disallow_any_generics = True
+disallow_incomplete_defs = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+implicit_reexport = False
+namespace_packages = True
+no_implicit_optional = True
+scripts_are_modules = True
+warn_redundant_casts = True
+warn_unused_configs = True
+warn_unused_ignores = True
--
2.31.1
[PULL 01/22] iotests/297: Move pylint config into pylintrc
Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
configuration out of code, which I think is probably a good thing in
general.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-id: 20211019144918.3159078-2-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 4 +---
tests/qemu-iotests/pylintrc | 16
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91ec34d9521..bc3a0ceb2aa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -65,10 +65,8 @@ def run_linters():
print('=== pylint ===')
sys.stdout.flush()
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
env = os.environ.copy()
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+subprocess.run(('pylint-3', *files),
env=env, check=False)
print('=== mypy ===')
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8cb4e1d6a6d..32ab77b8bb9 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -31,6 +31,22 @@ disable=invalid-name,
too-many-statements,
consider-using-f-string,
+
+[REPORTS]
+
+# Activate the evaluation score.
+score=no
+
+
+[MISCELLANEOUS]
+
+# List of note tags to take in consideration, separated by a comma.
+# TODO notes are fine, but FIXMEs or XXXs should probably just be
+# fixed (in tests, at least).
+notes=FIXME,
+ XXX,
+
+
[FORMAT]
# Maximum number of characters on a single line.
--
2.31.1
[PULL 00/22] Python patches
The following changes since commit af531756d25541a1b3b3d9a14e72e7fedd941a2e: Merge remote-tracking branch 'remotes/philmd/tags/renesas-20211030' into staging (2021-10-30 11:31:41 -0700) are available in the Git repository at: https://gitlab.com/jsnow/qemu.git tags/python-pull-request for you to fetch changes up to 76cd358671e6b8e7c435ec65b1c44200254514a9: python, iotests: replace qmp with aqmp (2021-11-01 11:54:59 -0400) Pull request John Snow (22): iotests/297: Move pylint config into pylintrc iotests/297: Split mypy configuration out into mypy.ini iotests/297: Add get_files() function iotests/297: Create main() function iotests/297: Don't rely on distro-specific linter binaries iotests/297: Split run_linters apart into run_pylint and run_mypy iotests/297: refactor run_[mypy|pylint] as generic execution shim iotests/297: Change run_linter() to raise an exception on failure iotests/297: update tool availability checks iotests/297: split test into sub-cases iotests: split linters.py out from 297 iotests/linters: Add entry point for linting via Python CI iotests/linters: Add workaround for mypy bug #9852 python: Add iotest linters to test suite python/machine: remove has_quit argument python/machine: Handle QMP errors on close more meticulously python/aqmp: Remove scary message iotests: Accommodate async QMP Exception classes iotests: Conditionally silence certain AQMP errors iotests/300: avoid abnormal shutdown race condition python/aqmp: Create sync QMP wrapper for iotests python, iotests: replace qmp with aqmp python/qemu/aqmp/__init__.py | 12 -- python/qemu/aqmp/legacy.py| 138 ++ python/qemu/machine/machine.py| 85 + python/tests/iotests-mypy.sh | 4 + python/tests/iotests-pylint.sh| 4 + scripts/simplebench/bench_block_job.py| 3 +- tests/qemu-iotests/040| 7 +- tests/qemu-iotests/218| 2 +- tests/qemu-iotests/255| 2 +- tests/qemu-iotests/297| 103 +++- tests/qemu-iotests/300| 13 +- tests/qemu-iotests/iotests.py | 20 +++- tests/qemu-iotests/linters.py | 105 tests/qemu-iotests/mypy.ini | 12 ++ tests/qemu-iotests/pylintrc | 16 +++ tests/qemu-iotests/tests/mirror-top-perms | 17 ++- 16 files changed, 424 insertions(+), 119 deletions(-) create mode 100644 python/qemu/aqmp/legacy.py create mode 100755 python/tests/iotests-mypy.sh create mode 100755 python/tests/iotests-pylint.sh create mode 100644 tests/qemu-iotests/linters.py create mode 100644 tests/qemu-iotests/mypy.ini -- 2.31.1
Re: [PATCH v2 11/15] iotests: split linters.py out from 297
On Thu, Oct 28, 2021, 6:34 AM Hanna Reitz wrote:
> On 26.10.21 20:30, John Snow wrote:
> >
> >
> > On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz wrote:
> >
> > On 19.10.21 16:49, John Snow wrote:
> > > Now, 297 is just the iotests-specific incantations and
> > linters.py is as
> > > minimal as I can think to make it. The only remaining element in
> > here
> > > that ought to be configuration and not code is the list of skip
> > files,
> > > but they're still numerous enough that repeating them for mypy and
> > > pylint configurations both would be ... a hassle.
> > >
> > > Signed-off-by: John Snow
> > > ---
> > > tests/qemu-iotests/297| 72
> > +
> > > tests/qemu-iotests/linters.py | 76
> > +++
> > > 2 files changed, 87 insertions(+), 61 deletions(-)
> > > create mode 100644 tests/qemu-iotests/linters.py
> >
> > Reviewed-by: Hanna Reitz
> >
> >
> > Thanks!
> >
> > I wonder about `check_linter()`, though. By not moving it to
> > linters.py, we can’t use it in its entry point, and so the Python
> > test
> > infrastructure will have a strong dependency on these linters. Though
> > then again, it probably already does, and I suppose that’s one of the
> > points hindering us from running this from make check?
> >
> >
> > That one is left behind because it uses iotests API to skip a test.
> > Environment setup that guarantees we won't *need* to skip the test is
> > handled by the virtual environment setup magic in qemu/python/Makefile.
> >
> > Hanna
> >
> >
> > More info than you require:
> >
> > There's maybe about four ways you could run the python tests that
> > might make sense depending on who you are and what you're trying to
> > accomplish; they're documented in "make help" and again in
> > qemu/python/README.rst;
> >
> > (1) make check-dev -- makes a venv with whatever python you happen to
> > have, installs the latest packages, runs the tests
> > (2) make check-pipenv -- requires python 3.6 specifically, installs
> > the *oldest* packages, runs the tests
> > (3) make check-tox -- requires python 3.6 through 3.10, installs the
> > newest packages, runs the tests per each python version
> > (4) make check -- perform no setup at all, just run the tests in the
> > current environment. (Used directly by all three prior invocations)
>
> AFAIU these are all to be run in build/python? Isn’t any of them hooked
> up to the global `make check` in build? Because...
>
None of them are on make check, correct. Not yet. I'll try to make that
happen soon to drop 297.
They run out of the source tree directly, since they're checks on the
source and aren't actually related to a build of QEMU in any way.
(Y'know, yet. I'm not saying never.)
> In general, I assume that human beings that aren't working on Python
> > stuff actively will be using (1) at their interactive console, if they
> > decide to run any of these at all.
>
> ...I’m just running `make check` in the build directory.
>
Yeah, that's OK. I mean, I don't expect you to run them unless you were
submitting a series to specifically me.
("If they decide to run any of these at all" - I'm aware that very few
people would or are doing so. I consider the CI to be mostly for me as the
maintainer to make sure nothing regressed.)
> I would have hoped that this is hooked up to something that won’t fail
> because I don’t have some necessary tools installed or perhaps even
> because I have the wrong version of some tools installed (although the
> latter would be kind of questionable...). Would be nice if the global
> `make check` had a summary on what was skipped...
>
I mean. These targets shouldn't fail unless you're missing some really
basic requirements. They're just not hooked in to the big "make check" yet.
In terms of environment probing and skipping the tests, though, I do need
another layer somewhere to manage that. Stuff I'll need to put in
configure/meson etc. I have to look into it.
> > It imposes the least pre-requirements while still endeavoring to be a
> > target that will "just work".
> > Options (2) and (3) are what power the CI tests 'check-python-pipenv'
> > and 'check-python-tox', respectively; with(2) being the one that
> > actually gates the CI.
> > Option (4) is only really a convenience for bypassing the venv-setup
> > stuff i
Re: [PATCH v5 2/8] python/machine: Handle QMP errors on close more meticulously
On Thu, Oct 28, 2021, 6:01 AM Kevin Wolf wrote: > Am 27.10.2021 um 19:49 hat John Snow geschrieben: > > This reply is long, sorry. > > And after writing half of a very long reply myself, I noticed that I was > just very confused, so sorry for making you write a long text for no > real reason (well, at least for my first point - for the second one, > your explanation was very helpful, so thanks for that). > > Somehow I ended up completely ignoring the context of the code (it's run > as part of shutdown functions) and instead thought of the general > condition of QMP connections going away anywhere in the code. > > I suppose this could be a relevant concern in an actually asynchronous > client that may read from the socket (and encounter an error if the QEMU > process has already gone away and closed the connection) at any time, > but that's not what machine.py is meant for, at least not for now. > > So I'll just delete what I already wrote. This patch should be fine. > > Kevin > No problem. The cleanup path here is quite complex, so it wasn't clear that there *wasn't* a problem. I'd like to upgrade machine.py to use asyncio more natively for the console socket and qmp layers in the future and help break it apart into smaller pieces that are easier to reason about. You're right, though, if/when this part becomes async then needing more precision on when we mark a vm as defunct will become important. Thanks for looking at it! --js >
Re: [PATCH v5 0/8] Switch iotests to using Async QMP
On Thu, Oct 28, 2021 at 6:37 AM Kevin Wolf wrote: > Am 26.10.2021 um 19:56 hat John Snow geschrieben: > > GitLab: > https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper > > CI: https://gitlab.com/jsnow/qemu/-/pipelines/395925703 > > > > Hiya, > > > > This series continues where the last two AQMP series left off and adds a > > synchronous 'legacy' wrapper around the new AQMP interface, then drops > > it straight into iotests to prove that AQMP is functional and totally > > cool and fine. The disruption and churn to iotests is pretty minimal. > > > > In the event that a regression happens and I am not physically proximate > > to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable > > to any non-empty string as it pleases you to engage the QMP machinery > > you are used to. > > I obviously haven't reviewed systematically that AQMP is actually > correctly implemented and does what this series expects it to do, but > treating it as a black box should be good enough for this series: > > Reviewed-by: Kevin Wolf > Yeah. I've tested it "a lot" and I think it should work fine, it seems to work fine in practice, there's lots of unit tests for the core transport/async bits. And worst case, we can switch it back with a single line change. Thanks! --js
Re: [PATCH v5 6/8] iotests/300: avoid abnormal shutdown race condition
On Wed, Oct 27, 2021 at 8:56 AM Kevin Wolf wrote: > Am 26.10.2021 um 19:56 hat John Snow geschrieben: > > Wait for the destination VM to close itself instead of racing to shut it > > down first, which produces different error log messages from AQMP > > depending on precisely when we tried to shut it down. > > > > (For example: We may try to issue 'quit' immediately prior to the target > > VM closing its QMP socket, which will cause an ECONNRESET error to be > > logged. Waiting for the VM to exit itself avoids the race on shutdown > > behavior.) > > > > Reported-by: Hanna Reitz > > Signed-off-by: John Snow > > I think this will still expose the race I described in my comment on > patch 2. > > Kevin > > I wrote a long, meandering explanation of how I think things work in reply to that comment. TLDR: I didn't see the problem. Here in reply to this comment I'll just ask: what exactly *is* the race you see happening here even with this patch? I'm not sure I see it. --js
Re: [PATCH v5 2/8] python/machine: Handle QMP errors on close more meticulously
This reply is long, sorry. On Wed, Oct 27, 2021 at 7:19 AM Kevin Wolf wrote: > Am 26.10.2021 um 19:56 hat John Snow geschrieben: > > To use the AQMP backend, Machine just needs to be a little more diligent > > about what happens when closing a QMP connection. The operation is no > > longer a freebie in the async world; it may return errors encountered in > > the async bottom half on incoming message receipt, etc. > > > > (AQMP's disconnect, ultimately, serves as the quiescence point where all > > async contexts are gathered together, and any final errors reported at > > that point.) > > > > Because async QMP continues to check for messages asynchronously, it's > > almost certainly likely that the loop will have exited due to EOF after > > issuing the last 'quit' command. That error will ultimately be bubbled > > up when attempting to close the QMP connection. The manager class here > > then is free to discard it -- if it was expected. > > > > Signed-off-by: John Snow > > Reviewed-by: Hanna Reitz > > --- > > python/qemu/machine/machine.py | 48 +- > > 1 file changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > index 0bd40bc2f76..a0cf69786b4 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -342,9 +342,15 @@ def _post_shutdown(self) -> None: > > # Comprehensive reset for the failed launch case: > > self._early_cleanup() > > > > -if self._qmp_connection: > > -self._qmp.close() > > -self._qmp_connection = None > > +try: > > +self._close_qmp_connection() > > +except Exception as err: # pylint: disable=broad-except > > +LOG.warning( > > +"Exception closing QMP connection: %s", > > +str(err) if str(err) else type(err).__name__ > > +) > > +finally: > > +assert self._qmp_connection is None > > > > self._close_qemu_log_file() > > > > @@ -420,6 +426,31 @@ def _launch(self) -> None: > > close_fds=False) > > self._post_launch() > > > > +def _close_qmp_connection(self) -> None: > > +""" > > +Close the underlying QMP connection, if any. > > + > > +Dutifully report errors that occurred while closing, but assume > > +that any error encountered indicates an abnormal termination > > +process and not a failure to close. > > +""" > > +if self._qmp_connection is None: > > +return > > + > > +try: > > +self._qmp.close() > > +except EOFError: > > +# EOF can occur as an Exception here when using the Async > > +# QMP backend. It indicates that the server closed the > > +# stream. If we successfully issued 'quit' at any point, > > +# then this was expected. If the remote went away without > > +# our permission, it's worth reporting that as an abnormal > > +# shutdown case. > > +if not (self._user_killed or self._quit_issued): > > +raise > > Isn't this racy for those tests that expect QEMU to quit by itself and > then later call wait()? self._quit_issued is only set to True in wait(), > but whatever will cause QEMU to quit happens earlier and it might > actually quit before wait() is called. > _quit_issued is also set to True via qmp() and command(), but I think you're referring to cases where QEMU terminates for other reasons than an explicit command. So, yes, QEMU might indeed terminate/abort/quit before machine.py has recorded that fact somewhere. I wasn't aware of that being a problem. I suppose it'd be racy if, say, you orchestrated a "side-channel quit" and then didn't call wait() and instead called shutdown(). I think that's the case you're worried about? But, this code should ultimately be called in only four cases: (1) Connection failure (2) shutdown() (3) shutdown(), via wait() (4) shutdown(), via kill() I had considered it a matter of calling the correct exit path from the test code. If shutdown() does what the label on the tin says (it shuts down the VM), I actually would expect it to be racy if QEMU was in the middle of deconstructing itself. That's the belief behind changing around iotest 300 the way I did. > It would make sense to me that s
Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
On Tue, Oct 26, 2021 at 2:36 PM John Snow wrote: > > > On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz wrote: > >> On 19.10.21 16:49, John Snow wrote: >> > We need at least a tiny little shim here to join test file discovery >> > with test invocation. This logic could conceivably be hosted somewhere >> > in python/, but I felt it was strictly the least-rude thing to keep the >> > test logic here in iotests/, even if this small function isn't itself an >> > iotest. >> > >> > Note that we don't actually even need the executable bit here, we'll be >> > relying on the ability to run this module as a script using Python CLI >> > arguments. No chance it gets misunderstood as an actual iotest that way. >> > >> > (It's named, not in tests/, doesn't have the execute bit, and doesn't >> > have an execution shebang.) >> > >> > Signed-off-by: John Snow >> > --- >> > tests/qemu-iotests/linters.py | 27 +++ >> > 1 file changed, 27 insertions(+) >> >> Reviewed-by: Hanna Reitz >> >> > Thanks! I'll endeavor to try and clean up the list of exempted files to > continue cleaning up this mess, but it's not a top priority right now. I'll > put it on the backburner after I finish typing the QAPI generator. A lot of > the weird compatibility goop will go away over time as I consolidate more > of the venv logic. > > (I think this series is good to go, now? I think it could be applied in > any order vs my other series. If you want, if/when you give the go-ahead > for the other series, I could just stage them both myself and make sure > they work well together and save you the headache.) > Update: I pre-emptively staged both series (the iotests one first, followed by the AQMP one) to jsnow/python and verified that all of the python tests pass for each commit between: [14] python-add-iotest-linters-to # python: Add iotest linters to test suite ... [22] python-iotests-replace-qmp # python, iotests: replace qmp with aqmp and I'm running the CI on all of that now at https://gitlab.com/jsnow/qemu/-/pipelines/396002744 (I just wanted to double-check they didn't conflict with each other in any unanticipated ways. Let me know if I should send the PR or if that'll just create hassle for you.) --js
Re: [PATCH v2 0/2] pylint: fix new errors and warnings in qemu-iotests
On Mon, Oct 11, 2021 at 5:58 AM Emanuele Giuseppe Esposito < eespo...@redhat.com> wrote: > > > On 11/10/2021 11:29, Hanna Reitz wrote: > > On 08.10.21 08:28, Emanuele Giuseppe Esposito wrote: > >> There are some warnings and errors that we either miss or > >> are new in pylint. Anyways, test 297 of qemu-iotests fails > >> because of that, so we need to fix it. > >> > >> All these fixes involve just indentation or additional spaces > >> added. > >> > >> Signed-off-by: Emanuele Giuseppe Esposito > >> --- > >> v2: > >> * temporarly enable and then disable "bad whitespace" error in > >> image-fleecing > >> * better indentation for a fix in test 129 in patch one > > > > So the changes look good to me, but I can’t get my pylint to generate a > > bad-whitespace warning no matter how hard I try. (When you asked on IRC > > whether others see pylint warnings, I thought you meant the > > consider-using-f-string warnings that John disabled in > > 3765315d4c84f9c0799744f43a314169baaccc05.) > > > > I have pylint 2.11.1, which should be the newest version. So I tried to > > look around what might be the cause and found this: > > https://pylint.pycqa.org/en/latest/whatsnew/2.6.html – it seems that as > > of pylint 2.6, bad-whitespace warnings are no longer emitted. If that’s > > the reason why I can’t see the warning, then I think we should take only > > patch 1 (because it just makes sense), but skip patch 2. > > > > Yes you are right. I had 2.4.4, and now that I upgraded to 2.11.1 I > don't see bad-whitespace errors anymore (actually pylint does not seem > to complain at all). So I agree we can just take patch 1, as formatting > is wrong anyways. > > FWIW, the minimum version of pylint supported by the python/ tests is 2.8.0, for other reasons -- see commit b4d37d81. I no longer test or check for compatibility with older versions of pylint on any of our codebase. I've also authored a series to add iotest 297 to the Python CI directly, see https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg04329.html -- this gives a sense of "canonically supported linter versions" to that test. Currently, that support matrix is: Python 3.6 to Python 3.10 mypy >= 0.770 (Known to work with current latest, 0.910) pylint >= 2.8.0 (Known to work with current latest, 2.11.1) The "check-python-pipenv" test is the one Python CI test that will actually gate a build -- that test runs Python 3.6 with the oldest possible linter versions we support to ensure we don't accidentally start using new features. "check-python-tox" tests all python versions, 3.6 through 3.10, with bleeding edge packages. That CI test is "allowed to fail with a warning" and serves to alert me to new incompatible changes in updated Python dependencies. --js
Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > We need at least a tiny little shim here to join test file discovery > > with test invocation. This logic could conceivably be hosted somewhere > > in python/, but I felt it was strictly the least-rude thing to keep the > > test logic here in iotests/, even if this small function isn't itself an > > iotest. > > > > Note that we don't actually even need the executable bit here, we'll be > > relying on the ability to run this module as a script using Python CLI > > arguments. No chance it gets misunderstood as an actual iotest that way. > > > > (It's named, not in tests/, doesn't have the execute bit, and doesn't > > have an execution shebang.) > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/linters.py | 27 +++ > > 1 file changed, 27 insertions(+) > > Reviewed-by: Hanna Reitz > > Thanks! I'll endeavor to try and clean up the list of exempted files to continue cleaning up this mess, but it's not a top priority right now. I'll put it on the backburner after I finish typing the QAPI generator. A lot of the weird compatibility goop will go away over time as I consolidate more of the venv logic. (I think this series is good to go, now? I think it could be applied in any order vs my other series. If you want, if/when you give the go-ahead for the other series, I could just stage them both myself and make sure they work well together and save you the headache.) --js
Re: [PATCH v2 11/15] iotests: split linters.py out from 297
On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Now, 297 is just the iotests-specific incantations and linters.py is as > > minimal as I can think to make it. The only remaining element in here > > that ought to be configuration and not code is the list of skip files, > > but they're still numerous enough that repeating them for mypy and > > pylint configurations both would be ... a hassle. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297| 72 + > > tests/qemu-iotests/linters.py | 76 +++ > > 2 files changed, 87 insertions(+), 61 deletions(-) > > create mode 100644 tests/qemu-iotests/linters.py > > Reviewed-by: Hanna Reitz > > Thanks! > I wonder about `check_linter()`, though. By not moving it to > linters.py, we can’t use it in its entry point, and so the Python test > infrastructure will have a strong dependency on these linters. Though > then again, it probably already does, and I suppose that’s one of the > points hindering us from running this from make check? > > That one is left behind because it uses iotests API to skip a test. Environment setup that guarantees we won't *need* to skip the test is handled by the virtual environment setup magic in qemu/python/Makefile. > Hanna > More info than you require: There's maybe about four ways you could run the python tests that might make sense depending on who you are and what you're trying to accomplish; they're documented in "make help" and again in qemu/python/README.rst; (1) make check-dev -- makes a venv with whatever python you happen to have, installs the latest packages, runs the tests (2) make check-pipenv -- requires python 3.6 specifically, installs the *oldest* packages, runs the tests (3) make check-tox -- requires python 3.6 through 3.10, installs the newest packages, runs the tests per each python version (4) make check -- perform no setup at all, just run the tests in the current environment. (Used directly by all three prior invocations) In general, I assume that human beings that aren't working on Python stuff actively will be using (1) at their interactive console, if they decide to run any of these at all. It imposes the least pre-requirements while still endeavoring to be a target that will "just work". Options (2) and (3) are what power the CI tests 'check-python-pipenv' and 'check-python-tox', respectively; with(2) being the one that actually gates the CI. Option (4) is only really a convenience for bypassing the venv-setup stuff if you want to construct your own (or not use one at all). So the tests just assume that the tools they have available will work. It's kind of a 'power user' target. The way the CI works at the moment is that it uses a "fedora:latest" base image and installs python interpreters 3.6 through 3.10 inclusive, pipenv, and tox. From there, it has enough juice to run the makefile targets which take care of all of the actual linting dependencies and their different versions to get a wider matrix on the version testing to ensure we're still keeping compatibility with the 3.6 platform while also checking for new problems that show up on the bleeding edge. iotests 297 right now doesn't do any python environment setup at all, so we can't guarantee that the linters are present, so we decide to allow the test to be skipped. My big hesitation of adding this test directly into 'make check' is that I will need to do some environment probing to make sure that the 'pylint' version isn't too old -- or otherwise set up a venv in the build directory that can be used to run tests. I know we already set one up for the acceptance tests, so maybe there's an opportunity to re-use that venv, but I need to make sure that the dependencies between the two sets of tests are aligned. I don't know if they agree, currently. I think I probably want to minimize the number of different virtual python environments we create during the build, so I will look into what problems might exist in re-purposing the acceptance test venv. If that can get squared away easily, there's likely no real big barrier to adding more tests that rely on a python venv to get cooking during the normal build/check process, including iotest 297. --js
[PATCH v5 6/8] iotests/300: avoid abnormal shutdown race condition
Wait for the destination VM to close itself instead of racing to shut it
down first, which produces different error log messages from AQMP
depending on precisely when we tried to shut it down.
(For example: We may try to issue 'quit' immediately prior to the target
VM closing its QMP socket, which will cause an ECONNRESET error to be
logged. Waiting for the VM to exit itself avoids the race on shutdown
behavior.)
Reported-by: Hanna Reitz
Signed-off-by: John Snow
---
tests/qemu-iotests/300 | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 10f9f2a8da6..dbd28384ec3 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,8 +24,6 @@ import random
import re
from typing import Dict, List, Optional
-from qemu.machine import machine
-
import iotests
@@ -461,12 +459,11 @@ class
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
f"'{self.src_node_name}': Name is longer than 255 bytes",
log)
-# Expect abnormal shutdown of the destination VM because of
-# the failed migration
-try:
-self.vm_b.shutdown()
-except machine.AbnormalShutdown:
-pass
+# Destination VM will terminate w/ error of its own accord
+# due to the failed migration.
+self.vm_b.wait()
+rc = self.vm_b.exitcode()
+assert rc is not None and rc > 0
def test_aliased_bitmap_name_too_long(self) -> None:
# Longer than the maximum for bitmap names
--
2.31.1
Re: [PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure
On Tue, Oct 26, 2021 at 6:10 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Instead of using a process return code as the python function return > > value (or just not returning anything at all), allow run_linter() to > > raise an exception instead. > > > > The responsibility for printing output on error shifts from the function > > itself to the caller, who will know best how to present/format that > > information. (Also, "suppress_output" is now a lot more accurate of a > > parameter name.) > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297 | 24 ++-- > > 1 file changed, 14 insertions(+), 10 deletions(-) > > Thanks! :) > > Reviewed-by: Hanna Reitz > > No problem. Thanks for taking the time to review it! --js
[PATCH v5 7/8] python/aqmp: Create sync QMP wrapper for iotests
This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.
It does not, however, attempt to mimic Exception compatibility.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/legacy.py | 138 +
1 file changed, 138 insertions(+)
create mode 100644 python/qemu/aqmp/legacy.py
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+ wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
--
2.31.1
[PATCH v5 5/8] iotests: Conditionally silence certain AQMP errors
AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.
For example, during a failed QMP connection attempt, we might see:
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError
This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.
For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.
(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 20 +++-
tests/qemu-iotests/tests/mirror-top-perms | 12
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
import subprocess
import sys
import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
import unittest
@@ -114,6 +114,24 @@
sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
def unarchive_sample_image(sample, fname):
sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
from qemu.qmp import QMPConnectError
import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
- 'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+ 'this should not have happened')
except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
--
2.31.1
[PATCH v5 2/8] python/machine: Handle QMP errors on close more meticulously
To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.
(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)
Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
python/qemu/machine/machine.py | 48 +-
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
# Comprehensive reset for the failed launch case:
self._early_cleanup()
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err: # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
self._close_qemu_log_file()
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
self._post_launch()
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
def _early_cleanup(self) -> None:
"""
Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
self._early_cleanup()
if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
--
2.31.1
Re: [PATCH v2 10/15] iotests/297: split test into sub-cases
On Tue, Oct 26, 2021 at 6:29 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Take iotest 297's main() test function and split it into two sub-cases > > that can be skipped individually. We can also drop custom environment > > setup from the pylint test as it isn't needed. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297 | 63 ++ > > 1 file changed, 39 insertions(+), 24 deletions(-) > > Reviewed-by: Hanna Reitz > > (while heavily scratching myself to ease the itch to turn this into a > unit test now) > > (I strongly considered it, but didn't want to add yet-more-rewrites into this series. If the ultimate fate is to sunset this particular iotest, I didn't see a big benefit to doing it.)
[PATCH v5 4/8] iotests: Accommodate async QMP Exception classes
(But continue to support the old ones for now, too.)
There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:
- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
scripts/simplebench/bench_block_job.py| 3 ++-
tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
import os
-from qemu import qmp
+from qemu.aqmp import ConnectError
from qemu.machine import machine
+from qemu.qmp import QMPConnectError
import iotests
from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PATCH v5 3/8] python/aqmp: Remove scary message
The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/__init__.py | 12
1 file changed, 12 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
# the COPYING file in the top-level directory.
import logging
-import warnings
from .error import AQMPError
from .events import EventListener
@@ -31,17 +30,6 @@
from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
# Suppress logging unless an application engages it.
logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
--
2.31.1
[PATCH v5 1/8] python/machine: remove has_quit argument
If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.
The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.
In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
python/qemu/machine/machine.py | 34 +++---
tests/qemu-iotests/040 | 7 +--
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/255 | 2 +-
4 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
self._user_killed = False
+self._quit_issued = False
def __enter__(self: _T) -> _T:
return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
command = ''
LOG.warning(msg, -int(exitcode), command)
+self._quit_issued = False
self._user_killed = False
self._launched = False
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
self._subp.kill()
self._subp.wait(timeout=60)
-def _soft_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
"""
Perform early cleanup, attempt to gracefully shut down the VM, and wait
for it to terminate.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise ConnectionReset: On QMP communication errors
:raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
self._early_cleanup()
if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
# Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
"""
Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise AbnormalShutdown: When the VM could not be shut down gracefully.
The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
may result in its own exceptions, likely subprocess.TimeoutExpired.
"""
try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
except Exception as exc:
self._hard_shutdown()
raise AbnormalShutdown("Could not perform graceful shutdown") \
from exc
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
hard: bool = False,
timeout: Optional[int] = 30) -> None:
"""
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
If the VM has not yet been launched, or shutdown(), wait(), or kill()
have already been called, this method does nothing.
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
:param hard: When true, do not attempt graceful shutdown, and
suppress the SIGKILL warning log message.
:param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
self._user_killed = True
self._hard_shutdown()
else:
-self._do_shutdown(timeout, has_quit)
+self._do_shutdown(timeout)
finally:
self._post_shutdown()
@@ -529,7 +526,8 @@ def wait(se
[PATCH v5 0/8] Switch iotests to using Async QMP
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper CI: https://gitlab.com/jsnow/qemu/-/pipelines/395925703 Hiya, This series continues where the last two AQMP series left off and adds a synchronous 'legacy' wrapper around the new AQMP interface, then drops it straight into iotests to prove that AQMP is functional and totally cool and fine. The disruption and churn to iotests is pretty minimal. In the event that a regression happens and I am not physically proximate to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable to any non-empty string as it pleases you to engage the QMP machinery you are used to. V5: 006: Fixed a typing error. (Hanna) V4: 006: Added to address a race condition in iotest 300. (Thanks for the repro, Hanna) V3: Near as we can tell, our immediate ancestral forebear. V2: A distant dream, half-remembered. V1: Apocrypha. John Snow (8): python/machine: remove has_quit argument python/machine: Handle QMP errors on close more meticulously python/aqmp: Remove scary message iotests: Accommodate async QMP Exception classes iotests: Conditionally silence certain AQMP errors iotests/300: avoid abnormal shutdown race condition python/aqmp: Create sync QMP wrapper for iotests python, iotests: replace qmp with aqmp python/qemu/aqmp/__init__.py | 12 -- python/qemu/aqmp/legacy.py| 138 ++ python/qemu/machine/machine.py| 85 + scripts/simplebench/bench_block_job.py| 3 +- tests/qemu-iotests/040| 7 +- tests/qemu-iotests/218| 2 +- tests/qemu-iotests/255| 2 +- tests/qemu-iotests/300| 13 +- tests/qemu-iotests/iotests.py | 20 +++- tests/qemu-iotests/tests/mirror-top-perms | 17 ++- 10 files changed, 243 insertions(+), 56 deletions(-) create mode 100644 python/qemu/aqmp/legacy.py -- 2.31.1
[PATCH v5 8/8] python, iotests: replace qmp with aqmp
Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.
Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementation, proving that both implementations work
concurrently.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
)
from qemu.qmp import ( # pylint: disable=import-error
-QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
SocketAddrT,
@@ -50,6 +49,12 @@
from . import console_socket
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
LOG = logging.getLogger(__name__)
--
2.31.1
Re: [PATCH v4 6/8] iotests/300: avoid abnormal shutdown race condition
On Mon, Oct 25, 2021 at 9:20 AM Hanna Reitz wrote:
> On 13.10.21 23:57, John Snow wrote:
> > Wait for the destination VM to close itself instead of racing to shut it
> > down first, which produces different error log messages from AQMP
> > depending on precisely when we tried to shut it down.
> >
> > (For example: We may try to issue 'quit' immediately prior to the target
> > VM closing its QMP socket, which will cause an ECONNRESET error to be
> > logged. Waiting for the VM to exit itself avoids the race on shutdown
> > behavior.)
> >
> > Reported-by: Hanna Reitz
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/300 | 12
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> > index 10f9f2a8da6..bbea7248005 100755
> > --- a/tests/qemu-iotests/300
> > +++ b/tests/qemu-iotests/300
> > @@ -24,8 +24,6 @@ import random
> > import re
> > from typing import Dict, List, Optional
> >
> > -from qemu.machine import machine
> > -
> > import iotests
> >
> >
> > @@ -461,12 +459,10 @@ class
> TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
> > f"'{self.src_node_name}': Name is longer than
> 255 bytes",
> > log)
> >
> > -# Expect abnormal shutdown of the destination VM because of
> > -# the failed migration
> > -try:
> > -self.vm_b.shutdown()
> > -except machine.AbnormalShutdown:
> > -pass
> > +# Destination VM will terminate w/ error of its own accord
> > +# due to the failed migration.
> > +self.vm_b.wait()
> > +assert self.vm_b.exitcode() > 0
>
> Trying to test, I can see that this fails iotest 297, because
> `.exitcode()` is `Optional[int]`...
>
> (I can’t believe how long it took me to figure this out – the message
> “300:465: Unsupported operand types for < ("int" and "None")” made me
> believe that it was 300 that was failing, because `exitcode()` was
> returning `None` for some inconceivable reason. I couldn’t understand
> why my usual test setup failed on every run, but I couldn’t get 300 to
> fail manually... Until I noticed that the message came below the “297”
> line, not the “300” line...)
>
>
Oops. Is there anything we can do to improve the visual clarity there?
> Hanna
>
>
Embarrassing. I scrutinized the other series I sent out, but forgot to
apply the same tests to this one. :(
Fixed, sorry for the noise.
--js
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
On Tue, Oct 26, 2021 at 3:56 AM Markus Armbruster wrote: > John Snow writes: > > > On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster > wrote: > > > >> Add special feature 'unstable' everywhere the name starts with 'x-', > >> except for InputBarrierProperties member x-origin and > >> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, > >> because these two are actually stable. > >> > >> Signed-off-by: Markus Armbruster > >> --- > >> qapi/block-core.json | 123 +++ > >> qapi/migration.json | 35 +--- > >> qapi/misc.json | 6 ++- > >> qapi/qom.json| 11 ++-- > >> 4 files changed, 130 insertions(+), 45 deletions(-) > >> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> index 6d3217abb6..ce2c1352cb 100644 > >> --- a/qapi/block-core.json > >> +++ b/qapi/block-core.json > >> @@ -1438,6 +1438,9 @@ > >> # > >> # @x-perf: Performance options. (Since 6.0) > >> # > >> +# Features: > >> +# @unstable: Member @x-perf is experimental. > >> +# > >> > > > > It'd be a lot cooler if we could annotate the unstable member directly > > instead of confusing it with the syntax that might describe the entire > > struct/union/command/etc, but ... eh, it's just a doc field, so I'm not > > gonna press on this. I don't have the energy to get into a doc formatting > > standard discussion right now, so: sure, why not? > > By design, we have a single doc comment block for the entire definition. > > When Kevin invented feature flags (merge commit 4747524f9f2), he added > them just to struct types. He added "feature sections" for documenting > features. It mirrors the "argument sections" for documenting members. > Makes sense. > > Aside: he neglected to update qapi-code-gen.rst section "Definition > documentation", and I failed to catch it. I'll make up for it. > > Peter and I then added feature flags to the remaining definitions > (commit 23394b4c39 and 013b4efc9b). Feature sections make sense there, > too. > > I then added them to struct members (commit 84ab008687). Instead of > doing something fancy for documenting feature flags of members, I simply > used the existing feature sections. This conflates member features with > struct features. > > Yeah, that's the part I don't like. If this isn't the first instance of breaking the seal, though, this is the wrong time for me to comment on it. Don't worry about it for this series. > What could "something fancy" be? All we have for members is "argument > sections", which are basically a name plus descriptive text. We'd have > to add structure to that, say nest feature sections into argument > sections. I have no appetite for that right now. > > (Tangent below, unrelated to acceptance of this series) Yeah, I don't have an appetite for it at the moment either. I'll have to read Marc-Andre's recent sphinx patches and see if I want to do more work -- I had a bigger prototype a few months back I didn't bring all the way home, but I am still thinking about reworking our QAPIDoc format. It's ... well. I don't really want to, but I am not sure how else to bring some of the features I want home, and I think I need some more time to think carefully through exactly what I want to do and why. I wouldn't mind chatting about it with you sometime soon -- there's a few things to balance here, such as: (1) Reworking the doc format would be an obnoxious amount of churn, ... (2) ...but the Sphinx internals are really not meant to be used the way we're using them right now, ... (3) ...but I also don't want to write our QAPI docstrings in a way that *targets* Sphinx. Not that I think we'll be dropping it any time soon, but it still feels like the wrong idea to tie them so closely together. I'm hoping there's an opportunity to make the parsing nicer with minimal changes to the parsing and format, though. It just might require enforcing a *pinch* more structure. I could see how I feel about per-field annotations at that point. I consider them interesting for things like the Python SDK where I may want to enable/disable warnings for using deprecated stuff at the client-level. (e.g., let's say you're using Python SDK 6.2 to talk to a 6.1 client. Nothing stops you from doing this, but some commands will simply be rejected by QEMU as it won't know what you're talking about. Using deprecated fields or commands to talk to an older client will produce no visible warning from QEMU either, as it wasn't deprecated at that point in time. Still, the client may wish to know that they're asking for f
Re: [PATCH 7/9] qapi: Generalize enum member policy checking
On Tue, Oct 26, 2021 at 5:43 AM Markus Armbruster wrote:
> John Snow writes:
>
> > On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster
> wrote:
> >
> >> The code to check enumeration value policy can see special feature
> >> flag 'deprecated' in QEnumLookup member flags[value]. I want to make
> >> feature flag 'unstable' visible there as well, so I can add policy for
> >> it.
> >>
> >> Instead of extending flags[], replace it by @special_features (a
> >> bitset of QapiSpecialFeature), because that's how special features get
> >> passed around elsewhere.
> >>
> >> Signed-off-by: Markus Armbruster
> >> ---
> >> include/qapi/util.h| 5 +
> >> qapi/qapi-visit-core.c | 3 ++-
> >> scripts/qapi/types.py | 22 --
> >> 3 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/qapi/util.h b/include/qapi/util.h
> >> index 7a8d5c7d72..0cc98db9f9 100644
> >> --- a/include/qapi/util.h
> >> +++ b/include/qapi/util.h
> >> @@ -15,12 +15,9 @@ typedef enum {
> >> QAPI_DEPRECATED,
> >> } QapiSpecialFeature;
> >>
> >> -/* QEnumLookup flags */
> >> -#define QAPI_ENUM_DEPRECATED 1
> >> -
> >> typedef struct QEnumLookup {
> >> const char *const *array;
> >> -const unsigned char *const flags;
> >> +const unsigned char *const special_features;
> >> const int size;
> >> } QEnumLookup;
> >>
> >> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> >> index b4a81f1757..5572d90efb 100644
> >> --- a/qapi/qapi-visit-core.c
> >> +++ b/qapi/qapi-visit-core.c
> >> @@ -407,7 +407,8 @@ static bool input_type_enum(Visitor *v, const char
> >> *name, int *obj,
> >> return false;
> >> }
> >>
> >> -if (lookup->flags && (lookup->flags[value] &
> QAPI_ENUM_DEPRECATED)) {
> >> +if (lookup->special_features
> >> +&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
> >> switch (v->compat_policy.deprecated_input) {
> >> case COMPAT_POLICY_INPUT_ACCEPT:
> >> break;
> >> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> >> index ab2441adc9..3013329c24 100644
> >> --- a/scripts/qapi/types.py
> >> +++ b/scripts/qapi/types.py
> >> @@ -16,7 +16,7 @@
> >> from typing import List, Optional
> >>
> >> from .common import c_enum_const, c_name, mcgen
> >> -from .gen import QAPISchemaModularCVisitor, ifcontext
> >> +from .gen import QAPISchemaModularCVisitor, gen_special_features,
> >> ifcontext
> >> from .schema import (
> >> QAPISchema,
> >> QAPISchemaEnumMember,
> >> @@ -39,7 +39,7 @@ def gen_enum_lookup(name: str,
> >> members: List[QAPISchemaEnumMember],
> >> prefix: Optional[str] = None) -> str:
> >> max_index = c_enum_const(name, '_MAX', prefix)
> >> -flags = ''
> >> +feats = ''
> >> ret = mcgen('''
> >>
> >> const QEnumLookup %(c_name)s_lookup = {
> >> @@ -54,19 +54,21 @@ def gen_enum_lookup(name: str,
> >> ''',
> >> index=index, name=memb.name)
> >> ret += memb.ifcond.gen_endif()
> >> -if 'deprecated' in (f.name for f in memb.features):
> >> -flags += mcgen('''
> >> -[%(index)s] = QAPI_ENUM_DEPRECATED,
> >> -''',
> >> - index=index)
> >>
> >> -if flags:
> >> +special_features = gen_special_features(memb.features)
> >> +if special_features != '0':
> >>
> >
> > Though, I have to admit the common reoccurrence of this pattern suggests
> to
> > me that gen_special_features really ought to be returning something
> false-y
> > in these cases. Something about testing for the empty case with something
> > that represents, but isn't empty, gives me a brief pause.
> >
> > Not willing to wage war over it.
>
> I habitually start stupid, and when stupid confuses or annoys me later,
> I smarten it up some.
>
> Let's see how this instance of "stupid" ages, okay?
>
>
"I see what you're saying, but meh" is a relatable feeling ;)
>
> >> +feats += mcgen('''
> >> +[%(index)s] = %(special_features)s,
> >> +''',
> >> + index=index,
> special_features=special_features)
> >> +
> >> +if feats:
> >> ret += mcgen('''
> >> },
> >> -.flags = (const unsigned char[%(max_index)s]) {
> >> +.special_features = (const unsigned char[%(max_index)s]) {
> >> ''',
> >> max_index=max_index)
> >> -ret += flags
> >> +ret += feats
> >>
> >> ret += mcgen('''
> >> },
> >> --
> >> 2.31.1
> >>
> >>
> > Python bits: Acked-by: John Snow
>
> Thanks!
>
>
Re: [PATCH 7/9] qapi: Generalize enum member policy checking
On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster wrote:
> The code to check enumeration value policy can see special feature
> flag 'deprecated' in QEnumLookup member flags[value]. I want to make
> feature flag 'unstable' visible there as well, so I can add policy for
> it.
>
> Instead of extending flags[], replace it by @special_features (a
> bitset of QapiSpecialFeature), because that's how special features get
> passed around elsewhere.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/util.h| 5 +
> qapi/qapi-visit-core.c | 3 ++-
> scripts/qapi/types.py | 22 --
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 7a8d5c7d72..0cc98db9f9 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -15,12 +15,9 @@ typedef enum {
> QAPI_DEPRECATED,
> } QapiSpecialFeature;
>
> -/* QEnumLookup flags */
> -#define QAPI_ENUM_DEPRECATED 1
> -
> typedef struct QEnumLookup {
> const char *const *array;
> -const unsigned char *const flags;
> +const unsigned char *const special_features;
> const int size;
> } QEnumLookup;
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index b4a81f1757..5572d90efb 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -407,7 +407,8 @@ static bool input_type_enum(Visitor *v, const char
> *name, int *obj,
> return false;
> }
>
> -if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
> +if (lookup->special_features
> +&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
> switch (v->compat_policy.deprecated_input) {
> case COMPAT_POLICY_INPUT_ACCEPT:
> break;
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index ab2441adc9..3013329c24 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -16,7 +16,7 @@
> from typing import List, Optional
>
> from .common import c_enum_const, c_name, mcgen
> -from .gen import QAPISchemaModularCVisitor, ifcontext
> +from .gen import QAPISchemaModularCVisitor, gen_special_features,
> ifcontext
> from .schema import (
> QAPISchema,
> QAPISchemaEnumMember,
> @@ -39,7 +39,7 @@ def gen_enum_lookup(name: str,
> members: List[QAPISchemaEnumMember],
> prefix: Optional[str] = None) -> str:
> max_index = c_enum_const(name, '_MAX', prefix)
> -flags = ''
> +feats = ''
> ret = mcgen('''
>
> const QEnumLookup %(c_name)s_lookup = {
> @@ -54,19 +54,21 @@ def gen_enum_lookup(name: str,
> ''',
> index=index, name=memb.name)
> ret += memb.ifcond.gen_endif()
> -if 'deprecated' in (f.name for f in memb.features):
> -flags += mcgen('''
> -[%(index)s] = QAPI_ENUM_DEPRECATED,
> -''',
> - index=index)
>
> -if flags:
> +special_features = gen_special_features(memb.features)
> +if special_features != '0':
>
Though, I have to admit the common reoccurrence of this pattern suggests to
me that gen_special_features really ought to be returning something false-y
in these cases. Something about testing for the empty case with something
that represents, but isn't empty, gives me a brief pause.
Not willing to wage war over it.
> +feats += mcgen('''
> +[%(index)s] = %(special_features)s,
> +''',
> + index=index, special_features=special_features)
> +
> +if feats:
> ret += mcgen('''
> },
> -.flags = (const unsigned char[%(max_index)s]) {
> +.special_features = (const unsigned char[%(max_index)s]) {
> ''',
> max_index=max_index)
> -ret += flags
> +ret += feats
>
> ret += mcgen('''
> },
> --
> 2.31.1
>
>
Python bits: Acked-by: John Snow
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'. This makes the feature flag visible to the actual
> visitors. I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument. Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/visitor-impl.h | 6 --
> include/qapi/visitor.h| 17 +
> qapi/qapi-forward-visitor.c | 16 +---
> qapi/qapi-visit-core.c| 22 --
> qapi/qobject-input-visitor.c | 15 ++-
> qapi/qobject-output-visitor.c | 9 ++---
> qapi/trace-events | 4 ++--
> scripts/qapi/visit.py | 14 +++---
> 8 files changed, 63 insertions(+), 40 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 72b6537bef..2badec5ba4 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -114,10 +114,12 @@ struct Visitor
> void (*optional)(Visitor *v, const char *name, bool *present);
>
> /* Optional */
> -bool (*deprecated_accept)(Visitor *v, const char *name, Error **errp);
> +bool (*policy_reject)(Visitor *v, const char *name,
> + unsigned special_features, Error **errp);
>
> /* Optional */
> -bool (*deprecated)(Visitor *v, const char *name);
> +bool (*policy_skip)(Visitor *v, const char *name,
> +unsigned special_features);
>
> /* Must be set */
> VisitorType type;
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index dcb96018a9..d53a84c9ba 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -461,22 +461,31 @@ void visit_end_alternate(Visitor *v, void **obj);
> bool visit_optional(Visitor *v, const char *name, bool *present);
>
> /*
> - * Should we reject deprecated member @name?
> + * Should we reject member @name due to policy?
> + *
> + * @special_features is the member's special features encoded as a
> + * bitset of QapiSpecialFeature.
> *
> * @name must not be NULL. This function is only useful between
> * visit_start_struct() and visit_end_struct(), since only objects
> * have deprecated members.
> */
> -bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp);
> +bool visit_policy_reject(Visitor *v, const char *name,
> + unsigned special_features, Error **errp);
>
> /*
> - * Should we visit deprecated member @name?
> + *
> + * Should we skip member @name due to policy?
> + *
> + * @special_features is the member's special features encoded as a
> + * bitset of QapiSpecialFeature.
> *
> * @name must not be NULL. This function is only useful between
> * visit_start_struct() and visit_end_struct(), since only objects
> * have deprecated members.
> */
> -bool visit_deprecated(Visitor *v, const char *name);
> +bool visit_policy_skip(Visitor *v, const char *name,
> + unsigned special_features);
>
> /*
> * Set policy for handling deprecated management interfaces.
> diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
> index a4b111e22a..25d098aa8a 100644
> --- a/qapi/qapi-forward-visitor.c
> +++ b/qapi/qapi-forward-visitor.c
> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const
> char *name, bool *present)
> visit_optional(ffv->target, name, present);
> }
>
> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool forward_field_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, , errp)) {
> return false;
> }
> -return visit_deprecated_accept(ffv->target, name, errp);
> +return visit_policy_reject(ffv->target, name, special_features, errp);
> }
>
> -static bool forward_field_deprecated(Visitor *v, const char *name)
> +static bool forward_field_policy_skip(Visitor *v, const char *name,
> + unsigned special_features)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, , NULL)) {
> return false;
> }
> -return visit_deprecated(ffv->target, name);
> +return visit_policy_skip(ffv->target, name, special_features);
> }
>
> static void forward_field_complete(Visitor *v, void *opaque)
> @@ -313,8
Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces
> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
> +"Policy for handling unstable management
> interfaces\n",
> QEMU_ARCH_ALL)
> SRST
> ``-compat
> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
> @@ -3659,6 +3661,22 @@ SRST
> Suppress deprecated command results and events
>
> Limitation: covers only syntactic aspects of QMP.
> +
> +``-compat
> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
> +Set policy for handling unstable management interfaces (experimental):
> +
> +``unstable-input=accept`` (default)
> +Accept unstable commands and arguments
> +``unstable-input=reject``
> +Reject unstable commands and arguments
> +``unstable-input=crash``
> +Crash on unstable commands and arguments
> +``unstable-output=accept`` (default)
> +Emit unstable command results and events
> +``unstable-output=hide``
> +Suppress unstable command results and events
> +
> +Limitation: covers only syntactic aspects of QMP.
> ERST
>
> DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 82475e84ec..27b44c49f5 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -109,13 +109,15 @@ def gen_event_send(name: str,
> if not boxed:
> ret += gen_param_var(arg_type)
>
> -if 'deprecated' in [f.name for f in features]:
> -ret += mcgen('''
> +for f in features:
> +if f.is_special():
> +ret += mcgen('''
>
> -if (compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) {
> +if (compat_policy.%(feat)s_output == COMPAT_POLICY_OUTPUT_HIDE) {
> return;
> }
> -''')
> +''',
> + feat=f.name)
>
> ret += mcgen('''
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 55f82d7389..b7b3fc0ce4 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -254,9 +254,11 @@ def doc_type(self):
>
> def check(self, schema):
> QAPISchemaEntity.check(self, schema)
> -if 'deprecated' in [f.name for f in self.features]:
> -raise QAPISemError(
> -self.info, "feature 'deprecated' is not supported for
> types")
> +for feat in self.features:
> +if feat.is_special():
> +raise QAPISemError(
> +self.info,
> +f"feature '{feat.name}' is not supported for types")
>
> def describe(self):
> assert self.meta
> @@ -726,7 +728,7 @@ class QAPISchemaFeature(QAPISchemaMember):
> role = 'feature'
>
> def is_special(self):
> -return self.name in ('deprecated')
> +return self.name in ('deprecated', 'unstable')
>
>
> class QAPISchemaObjectTypeMember(QAPISchemaMember):
> --
> 2.31.1
>
>
Python bits: Acked-by: John Snow
Looks good overall from what I can see, minor style quibbles that I'd
probably fold on if you frowned at me.
--js
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote: > The code to check policy for handling deprecated input is triplicated. > Factor it out into compat_policy_input_ok() before I mess with it in > the next commit. > > Signed-off-by: Markus Armbruster > (Skipping C-only patches for quick review. I'll trust you on these.) --js
Re: [PATCH 6/9] qapi: Generalize command policy checking
u-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index 10a1a33761..52cf17e8ac 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -146,7 +146,8 @@ static void init_qmp_commands(void)
>
> QTAILQ_INIT(_cap_negotiation_commands);
> qmp_register_command(_cap_negotiation_commands,
> "qmp_capabilities",
> - qmp_marshal_qmp_capabilities,
> QCO_ALLOW_PRECONFIG);
> + qmp_marshal_qmp_capabilities,
> + QCO_ALLOW_PRECONFIG, 0);
> }
>
> static int getopt_set_loc(int argc, char **argv, const char *optstring,
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index c8a975528f..21001bbd6b 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -26,6 +26,7 @@
> QAPISchemaModularCVisitor,
> build_params,
> ifcontext,
> +gen_special_features,
> )
> from .schema import (
> QAPISchema,
> @@ -217,9 +218,6 @@ def gen_register_command(name: str,
> coroutine: bool) -> str:
> options = []
>
> -if 'deprecated' in [f.name for f in features]:
> -options += ['QCO_DEPRECATED']
> -
> if not success_response:
> options += ['QCO_NO_SUCCESS_RESP']
> if allow_oob:
> @@ -231,10 +229,11 @@ def gen_register_command(name: str,
>
> ret = mcgen('''
> qmp_register_command(cmds, "%(name)s",
> - qmp_marshal_%(c_name)s, %(opts)s);
> + qmp_marshal_%(c_name)s, %(opts)s, %(feats)s);
> ''',
> name=name, c_name=c_name(name),
> -opts=' | '.join(options) or 0)
> +opts=' | '.join(options) or 0,
> +feats=gen_special_features(features))
>
Ah, you use the '0' return here. Alright then.
> return ret
>
>
> --
> 2.31.1
>
>
Python bits: Acked-by: John Snow
C bits: "I believe in my heart that they probably work."
(for this and previous patch.)
Re: [PATCH 4/9] qapi: Tools for sets of special feature flags in generated code
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
> New enum QapiSpecialFeature enumerates the special feature flags.
>
> New helper gen_special_features() returns code to represent a
> collection of special feature flags as a bitset.
>
> The next few commits will put them to use.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/util.h| 4
> scripts/qapi/gen.py| 13 +
> scripts/qapi/schema.py | 3 +++
> 3 files changed, 20 insertions(+)
>
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 257c600f99..7a8d5c7d72 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -11,6 +11,10 @@
> #ifndef QAPI_UTIL_H
> #define QAPI_UTIL_H
>
> +typedef enum {
> +QAPI_DEPRECATED,
> +} QapiSpecialFeature;
> +
> /* QEnumLookup flags */
> #define QAPI_ENUM_DEPRECATED 1
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 2ec1e7b3b6..9d07b88cf6 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -29,6 +29,7 @@
> mcgen,
> )
> from .schema import (
> +QAPISchemaFeature,
> QAPISchemaIfCond,
> QAPISchemaModule,
> QAPISchemaObjectType,
> @@ -37,6 +38,18 @@
> from .source import QAPISourceInfo
>
>
> +def gen_special_features(features: QAPISchemaFeature):
> +ret = ''
> +sep = ''
> +
> +for feat in features:
> +if feat.is_special():
> +ret += ('%s1u << QAPI_%s' % (sep, feat.name.upper()))
>
Building the constant name here "feels" fragile, but I'll trust that the
test suite and/or the compiler will catch us if we accidentally goof up
this mapping. In the interest of simplicity, then, "sure, why not."
> +sep = ' | '
> +
>
+return ret or '0'
> +
>
Subjectively more pythonic:
special_features = [f"1u << QAPI_{feat.name.upper()}" for feat in features
if feat.is_special()]
ret = ' | '.join(special_features)
return ret or '0'
A bit more dense, but more functional. Up to you, but I find join() easier
to read and reason about for the presence of separators.
> +
> class QAPIGen:
> def __init__(self, fname: str):
> self.fname = fname
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 6d5f46509a..55f82d7389 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -725,6 +725,9 @@ def connect_doc(self, doc):
> class QAPISchemaFeature(QAPISchemaMember):
> role = 'feature'
>
> +def is_special(self):
> +return self.name in ('deprecated')
> +
>
alrighty.
(Briefly wondered: is it worth naming special features as a property of the
class, but with only two names, it's probably fine enough to leave it
embedded in the method logic. Only a style thing and doesn't have any
actual impact that I can imagine, so ... nevermind.)
>
> class QAPISchemaObjectTypeMember(QAPISchemaMember):
> def __init__(self, name, info, typ, optional, ifcond=None,
> features=None):
> --
> 2.31.1
>
>
Well, either way:
Reviewed-by: John Snow
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
(Since 5.2)
> #
> +# Features:
> +# @unstable: Member @x-checkpoint-delay is experimental.
> +#
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> @@ -1119,7 +1137,8 @@
> '*tls-authz': 'str',
> '*max-bandwidth': 'size',
> '*downtime-limit': 'uint64',
> -'*x-checkpoint-delay': 'uint32',
> +'*x-checkpoint-delay': { 'type': 'uint32',
> + 'features': [ 'unstable' ] },
> '*block-incremental': 'bool',
> '*multifd-channels': 'uint8',
> '*xbzrle-cache-size': 'size',
> @@ -1351,6 +1370,9 @@
> # If sent to the Secondary, the Secondary side will run failover work,
> # then takes over server operation to become the service VM.
> #
> +# Features:
> +# @unstable: This command is experimental.
> +#
> # Since: 2.8
> #
> # Example:
> @@ -1359,7 +1381,8 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'x-colo-lost-heartbeat' }
> +{ 'command': 'x-colo-lost-heartbeat',
> + 'features': [ 'unstable' ] }
>
> ##
> # @migrate_cancel:
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 5c2ca3b556..358548abe1 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -185,6 +185,9 @@
> # available during the preconfig state (i.e. when the --preconfig command
> # line option was in use).
> #
> +# Features:
> +# @unstable: This command is experimental.
> +#
> # Since 3.0
> #
> # Returns: nothing
> @@ -195,7 +198,8 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'x-exit-preconfig', 'allow-preconfig': true }
> +{ 'command': 'x-exit-preconfig', 'allow-preconfig': true,
> + 'features': [ 'unstable' ] }
>
> ##
> # @human-monitor-command:
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 7231ac3f34..ccd1167808 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -559,10 +559,8 @@
> #for ramblock-id. Disable this
> for 4.0
> #machine types or older to allow
> # migration with newer QEMU
> versions.
> -#This option is considered stable
> -#despite the x- prefix. (default:
> -#false generally, but true for
> machine
> -#types <= 4.0)
> +#(default: false generally,
> +#but true for machine types <=
> 4.0)
> #
> # Note: prealloc=true and reserve=false cannot be set at the same time.
> With
> # reserve=true, the behavior depends on the operating system: for
> example,
> @@ -785,6 +783,9 @@
> ##
> # @ObjectType:
> #
> +# Features:
> +# @unstable: Member @x-remote-object is experimental.
> +#
> # Since: 6.0
> ##
> { 'enum': 'ObjectType',
> @@ -836,7 +837,7 @@
> 'tls-creds-psk',
> 'tls-creds-x509',
> 'tls-cipher-suites',
> -'x-remote-object'
> +{ 'name': 'x-remote-object', 'features': [ 'unstable' ] }
>] }
>
> ##
> --
> 2.31.1
>
>
Seems OK, but I didn't audit for false positives/negatives. Trusting your
judgment here. (It looks like Phil started to audit this in his reply to
your previous commit, so I'll trust that.)
Acked-by: John Snow
Re: [PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/qmp/dispatch.h | 1 -
> monitor/misc.c | 3 +--
> scripts/qapi/commands.py| 5 +
> 3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 075203dc67..0ce88200b9 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -21,7 +21,6 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error
> **);
>
> typedef enum QmpCommandOptions
> {
> -QCO_NO_OPTIONS= 0x0,
> QCO_NO_SUCCESS_RESP = (1U << 0),
> QCO_ALLOW_OOB = (1U << 1),
> QCO_ALLOW_PRECONFIG = (1U << 2),
> diff --git a/monitor/misc.c b/monitor/misc.c
> index ffe7966870..3556b177f6 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -230,8 +230,7 @@ static void monitor_init_qmp_commands(void)
>
> qmp_init_marshal(_commands);
>
> -qmp_register_command(_commands, "device_add", qmp_device_add,
> - QCO_NO_OPTIONS);
> +qmp_register_command(_commands, "device_add", qmp_device_add, 0);
>
> QTAILQ_INIT(_cap_negotiation_commands);
> qmp_register_command(_cap_negotiation_commands,
> "qmp_capabilities",
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 3654825968..c8a975528f 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -229,15 +229,12 @@ def gen_register_command(name: str,
> if coroutine:
> options += ['QCO_COROUTINE']
>
> -if not options:
> -options = ['QCO_NO_OPTIONS']
> -
> ret = mcgen('''
> qmp_register_command(cmds, "%(name)s",
> qmp_marshal_%(c_name)s, %(opts)s);
> ''',
> name=name, c_name=c_name(name),
> -opts=" | ".join(options))
> +opts=' | '.join(options) or 0)
> return ret
>
>
>
I'm not a big fan of naked constants on the C side, but the generator
simplification is nice. I suppose it's worth the trade-off if you like it
better this way.
"eh".
Reviewed-by: John Snow
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
73,6 +374,7 @@ command test-command-features1 None -> None > feature deprecated > command test-command-features3 None -> None > gen=True success_response=True boxed=False oob=False preconfig=False > +feature unstable > feature feature1 > feature feature2 > command test-command-cond-features1 None -> None > @@ -394,6 +396,9 @@ event TEST_EVENT_FEATURES0 FeatureStruct1 > event TEST_EVENT_FEATURES1 None > boxed=False > feature deprecated > +event TEST_EVENT_FEATURES2 None > +boxed=False > +feature unstable > module include/sub-module.json > include sub-sub-module.json > object SecondArrayRef > -- > 2.31.1 > > Feels odd to combine the doc update *and* test prep, but eh, whatever. Reviewed-by: John Snow
[PATCH v2 15/15] iotests: [RFC] drop iotest 297
(This is highlighting a what-if, which might make it clear why any
special infrastructure is still required at all. It's not intended to
actually be merged at this step -- running JUST the iotest linters from
e.g. 'make check' is not yet accommodated, so there's no suitable
replacement for 297 for block test authors.)
Drop 297. As a consequence, we no longer need to pass an environment
variable to the mypy/pylint invocations, so that can be dropped. We also
now no longer need to hide output-except-on-error, so that can be
dropped as well.
The only thing that necessitates any special running logic anymore is
the skip list and the python-test-detection code. Without those, we
could easily codify the tests as simply:
[pylint|mypy] *.py tests/*.py
... and drop this entire file. We're not quite there yet, though.
Signed-off-by: John Snow
---
tests/qemu-iotests/297| 82 ---
tests/qemu-iotests/297.out| 2 -
tests/qemu-iotests/linters.py | 14 +-
3 files changed, 2 insertions(+), 96 deletions(-)
delete mode 100755 tests/qemu-iotests/297
delete mode 100644 tests/qemu-iotests/297.out
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
deleted file mode 100755
index ee78a627359..000
--- a/tests/qemu-iotests/297
+++ /dev/null
@@ -1,82 +0,0 @@
-#!/usr/bin/env python3
-# group: meta
-#
-# Copyright (C) 2020 Red Hat, Inc.
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program. If not, see <http://www.gnu.org/licenses/>.
-
-import os
-import subprocess
-import sys
-from typing import List
-
-import iotests
-import linters
-
-
-# Looking for something?
-#
-# List of files to exclude from linting: linters.py
-# mypy configuration:mypy.ini
-# pylint configuration: pylintrc
-
-
-def check_linter(linter: str) -> bool:
-try:
-linters.run_linter(linter, ['--version'], suppress_output=True)
-except subprocess.CalledProcessError:
-iotests.case_notrun(f"'{linter}' not found")
-return False
-return True
-
-
-def test_pylint(files: List[str]) -> None:
-print('=== pylint ===')
-sys.stdout.flush()
-
-if not check_linter('pylint'):
-return
-
-linters.run_linter('pylint', files)
-
-
-def test_mypy(files: List[str]) -> None:
-print('=== mypy ===')
-sys.stdout.flush()
-
-if not check_linter('mypy'):
-return
-
-env = os.environ.copy()
-env['MYPYPATH'] = env['PYTHONPATH']
-
-linters.run_linter('mypy', files, env=env, suppress_output=True)
-
-
-def main() -> None:
-files = linters.get_test_files()
-
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-for test in (test_pylint, test_mypy):
-try:
-test(files)
-except subprocess.CalledProcessError as exc:
-# Linter failure will be caught by diffing the IO.
-if exc.output:
-print(exc.output)
-
-
-iotests.script_main(main)
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
deleted file mode 100644
index f2e1314d104..000
--- a/tests/qemu-iotests/297.out
+++ /dev/null
@@ -1,2 +0,0 @@
-=== pylint ===
-=== mypy ===
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 65c4c4e8272..486b7125c1d 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -17,7 +17,7 @@
import re
import subprocess
import sys
-from typing import List, Mapping, Optional
+from typing import List
# TODO: Empty this list!
@@ -55,25 +55,15 @@ def get_test_files() -> List[str]:
return list(filter(is_python_file, check_tests))
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> None:
+def run_linter(tool: str, args: List[str]) -> None:
"""
Run a python-based linting tool.
-:param suppress_output: If True, suppress all stdout/stderr output.
:raise CalledProcessError: If the linter process exits with failure.
"""
subprocess.run(
('python3', '-m', tool, *args),
-env=env,
check=True,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
)
--
2.31.1
[PATCH v2 13/15] iotests/linters: Add workaround for mypy bug #9852
This one is insidious: if you write an import as "from {namespace}
import {subpackage}" as mirror-top-perms (now) does, mypy will fail on
every-other invocation *if* the package being imported is a typed,
installed, namespace-scoped package.
Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in
the context of Python CI tests.
Now, I could just edit mirror-top-perms to avoid this invocation, but
since I tripped on a landmine, I might as well head it off at the pass
and make sure nobody else trips on that same landmine.
It seems to have something to do with the order in which files are
checked as well, meaning the random order in which set(os.listdir())
produces the list of files to test will cause problems intermittently
and not just strictly "every other run".
This will be fixed in mypy >= 0.920, which is not released yet. The
workaround for now is to disable incremental checking, which avoids the
issue.
Note: This workaround is not applied when running iotest 297 directly,
because the bug does not surface there! Given the nature of CI jobs not
starting with any stale cache to begin with, this really only has a
half-second impact on manual runs of the Python test suite when executed
directly by a developer on their local machine. The workaround may be
removed when the Python package requirements can stipulate mypy 0.920 or
higher, which can happen as soon as it is released. (Barring any
unforseen compatibility issues that 0.920 may bring with it.)
See also:
https://github.com/python/mypy/issues/11010
https://github.com/python/mypy/issues/9852
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/linters.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 46c28fdcda0..65c4c4e8272 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -93,7 +93,9 @@ def show_usage() -> None:
if sys.argv[1] == '--pylint':
run_linter('pylint', files)
elif sys.argv[1] == '--mypy':
-run_linter('mypy', files)
+# mypy bug #9852; disable incremental checking as a workaround.
+args = ['--no-incremental'] + files
+run_linter('mypy', args)
else:
print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
show_usage()
--
2.31.1
[PATCH v2 11/15] iotests: split linters.py out from 297
Now, 297 is just the iotests-specific incantations and linters.py is as
minimal as I can think to make it. The only remaining element in here
that ought to be configuration and not code is the list of skip files,
but they're still numerous enough that repeating them for mypy and
pylint configurations both would be ... a hassle.
Signed-off-by: John Snow
---
tests/qemu-iotests/297| 72 +
tests/qemu-iotests/linters.py | 76 +++
2 files changed, 87 insertions(+), 61 deletions(-)
create mode 100644 tests/qemu-iotests/linters.py
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b7d9d6077b3..ee78a627359 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,74 +17,24 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import os
-import re
import subprocess
import sys
-from typing import List, Mapping, Optional
+from typing import List
import iotests
+import linters
-# TODO: Empty this list!
-SKIP_FILES = (
-'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-'096', '118', '124', '132', '136', '139', '147', '148', '149',
-'151', '152', '155', '163', '165', '194', '196', '202',
-'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '224', '228', '234', '235', '236', '237', '238',
-'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '302', '303', '304', '307',
-'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename):
-if not os.path.isfile(filename):
-return False
-
-if filename.endswith('.py'):
-return True
-
-with open(filename, encoding='utf-8') as f:
-try:
-first_line = f.readline()
-return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError: # Ignore binary files
-return False
-
-
-def get_test_files() -> List[str]:
-named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-return list(filter(is_python_file, check_tests))
-
-
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> None:
-"""
-Run a python-based linting tool.
-
-:param suppress_output: If True, suppress all stdout/stderr output.
-:raise CalledProcessError: If the linter process exits with failure.
-"""
-subprocess.run(
-('python3', '-m', tool, *args),
-env=env,
-check=True,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
-)
+# Looking for something?
+#
+# List of files to exclude from linting: linters.py
+# mypy configuration:mypy.ini
+# pylint configuration: pylintrc
def check_linter(linter: str) -> bool:
try:
-run_linter(linter, ['--version'], suppress_output=True)
+linters.run_linter(linter, ['--version'], suppress_output=True)
except subprocess.CalledProcessError:
iotests.case_notrun(f"'{linter}' not found")
return False
@@ -98,7 +48,7 @@ def test_pylint(files: List[str]) -> None:
if not check_linter('pylint'):
return
-run_linter('pylint', files)
+linters.run_linter('pylint', files)
def test_mypy(files: List[str]) -> None:
@@ -111,11 +61,11 @@ def test_mypy(files: List[str]) -> None:
env = os.environ.copy()
env['MYPYPATH'] = env['PYTHONPATH']
-run_linter('mypy', files, env=env, suppress_output=True)
+linters.run_linter('mypy', files, env=env, suppress_output=True)
def main() -> None:
-files = get_test_files()
+files = linters.get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 000..c515c7afe36
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,76 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received
[PATCH v2 10/15] iotests/297: split test into sub-cases
Take iotest 297's main() test function and split it into two sub-cases
that can be skipped individually. We can also drop custom environment
setup from the pylint test as it isn't needed.
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 63 ++
1 file changed, 39 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b2ad8d1cbe0..b7d9d6077b3 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,36 +82,51 @@ def run_linter(
)
+def check_linter(linter: str) -> bool:
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.case_notrun(f"'{linter}' not found")
+return False
+return True
+
+
+def test_pylint(files: List[str]) -> None:
+print('=== pylint ===')
+sys.stdout.flush()
+
+if not check_linter('pylint'):
+return
+
+run_linter('pylint', files)
+
+
+def test_mypy(files: List[str]) -> None:
+print('=== mypy ===')
+sys.stdout.flush()
+
+if not check_linter('mypy'):
+return
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+run_linter('mypy', files, env=env, suppress_output=True)
+
+
def main() -> None:
-for linter in ('pylint', 'mypy'):
-try:
-run_linter(linter, ['--version'], suppress_output=True)
-except subprocess.CalledProcessError:
-iotests.notrun(f"'{linter}' not found")
-
files = get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
-env = os.environ.copy()
-env['MYPYPATH'] = env['PYTHONPATH']
-
-print('=== pylint ===')
-sys.stdout.flush()
-try:
-run_linter('pylint', files, env=env)
-except subprocess.CalledProcessError:
-# pylint failure will be caught by diffing the IO.
-pass
-
-print('=== mypy ===')
-sys.stdout.flush()
-try:
-run_linter('mypy', files, env=env, suppress_output=True)
-except subprocess.CalledProcessError as exc:
-if exc.output:
-print(exc.output)
+for test in (test_pylint, test_mypy):
+try:
+test(files)
+except subprocess.CalledProcessError as exc:
+# Linter failure will be caught by diffing the IO.
+if exc.output:
+print(exc.output)
iotests.script_main(main)
--
2.31.1
[PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
We need at least a tiny little shim here to join test file discovery
with test invocation. This logic could conceivably be hosted somewhere
in python/, but I felt it was strictly the least-rude thing to keep the
test logic here in iotests/, even if this small function isn't itself an
iotest.
Note that we don't actually even need the executable bit here, we'll be
relying on the ability to run this module as a script using Python CLI
arguments. No chance it gets misunderstood as an actual iotest that way.
(It's named, not in tests/, doesn't have the execute bit, and doesn't
have an execution shebang.)
Signed-off-by: John Snow
---
tests/qemu-iotests/linters.py | 27 +++
1 file changed, 27 insertions(+)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index c515c7afe36..46c28fdcda0 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -16,6 +16,7 @@
import os
import re
import subprocess
+import sys
from typing import List, Mapping, Optional
@@ -74,3 +75,29 @@ def run_linter(
stderr=subprocess.STDOUT if suppress_output else None,
universal_newlines=True,
)
+
+
+def main() -> None:
+"""
+Used by the Python CI system as an entry point to run these linters.
+"""
+def show_usage() -> None:
+print(f"Usage: {sys.argv[0]} < --mypy | --pylint >", file=sys.stderr)
+sys.exit(1)
+
+if len(sys.argv) != 2:
+show_usage()
+
+files = get_test_files()
+
+if sys.argv[1] == '--pylint':
+run_linter('pylint', files)
+elif sys.argv[1] == '--mypy':
+run_linter('mypy', files)
+else:
+print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
+show_usage()
+
+
+if __name__ == '__main__':
+main()
--
2.31.1
[PATCH v2 09/15] iotests/297: update tool availability checks
As mentioned in 'iotests/297: Don't rely on distro-specific linter
binaries', these checks are overly strict. Update them to be in-line
with how we actually invoke the linters themselves.
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 76d6a23f531..b2ad8d1cbe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -18,7 +18,6 @@
import os
import re
-import shutil
import subprocess
import sys
from typing import List, Mapping, Optional
@@ -84,9 +83,11 @@ def run_linter(
def main() -> None:
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+for linter in ('pylint', 'mypy'):
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.notrun(f"'{linter}' not found")
files = get_test_files()
--
2.31.1
[PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure
Instead of using a process return code as the python function return
value (or just not returning anything at all), allow run_linter() to
raise an exception instead.
The responsibility for printing output on error shifts from the function
itself to the caller, who will know best how to present/format that
information. (Also, "suppress_output" is now a lot more accurate of a
parameter name.)
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d21673a2929..76d6a23f531 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -70,22 +70,18 @@ def run_linter(
"""
Run a python-based linting tool.
-If suppress_output is True, capture stdout/stderr of the child
-process and only print that information back to stdout if the child
-process's return code was non-zero.
+:param suppress_output: If True, suppress all stdout/stderr output.
+:raise CalledProcessError: If the linter process exits with failure.
"""
-p = subprocess.run(
+subprocess.run(
('python3', '-m', tool, *args),
env=env,
-check=False,
+check=True,
stdout=subprocess.PIPE if suppress_output else None,
stderr=subprocess.STDOUT if suppress_output else None,
universal_newlines=True,
)
-if suppress_output and p.returncode != 0:
-print(p.stdout)
-
def main() -> None:
for linter in ('pylint-3', 'mypy'):
@@ -102,11 +98,19 @@ def main() -> None:
print('=== pylint ===')
sys.stdout.flush()
-run_linter('pylint', files, env=env)
+try:
+run_linter('pylint', files, env=env)
+except subprocess.CalledProcessError:
+# pylint failure will be caught by diffing the IO.
+pass
print('=== mypy ===')
sys.stdout.flush()
-run_linter('mypy', files, env=env, suppress_output=True)
+try:
+run_linter('mypy', files, env=env, suppress_output=True)
+except subprocess.CalledProcessError as exc:
+if exc.output:
+print(exc.output)
iotests.script_main(main)
--
2.31.1
[PATCH v2 04/15] iotests/297: Create main() function
Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.
(That environment setup will be moved over in forthcoming commits.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 15b54594c11..163ebc8ebfd 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -89,8 +89,12 @@ def run_linters():
print(p.stdout)
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+def main() -> None:
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
-iotests.script_main(run_linters)
+run_linters()
+
+
+iotests.script_main(main)
--
2.31.1
[PATCH v2 02/15] iotests/297: Split mypy configuration out into mypy.ini
More separation of code and configuration.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 14 +-
tests/qemu-iotests/mypy.ini | 12
2 files changed, 13 insertions(+), 13 deletions(-)
create mode 100644 tests/qemu-iotests/mypy.ini
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bc3a0ceb2aa..b8101e6024a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,7 @@ def run_linters():
sys.stdout.flush()
env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-'--scripts-are-modules',
-*files),
+p = subprocess.run(('mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
new file mode 100644
index 000..4c0339f5589
--- /dev/null
+++ b/tests/qemu-iotests/mypy.ini
@@ -0,0 +1,12 @@
+[mypy]
+disallow_any_generics = True
+disallow_incomplete_defs = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+implicit_reexport = False
+namespace_packages = True
+no_implicit_optional = True
+scripts_are_modules = True
+warn_redundant_casts = True
+warn_unused_configs = True
+warn_unused_ignores = True
--
2.31.1
[PATCH v2 01/15] iotests/297: Move pylint config into pylintrc
Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
configuration out of code, which I think is probably a good thing in
general.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 4 +---
tests/qemu-iotests/pylintrc | 16
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91ec34d9521..bc3a0ceb2aa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -65,10 +65,8 @@ def run_linters():
print('=== pylint ===')
sys.stdout.flush()
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
env = os.environ.copy()
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+subprocess.run(('pylint-3', *files),
env=env, check=False)
print('=== mypy ===')
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8cb4e1d6a6d..32ab77b8bb9 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -31,6 +31,22 @@ disable=invalid-name,
too-many-statements,
consider-using-f-string,
+
+[REPORTS]
+
+# Activate the evaluation score.
+score=no
+
+
+[MISCELLANEOUS]
+
+# List of note tags to take in consideration, separated by a comma.
+# TODO notes are fine, but FIXMEs or XXXs should probably just be
+# fixed (in tests, at least).
+notes=FIXME,
+ XXX,
+
+
[FORMAT]
# Maximum number of characters on a single line.
--
2.31.1
[PATCH v2 14/15] python: Add iotest linters to test suite
Run mypy and pylint on the iotests files directly from the Python CI test infrastructure. This ensures that any accidental breakages to the qemu.[qmp|aqmp|machine|utils] packages will be caught by that test suite. It also ensures that these linters are run with well-known versions and test against a wide variety of python versions, which helps to find accidental cross-version python compatibility issues. Signed-off-by: John Snow Reviewed-by: Hanna Reitz --- python/tests/iotests-mypy.sh | 4 python/tests/iotests-pylint.sh | 4 2 files changed, 8 insertions(+) create mode 100755 python/tests/iotests-mypy.sh create mode 100755 python/tests/iotests-pylint.sh diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh new file mode 100755 index 000..ee764708199 --- /dev/null +++ b/python/tests/iotests-mypy.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --mypy diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh new file mode 100755 index 000..4cae03424b4 --- /dev/null +++ b/python/tests/iotests-pylint.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --pylint -- 2.31.1
[PATCH v2 06/15] iotests/297: Split run_linters apart into run_pylint and run_mypy
Move environment setup into main(), and split the actual linter
execution into run_pylint and run_mypy, respectively.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c1bddb9ce0e..189bcaf5f94 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
import shutil
import subprocess
import sys
-from typing import List
+from typing import List, Mapping, Optional
import iotests
@@ -61,23 +61,19 @@ def get_test_files() -> List[str]:
return list(filter(is_python_file, check_tests))
-def run_linters():
-files = get_test_files()
+def run_pylint(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-print('=== pylint ===')
-sys.stdout.flush()
-
-env = os.environ.copy()
subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
-print('=== mypy ===')
-sys.stdout.flush()
-env['MYPYPATH'] = env['PYTHONPATH']
+def run_mypy(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
@@ -94,7 +90,21 @@ def main() -> None:
if shutil.which(linter) is None:
iotests.notrun(f'{linter} not found')
-run_linters()
+files = get_test_files()
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+print('=== pylint ===')
+sys.stdout.flush()
+run_pylint(files, env=env)
+
+print('=== mypy ===')
+sys.stdout.flush()
+run_mypy(files, env=env)
iotests.script_main(main)
--
2.31.1
[PATCH v2 07/15] iotests/297: refactor run_[mypy|pylint] as generic execution shim
There's virtually nothing special here anymore; we can combine these
into a single, rather generic function.
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 42 ++
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 189bcaf5f94..d21673a2929 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,27 +61,29 @@ def get_test_files() -> List[str]:
return list(filter(is_python_file, check_tests))
-def run_pylint(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
+def run_linter(
+tool: str,
+args: List[str],
+env: Optional[Mapping[str, str]] = None,
+suppress_output: bool = False,
) -> None:
+"""
+Run a python-based linting tool.
-subprocess.run(('python3', '-m', 'pylint', *files),
- env=env, check=False)
+If suppress_output is True, capture stdout/stderr of the child
+process and only print that information back to stdout if the child
+process's return code was non-zero.
+"""
+p = subprocess.run(
+('python3', '-m', tool, *args),
+env=env,
+check=False,
+stdout=subprocess.PIPE if suppress_output else None,
+stderr=subprocess.STDOUT if suppress_output else None,
+universal_newlines=True,
+)
-
-def run_mypy(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
-) -> None:
-p = subprocess.run(('python3', '-m', 'mypy', *files),
- env=env,
- check=False,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True)
-
-if p.returncode != 0:
+if suppress_output and p.returncode != 0:
print(p.stdout)
@@ -100,11 +102,11 @@ def main() -> None:
print('=== pylint ===')
sys.stdout.flush()
-run_pylint(files, env=env)
+run_linter('pylint', files, env=env)
print('=== mypy ===')
sys.stdout.flush()
-run_mypy(files, env=env)
+run_linter('mypy', files, env=env, suppress_output=True)
iotests.script_main(main)
--
2.31.1
[PATCH v2 05/15] iotests/297: Don't rely on distro-specific linter binaries
'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.
Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing. This is addressed by a
commit later in this series;
'iotests/297: update tool availability checks'.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 163ebc8ebfd..c1bddb9ce0e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,14 +71,14 @@ def run_linters():
sys.stdout.flush()
env = os.environ.copy()
-subprocess.run(('pylint-3', *files),
+subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
print('=== mypy ===')
sys.stdout.flush()
env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy', *files),
+p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
--
2.31.1
[PATCH v2 03/15] iotests/297: Add get_files() function
Split out file discovery into its own method to begin separating out
configuration/setup and test execution.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/297 | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b8101e6024a..15b54594c11 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
import shutil
import subprocess
import sys
+from typing import List
import iotests
@@ -54,10 +55,14 @@ def is_python_file(filename):
return False
-def run_linters():
+def get_test_files() -> List[str]:
named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-files = [filename for filename in check_tests if is_python_file(filename)]
+return list(filter(is_python_file, check_tests))
+
+
+def run_linters():
+files = get_test_files()
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
--
2.31.1
[PATCH v2 00/15] python/iotests: Run iotest linters during Python CI
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt2 CI: https://gitlab.com/jsnow/qemu/-/pipelines/388626603 (There's no real rush on my part for this particular series, so review at-leisure, I'm just getting my edits back out onto the list. The AQMP series is more important to me in the short-term. I suppose this would be good to have prior to the RC testing phase, though.) Factor out pylint and mypy configuration from iotest 297 so that the Python test infrastructure in python/ can also run these linters. This will enable what is essentially iotest #297 to run via GitLab CI, via the 'check-python-pipenv' and 'check-python-tox' tests with new 'iotests-pylint' and 'iotests-mypy' cases. This series generally aims to split "linter configuration" out of code so that both iotest #297 and the Python test suite can both invoke the same linters (nearly) identically. iotest #297 is left as a valid way to run these tests as a convenience for block layer developers until I can integrate environment setup and test execution as a part of 'make check' or similar to serve as a replacement. This invocation (at present) just trusts you've installed the right packages into the right places with the right versions, as it always has. Future patches may begin to build the Python testing environment as part of 'make check', but there are some details to hammer out there, first. v5: Broadly: - Remove int return from linting helpers - Tighten tool availability checks - Explicitly ensured no regressions on 297 mid-series In detail: 001/15:[] [--] 'iotests/297: Move pylint config into pylintrc' 002/15:[] [--] 'iotests/297: Split mypy configuration out into mypy.ini' 003/15:[] [--] 'iotests/297: Add get_files() function' 004/15:[] [--] 'iotests/297: Create main() function' 005/15:[0002] [FC] 'iotests/297: Don't rely on distro-specific linter binaries' 006/15:[0023] [FC] 'iotests/297: Split run_linters apart into run_pylint and run_mypy' 007/15:[0006] [FC] 'iotests/297: refactor run_[mypy|pylint] as generic execution shim' 008/15:[down] 'iotests/297: Change run_linter() to raise an exception on failure' 009/15:[down] 'iotests/297: update tool availability checks' 010/15:[down] 'iotests/297: split test into sub-cases' 011/15:[0051] [FC] 'iotests: split linters.py out from 297' 012/15:[0021] [FC] 'iotests/linters: Add entry point for linting via Python CI' 013/15:[0004] [FC] 'iotests/linters: Add workaround for mypy bug #9852' 014/15:[] [--] 'python: Add iotest linters to test suite' 015/15:[0072] [FC] 'iotests: [RFC] drop iotest 297' - 005: Fixed extra parenthesis. (Oops.) - 006: Squashed with intermediate patch from v1. - 007: Removed 'int' return from run_linter() - 008-010: New. - 011: Modified comment, otherwise just rebased. - 012: Rewrote CLI handling to be more thorough. - 013: Rebase changes only. - 015: Rebase changes, not that it matters! v4: - Some optimizations and touchups were included in 'PATCH v2 0/6] iotests: update environment and linting configuration' instead, upon which this series is now based. - Rewrote most patches, being more aggressive about the factoring between "iotest" and "linter invocation". The patches are smaller now. - Almost all configuration is split out into mypy.ini and pylintrc. - Remove the PWD/CWD juggling that the previous versions added; it's not strictly needed for this integration. We can re-add it later if we wind up needing it for something. - mypy and pylintrc tests are split into separate tests. The GitLab CI now lists them as two separate test cases, so it's easier to see what is failing and why. (And how long those tests take.) It is also now therefore possible to ask avocado to run just one or the other. - mypy bug workaround is only applied strictly in cases where it is needed, optimizing speed of iotest 297. v3: - Added patch 1 which has been submitted separately upstream, but was necessary for testing. - Rebased on top of hreitz/block, which fixed some linting issues. - Added a workaround for a rather nasty mypy bug ... >:( v2: - Added patches 1-5 which do some more delinting. - Added patch 8, which scans subdirs for tests to lint. - Added patch 17, which improves the speed of mypy analysis. - Patch 14 is different because of the new patch 8. John Snow (15): iotests/297: Move pylint config into pylintrc iotests/297: Split mypy configuration out into mypy.ini iotests/297: Add get_files() function iotests/297: Create main() function iotests/297: Don't rely on distro-specific linter binaries iotests/297: Split run_linters apart into run_pylint and run_mypy iotests/297: refactor run_[mypy|pylint] as generic execution shim iotests/297: Change run_linter() to raise an exception on failure iotests/297: update tool availability checks iotests/297: split test into sub-cases iotests: split linters.py out from 297 iotests/linters: Add entr
Re: iotest 030 SIGSEGV
On Thu, Oct 14, 2021 at 9:20 AM Hanna Reitz wrote: > On 13.10.21 23:50, John Snow wrote: > > In trying to replace the QMP library backend, I have now twice > > stumbled upon a SIGSEGV in iotest 030 in the last three weeks or so. > > > > I didn't have debug symbols on at the time, so I've got only this > > stack trace: > > > > (gdb) thread apply all bt > > > > Thread 8 (Thread 0x7f0a6b8c4640 (LWP 1873554)): > > #0 0x7f0a748a53ff in poll () at /lib64/libc.so.6 > > #1 0x7f0a759bfa36 in g_main_context_iterate.constprop () at > > /lib64/libglib-2.0.so.0 > > #2 0x7f0a7596d163 in g_main_loop_run () at /lib64/libglib-2.0.so.0 > > #3 0x557dac31d121 in iothread_run > > (opaque=opaque@entry=0x557dadd98800) at ../../iothread.c:73 > > #4 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6b8c3650) at > > ../../util/qemu-thread-posix.c:557 > > #5 0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0 > > #6 0x7f0a748b04c3 in clone () at /lib64/libc.so.6 > > > > Thread 7 (Thread 0x7f0a6b000640 (LWP 1873555)): > > #0 0x7f0a747ed7d2 in sigtimedwait () at /lib64/libc.so.6 > > #1 0x7f0a74b72cdc in sigwait () at /lib64/libpthread.so.0 > > #2 0x557dac2e403b in dummy_cpu_thread_fn > > (arg=arg@entry=0x557dae041c10) at ../../accel/dummy-cpus.c:46 > > #3 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6afff650) at > > ../../util/qemu-thread-posix.c:557 > > #4 0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0 > > #5 0x7f0a748b04c3 in clone () at /lib64/libc.so.6 > > > > Thread 6 (Thread 0x7f0a56afa640 (LWP 1873582)): > > #0 0x7f0a74b71308 in do_futex_wait.constprop () at > > /lib64/libpthread.so.0 > > #1 0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at > > /lib64/libpthread.so.0 > > #2 0x557dac4d8f1f in qemu_sem_timedwait > > (sem=sem@entry=0x557dadd62878, ms=ms@entry=1) at > > ../../util/qemu-thread-posix.c:327 > > #3 0x557dac4f5ac4 in worker_thread > > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:91 > > #4 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a56af9650) at > > ../../util/qemu-thread-posix.c:557 > > #5 0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0 > > #6 0x7f0a748b04c3 in clone () at /lib64/libc.so.6 > > > > Thread 5 (Thread 0x7f0a57dff640 (LWP 1873580)): > > #0 0x7f0a74b71308 in do_futex_wait.constprop () at > > /lib64/libpthread.so.0 > > #1 0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at > > /lib64/libpthread.so.0 > > #2 0x557dac4d8f1f in qemu_sem_timedwait > > (sem=sem@entry=0x557dadd62878, ms=ms@entry=1) at > > ../../util/qemu-thread-posix.c:327 > > #3 0x557dac4f5ac4 in worker_thread > > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:91 > > #4 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a57dfe650) at > > ../../util/qemu-thread-posix.c:557 > > #5 0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0 > > #6 0x7f0a748b04c3 in clone () at /lib64/libc.so.6 > > > > Thread 4 (Thread 0x7f0a572fb640 (LWP 1873581)): > > #0 0x7f0a74b7296f in pread64 () at /lib64/libpthread.so.0 > > #1 0x557dac39f18f in pread64 (__offset=, > > __nbytes=, __buf=, __fd=) > > at /usr/include/bits/unistd.h:105 > > #2 handle_aiocb_rw_linear (aiocb=aiocb@entry=0x7f0a573fc150, > > buf=0x7f0a6a47e000 '\377' ...) at > > ../../block/file-posix.c:1481 > > #3 0x557dac39f664 in handle_aiocb_rw (opaque=0x7f0a573fc150) at > > ../../block/file-posix.c:1521 > > #4 0x557dac4f5b54 in worker_thread > > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:104 > > #5 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a572fa650) at > > ../../util/qemu-thread-posix.c:557 > > #6 0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0 > > #7 0x7f0a748b04c3 in clone () at /lib64/libc.so.6 > > > > Thread 3 (Thread 0x7f0a714e8640 (LWP 1873552)): > > #0 0x7f0a748aaedd in syscall () at /lib64/libc.so.6 > > #1 0x557dac4d916a in qemu_futex_wait (val=, > > f=) at /home/jsnow/src/qemu/include/qemu/futex.h:29 > > #2 qemu_event_wait (ev=ev@entry=0x557dace1f1e8 > > ) at ../../util/qemu-thread-posix.c:480 > > #3 0x557dac4e189a in call_rcu_thread (opaque=opaque@entry=0x0) at > > ../../util/rcu.c:258 > > #4 0x557dac4d7f89 in qemu_thread_start (args=0x7f0a714e7650) at > > ../../util/qemu-thread-posix.c:557 > > #5 0x7f0a74b683f9 in start_thread () at /l
[PATCH v4 2/8] python/machine: Handle QMP errors on close more meticulously
To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.
(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)
Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
python/qemu/machine/machine.py | 48 +-
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
# Comprehensive reset for the failed launch case:
self._early_cleanup()
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err: # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
self._close_qemu_log_file()
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
self._post_launch()
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
def _early_cleanup(self) -> None:
"""
Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
self._early_cleanup()
if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
--
2.31.1
[PATCH v4 7/8] python/aqmp: Create sync QMP wrapper for iotests
This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.
It does not, however, attempt to mimic Exception compatibility.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/legacy.py | 138 +
1 file changed, 138 insertions(+)
create mode 100644 python/qemu/aqmp/legacy.py
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+ wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
--
2.31.1
[PATCH v4 8/8] python, iotests: replace qmp with aqmp
Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.
Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementation, proving that both implementations work
concurrently.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
)
from qemu.qmp import ( # pylint: disable=import-error
-QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
SocketAddrT,
@@ -50,6 +49,12 @@
from . import console_socket
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
LOG = logging.getLogger(__name__)
--
2.31.1
[PATCH v4 4/8] iotests: Accommodate async QMP Exception classes
(But continue to support the old ones for now, too.)
There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:
- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
scripts/simplebench/bench_block_job.py| 3 ++-
tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
import os
-from qemu import qmp
+from qemu.aqmp import ConnectError
from qemu.machine import machine
+from qemu.qmp import QMPConnectError
import iotests
from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PATCH v4 3/8] python/aqmp: Remove scary message
The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/__init__.py | 12
1 file changed, 12 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
# the COPYING file in the top-level directory.
import logging
-import warnings
from .error import AQMPError
from .events import EventListener
@@ -31,17 +30,6 @@
from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
# Suppress logging unless an application engages it.
logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
--
2.31.1
[PATCH v4 5/8] iotests: Conditionally silence certain AQMP errors
AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.
For example, during a failed QMP connection attempt, we might see:
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError
This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.
For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.
(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 20 +++-
tests/qemu-iotests/tests/mirror-top-perms | 12
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
import subprocess
import sys
import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
import unittest
@@ -114,6 +114,24 @@
sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
def unarchive_sample_image(sample, fname):
sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
from qemu.qmp import QMPConnectError
import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
- 'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+ 'this should not have happened')
except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
--
2.31.1
