* [PATCH v2 01/17] python/aqmp: add greeting property to QMPClient
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 02/17] python/aqmp: add .empty() method to EventListener John Snow
` (16 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
Expose the greeting as a read-only property of QMPClient so it can be
retrieved at-will.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/aqmp/qmp_client.py | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 82e9dab124c..d2ad7459f9f 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -224,6 +224,11 @@ def __init__(self, name: Optional[str] = None) -> None:
'asyncio.Queue[QMPClient._PendingT]'
] = {}
+ @property
+ def greeting(self) -> Optional[Greeting]:
+ """The `Greeting` from the QMP server, if any."""
+ return self._greeting
+
@upper_half
async def _establish_session(self) -> None:
"""
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 02/17] python/aqmp: add .empty() method to EventListener
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
2021-09-23 0:49 ` [PATCH v2 01/17] python/aqmp: add greeting property to QMPClient John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 03/17] python/aqmp: Return cleared events from EventListener.clear() John Snow
` (15 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
Synchronous clients may want to know if they're about to block waiting
for an event or not. A method such as this is necessary to implement a
compatible interface for the old QEMUMonitorProtocol using the new async
internals.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/aqmp/events.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index fb81d216102..271899f6b82 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -556,6 +556,12 @@ async def get(self) -> Message:
"""
return await self._queue.get()
+ def empty(self) -> bool:
+ """
+ Return `True` if there are no pending events.
+ """
+ return self._queue.empty()
+
def clear(self) -> None:
"""
Clear this listener of all pending events.
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 03/17] python/aqmp: Return cleared events from EventListener.clear()
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
2021-09-23 0:49 ` [PATCH v2 01/17] python/aqmp: add greeting property to QMPClient John Snow
2021-09-23 0:49 ` [PATCH v2 02/17] python/aqmp: add .empty() method to EventListener John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 04/17] python/aqmp: add send_fd_scm John Snow
` (14 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
This serves two purposes:
(1) It is now possible to discern whether or not clear() removed any
event(s) from the queue with absolute certainty, and
(2) It is now very easy to get a List of all pending events in one
chunk, which is useful for the sync bridge.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/aqmp/events.py | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index 271899f6b82..5f7150c78d4 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -562,7 +562,7 @@ def empty(self) -> bool:
"""
return self._queue.empty()
- def clear(self) -> None:
+ def clear(self) -> List[Message]:
"""
Clear this listener of all pending events.
@@ -570,17 +570,22 @@ def clear(self) -> None:
pending FIFO queue synchronously. It can be also be used to
manually clear any pending events, if desired.
+ :return: The cleared events, if any.
+
.. warning::
Take care when discarding events. Cleared events will be
silently tossed on the floor. All events that were ever
accepted by this listener are visible in `history()`.
"""
+ events = []
while True:
try:
- self._queue.get_nowait()
+ events.append(self._queue.get_nowait())
except asyncio.QueueEmpty:
break
+ return events
+
def __aiter__(self) -> AsyncIterator[Message]:
return self
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 04/17] python/aqmp: add send_fd_scm
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (2 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 03/17] python/aqmp: Return cleared events from EventListener.clear() John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-07 14:52 ` Eric Blake
2021-09-23 0:49 ` [PATCH v2 05/17] python/aqmp: Add dict conversion method to Greeting object John Snow
` (13 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
The single space is indeed required to successfully transmit the file
descriptor to QEMU.
Python 3.11 removes support for calling sendmsg directly from a
transport's socket. There is no other interface for doing this, our use
case is, I suspect, "quite unique".
As far as I can tell, this is safe to do -- send_fd_scm is a synchronous
function and we can be guaranteed that the async coroutines will *not* be
running when it is invoked. In testing, it works correctly.
I investigated quite thoroughly the possibility of creating my own
asyncio Transport (The class that ultimately manages the raw socket
object) so that I could manage the socket myself, but this is so wildly
invasive and unportable I scrapped the idea. It would involve a lot of
copy-pasting of various python utilities and classes just to re-create
the same infrastructure, and for extremely little benefit. Nah.
Just boldly void the warranty instead, while I try to follow up on
https://bugs.python.org/issue43232
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/aqmp/qmp_client.py | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index d2ad7459f9f..f987da02eb0 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -9,6 +9,8 @@
import asyncio
import logging
+import socket
+import struct
from typing import (
Dict,
List,
@@ -624,3 +626,23 @@ async def execute(self, cmd: str,
"""
msg = self.make_execute_msg(cmd, arguments, oob=oob)
return await self.execute_msg(msg)
+
+ @upper_half
+ @require(Runstate.RUNNING)
+ def send_fd_scm(self, fd: int) -> None:
+ """
+ Send a file descriptor to the remote via SCM_RIGHTS.
+ """
+ assert self._writer is not None
+ sock = self._writer.transport.get_extra_info('socket')
+
+ 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
+ sock.sendmsg(
+ [b' '],
+ [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+ )
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 04/17] python/aqmp: add send_fd_scm
2021-09-23 0:49 ` [PATCH v2 04/17] python/aqmp: add send_fd_scm John Snow
@ 2021-10-07 14:52 ` Eric Blake
2021-10-07 16:27 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2021-10-07 14:52 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Wed, Sep 22, 2021 at 08:49:25PM -0400, John Snow wrote:
> The single space is indeed required to successfully transmit the file
> descriptor to QEMU.
Sending fds requires a payload of at least one byte, but I don't think
that qemu cares which byte. Thus, while your choice of space is fine,
the commit message may be a bit misleading at implying it must be
space.
>
> Python 3.11 removes support for calling sendmsg directly from a
> transport's socket. There is no other interface for doing this, our use
> case is, I suspect, "quite unique".
>
> As far as I can tell, this is safe to do -- send_fd_scm is a synchronous
> function and we can be guaranteed that the async coroutines will *not* be
> running when it is invoked. In testing, it works correctly.
>
> I investigated quite thoroughly the possibility of creating my own
> asyncio Transport (The class that ultimately manages the raw socket
> object) so that I could manage the socket myself, but this is so wildly
> invasive and unportable I scrapped the idea. It would involve a lot of
> copy-pasting of various python utilities and classes just to re-create
> the same infrastructure, and for extremely little benefit. Nah.
>
> Just boldly void the warranty instead, while I try to follow up on
> https://bugs.python.org/issue43232
Bummer that we have to do that, but at least you are documenting the
problems and pursuing a remedy upstream.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/aqmp/qmp_client.py | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> index d2ad7459f9f..f987da02eb0 100644
> --- a/python/qemu/aqmp/qmp_client.py
> +++ b/python/qemu/aqmp/qmp_client.py
> @@ -9,6 +9,8 @@
>
> import asyncio
> import logging
> +import socket
> +import struct
> from typing import (
> Dict,
> List,
> @@ -624,3 +626,23 @@ async def execute(self, cmd: str,
> """
> msg = self.make_execute_msg(cmd, arguments, oob=oob)
> return await self.execute_msg(msg)
> +
> + @upper_half
> + @require(Runstate.RUNNING)
> + def send_fd_scm(self, fd: int) -> None:
> + """
> + Send a file descriptor to the remote via SCM_RIGHTS.
> + """
> + assert self._writer is not None
> + sock = self._writer.transport.get_extra_info('socket')
> +
> + 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
> + sock.sendmsg(
> + [b' '],
> + [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
> + )
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 04/17] python/aqmp: add send_fd_scm
2021-10-07 14:52 ` Eric Blake
@ 2021-10-07 16:27 ` John Snow
2021-10-07 21:46 ` Eric Blake
0 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-10-07 16:27 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]
On Thu, Oct 7, 2021 at 10:52 AM Eric Blake <eblake@redhat.com> wrote:
> On Wed, Sep 22, 2021 at 08:49:25PM -0400, John Snow wrote:
> > The single space is indeed required to successfully transmit the file
> > descriptor to QEMU.
>
> Sending fds requires a payload of at least one byte, but I don't think
> that qemu cares which byte. Thus, while your choice of space is fine,
> the commit message may be a bit misleading at implying it must be
> space.
>
>
OK, I'll rephrase. (Space winds up being useful in particular because it
doesn't mess with the parsing for subsequent JSON objects sent over the
wire.)
(Idle curiosity: Is it possible to make QEMU accept an empty payload here?
I understand that for compatibility reasons it wouldn't change much for the
python lib even if we did, but I'm curious.)
> >
> > Python 3.11 removes support for calling sendmsg directly from a
> > transport's socket. There is no other interface for doing this, our use
> > case is, I suspect, "quite unique".
> >
> > As far as I can tell, this is safe to do -- send_fd_scm is a synchronous
> > function and we can be guaranteed that the async coroutines will *not* be
> > running when it is invoked. In testing, it works correctly.
> >
> > I investigated quite thoroughly the possibility of creating my own
> > asyncio Transport (The class that ultimately manages the raw socket
> > object) so that I could manage the socket myself, but this is so wildly
> > invasive and unportable I scrapped the idea. It would involve a lot of
> > copy-pasting of various python utilities and classes just to re-create
> > the same infrastructure, and for extremely little benefit. Nah.
> >
> > Just boldly void the warranty instead, while I try to follow up on
> > https://bugs.python.org/issue43232
>
> Bummer that we have to do that, but at least you are documenting the
> problems and pursuing a remedy upstream.
>
>
Yeah. I suspect our use case is so niche that it's not likely to get
traction, but I'll try again. This sort of thing might make it harder to
use projects like pypy, so it does feel like a defeat. Still, where there's
a will, there's a way, right? :)
--js
[-- Attachment #2: Type: text/html, Size: 3013 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 04/17] python/aqmp: add send_fd_scm
2021-10-07 16:27 ` John Snow
@ 2021-10-07 21:46 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2021-10-07 21:46 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Thu, Oct 07, 2021 at 12:27:24PM -0400, John Snow wrote:
> On Thu, Oct 7, 2021 at 10:52 AM Eric Blake <eblake@redhat.com> wrote:
>
> > On Wed, Sep 22, 2021 at 08:49:25PM -0400, John Snow wrote:
> > > The single space is indeed required to successfully transmit the file
> > > descriptor to QEMU.
> >
> > Sending fds requires a payload of at least one byte, but I don't think
> > that qemu cares which byte. Thus, while your choice of space is fine,
> > the commit message may be a bit misleading at implying it must be
> > space.
> >
> >
> OK, I'll rephrase. (Space winds up being useful in particular because it
> doesn't mess with the parsing for subsequent JSON objects sent over the
> wire.)
>
> (Idle curiosity: Is it possible to make QEMU accept an empty payload here?
> I understand that for compatibility reasons it wouldn't change much for the
> python lib even if we did, but I'm curious.)
No, my understanding is that for SCM_RIGHTS to work reliably, you HAVE
to have a payload to avoid a return value of 0 from recvmsg() which
would be ambiguous with the peer performing orderly shutdown. 'man 7
unix' confirms:
At least one byte of real data should be sent when sending ancillary
data. On Linux, this is required to successfully send ancillary data
over a UNIX domain stream socket. When sending ancillary data over a
UNIX domain datagram socket, it is not necessary on Linux to send any
accompanying real data. However, portable applications should also in‐
clude at least one byte of real data when sending ancillary data over a
datagram socket.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 05/17] python/aqmp: Add dict conversion method to Greeting object
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (3 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 04/17] python/aqmp: add send_fd_scm John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-07 14:53 ` Eric Blake
2021-09-23 0:49 ` [PATCH v2 06/17] python/aqmp: Reduce severity of EOFError-caused loop terminations John Snow
` (12 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
The iotests interface expects to return the greeting as a dict; AQMP
offers it as a rich object.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/aqmp/models.py | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/python/qemu/aqmp/models.py b/python/qemu/aqmp/models.py
index 24c94123ac0..de87f878047 100644
--- a/python/qemu/aqmp/models.py
+++ b/python/qemu/aqmp/models.py
@@ -8,8 +8,10 @@
# pylint: disable=too-few-public-methods
from collections import abc
+import copy
from typing import (
Any,
+ Dict,
Mapping,
Optional,
Sequence,
@@ -66,6 +68,17 @@ def __init__(self, raw: Mapping[str, Any]):
self._check_member('QMP', abc.Mapping, "JSON object")
self.QMP = QMPGreeting(self._raw['QMP'])
+ def _asdict(self) -> Dict[str, object]:
+ """
+ For compatibility with the iotests sync QMP wrapper.
+
+ The legacy QMP interface needs Greetings as a garden-variety Dict.
+
+ This interface is private in the hopes that it will be able to
+ be dropped again in the near-future. Caller beware!
+ """
+ return dict(copy.deepcopy(self._raw))
+
class QMPGreeting(Model):
"""
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 05/17] python/aqmp: Add dict conversion method to Greeting object
2021-09-23 0:49 ` [PATCH v2 05/17] python/aqmp: Add dict conversion method to Greeting object John Snow
@ 2021-10-07 14:53 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2021-10-07 14:53 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Wed, Sep 22, 2021 at 08:49:26PM -0400, John Snow wrote:
> The iotests interface expects to return the greeting as a dict; AQMP
> offers it as a rich object.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/aqmp/models.py | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 06/17] python/aqmp: Reduce severity of EOFError-caused loop terminations
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (4 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 05/17] python/aqmp: Add dict conversion method to Greeting object John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-07 14:55 ` Eric Blake
2021-09-23 0:49 ` [PATCH v2 07/17] python/aqmp: Disable logging messages by default John Snow
` (11 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
When we encounter an EOFError, we don't know if it's an "error" in the
perspective of the user of the library yet. Therefore, we should not log
it as an error. Reduce the severity of this logging message to "INFO" to
indicate that it's something that we expect to occur during the normal
operation of the library.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/aqmp/protocol.py | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 32e78749c11..ae1df240260 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -721,8 +721,11 @@ async def _bh_loop_forever(self, async_fn: _TaskFN, name: str) -> None:
self.logger.debug("Task.%s: cancelled.", name)
return
except BaseException as err:
- self.logger.error("Task.%s: %s",
- name, exception_summary(err))
+ self.logger.log(
+ logging.INFO if isinstance(err, EOFError) else logging.ERROR,
+ "Task.%s: %s",
+ name, exception_summary(err)
+ )
self.logger.debug("Task.%s: failure:\n%s\n",
name, pretty_traceback())
self._schedule_disconnect()
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 06/17] python/aqmp: Reduce severity of EOFError-caused loop terminations
2021-09-23 0:49 ` [PATCH v2 06/17] python/aqmp: Reduce severity of EOFError-caused loop terminations John Snow
@ 2021-10-07 14:55 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2021-10-07 14:55 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Wed, Sep 22, 2021 at 08:49:27PM -0400, John Snow wrote:
> When we encounter an EOFError, we don't know if it's an "error" in the
> perspective of the user of the library yet. Therefore, we should not log
> it as an error. Reduce the severity of this logging message to "INFO" to
> indicate that it's something that we expect to occur during the normal
> operation of the library.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/aqmp/protocol.py | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 07/17] python/aqmp: Disable logging messages by default
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (5 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 06/17] python/aqmp: Reduce severity of EOFError-caused loop terminations John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-07 15:02 ` Eric Blake
2021-09-23 0:49 ` [PATCH v2 08/17] python/qmp: clear events on get_events() call John Snow
` (10 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
AQMP is a library, and ideally it should not print error diagnostics
unless a user opts into seeing them. By default, Python will print all
WARNING, ERROR or CRITICAL messages to screen if no logging
configuration has been created by a client application.
In AQMP's case, ERROR logging statements are used to report additional
detail about runtime failures that will also eventually be reported to the
client library via an Exception, so these messages should not be
rendered by default.
(Why bother to have them at all, then? In async contexts, there may be
multiple Exceptions and we are only able to report one of them back to
the client application. It is not reasonably easy to predict ahead of
time if one or more of these Exceptions will be squelched. Therefore,
it's useful to log intermediate failures to help make sense of the
ultimate, resulting failure.)
Add a NullHandler that will suppress these messages until a client
application opts into logging via logging.basicConfig or similar. Note
that upon calling basicConfig(), this handler will *not* suppress these
messages from being displayed by the client's configuration.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/aqmp/__init__.py | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index ab1782999cf..d1b0e4dc3d3 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -21,6 +21,7 @@
# This work is licensed under the terms of the GNU GPL, version 2. See
# the COPYING file in the top-level directory.
+import logging
import warnings
from .error import AQMPError
@@ -41,6 +42,9 @@
warnings.warn(_WMSG, FutureWarning)
+# Suppress logging unless an application engages it.
+logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
+
# The order of these fields impact the Sphinx documentation order.
__all__ = (
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 07/17] python/aqmp: Disable logging messages by default
2021-09-23 0:49 ` [PATCH v2 07/17] python/aqmp: Disable logging messages by default John Snow
@ 2021-10-07 15:02 ` Eric Blake
0 siblings, 0 replies; 37+ messages in thread
From: Eric Blake @ 2021-10-07 15:02 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Wed, Sep 22, 2021 at 08:49:28PM -0400, John Snow wrote:
> AQMP is a library, and ideally it should not print error diagnostics
> unless a user opts into seeing them. By default, Python will print all
> WARNING, ERROR or CRITICAL messages to screen if no logging
> configuration has been created by a client application.
>
> In AQMP's case, ERROR logging statements are used to report additional
> detail about runtime failures that will also eventually be reported to the
> client library via an Exception, so these messages should not be
> rendered by default.
>
> (Why bother to have them at all, then? In async contexts, there may be
> multiple Exceptions and we are only able to report one of them back to
> the client application. It is not reasonably easy to predict ahead of
> time if one or more of these Exceptions will be squelched. Therefore,
> it's useful to log intermediate failures to help make sense of the
> ultimate, resulting failure.)
>
> Add a NullHandler that will suppress these messages until a client
> application opts into logging via logging.basicConfig or similar. Note
> that upon calling basicConfig(), this handler will *not* suppress these
> messages from being displayed by the client's configuration.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/aqmp/__init__.py | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 08/17] python/qmp: clear events on get_events() call
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (6 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 07/17] python/aqmp: Disable logging messages by default John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 09/17] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol John Snow
` (9 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
All callers in the tree *already* clear the events after a call to
get_events(). Do it automatically instead and update callsites to remove
the manual clear call.
These semantics are quite a bit easier to emulate with async QMP, and
nobody appears to be abusing some emergent properties of what happens if
you decide not to clear them, so let's dial down to the dumber, simpler
thing.
Specifically: callers of clear() right after a call to get_events() are
more likely expressing their desire to not see any events they just
retrieved, whereas callers of clear_events() not in relation to a recent
call to pull_event/get_events are likely expressing their desire to
simply drop *all* pending events straight onto the floor. In the sync
world, this is safe enough; in the async world it's nearly impossible to
promise that nothing happens between getting and clearing the
events.
Making the retrieval also clear the queue is vastly simpler.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/machine/machine.py | 1 -
python/qemu/qmp/__init__.py | 6 ++++--
python/qemu/qmp/qmp_shell.py | 1 -
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 34131884a57..ae945ca3c94 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -631,7 +631,6 @@ def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
events = self._qmp.get_events(wait=wait)
events.extend(self._events)
del self._events[:]
- self._qmp.clear_events()
return events
@staticmethod
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 269516a79b9..c27594b66a2 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -361,7 +361,7 @@ def pull_event(self,
def get_events(self, wait: bool = False) -> List[QMPMessage]:
"""
- Get a list of available QMP events.
+ Get a list of available QMP events and clear all pending events.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@@ -374,7 +374,9 @@ def get_events(self, wait: bool = False) -> List[QMPMessage]:
@return The list of available QMP events.
"""
self.__get_events(wait)
- return self.__events
+ events = self.__events
+ self.__events = []
+ return events
def clear_events(self) -> None:
"""
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index 337acfce2d2..e7d7eb18f19 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -381,7 +381,6 @@ def read_exec_command(self) -> bool:
if cmdline == '':
for event in self.get_events():
print(event)
- self.clear_events()
return True
return self._execute_cmd(cmdline)
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 09/17] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (7 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 08/17] python/qmp: clear events on get_events() call John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 10/17] python, iotests: remove socket_scm_helper John Snow
` (8 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
It turns out you can do this directly from Python ... and because of
this, you don't need to worry about setting the inheritability of the
fds or spawning another process.
Doing this is helpful because it allows QEMUMonitorProtocol to keep its
file descriptor and socket object as private implementation
details. /that/ is helpful in turn because it allows me to write a
compatible, alternative implementation.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/machine/machine.py | 44 +++++++---------------------------
python/qemu/qmp/__init__.py | 21 +++++++---------
2 files changed, 18 insertions(+), 47 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ae945ca3c94..1c6532a3d68 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -213,48 +213,22 @@ def add_fd(self: _T, fd: int, fdset: int,
def send_fd_scm(self, fd: Optional[int] = None,
file_path: Optional[str] = None) -> int:
"""
- Send an fd or file_path to socket_scm_helper.
+ Send an fd or file_path to the remote via SCM_RIGHTS.
- Exactly one of fd and file_path must be given.
- If it is file_path, the helper will open that file and pass its own fd.
+ Exactly one of fd and file_path must be given. If it is
+ file_path, the file will be opened read-only and the new file
+ descriptor will be sent to the remote.
"""
- # In iotest.py, the qmp should always use unix socket.
- assert self._qmp.is_scm_available()
- if self._socket_scm_helper is None:
- raise QEMUMachineError("No path to socket_scm_helper set")
- if not os.path.exists(self._socket_scm_helper):
- raise QEMUMachineError("%s does not exist" %
- self._socket_scm_helper)
-
- # This did not exist before 3.4, but since then it is
- # mandatory for our purpose
- if hasattr(os, 'set_inheritable'):
- os.set_inheritable(self._qmp.get_sock_fd(), True)
- if fd is not None:
- os.set_inheritable(fd, True)
-
- fd_param = ["%s" % self._socket_scm_helper,
- "%d" % self._qmp.get_sock_fd()]
-
if file_path is not None:
assert fd is None
- fd_param.append(file_path)
+ with open(file_path, "rb") as passfile:
+ fd = passfile.fileno()
+ self._qmp.send_fd_scm(fd)
else:
assert fd is not None
- fd_param.append(str(fd))
+ self._qmp.send_fd_scm(fd)
- proc = subprocess.run(
- fd_param,
- stdin=subprocess.DEVNULL,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- check=False,
- close_fds=False,
- )
- if proc.stdout:
- LOG.debug(proc.stdout)
-
- return proc.returncode
+ return 0
@staticmethod
def _remove_if_exists(path: str) -> None:
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index c27594b66a2..358c0971d06 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -21,6 +21,7 @@
import json
import logging
import socket
+import struct
from types import TracebackType
from typing import (
Any,
@@ -408,18 +409,14 @@ def settimeout(self, timeout: Optional[float]) -> None:
raise ValueError(msg)
self.__sock.settimeout(timeout)
- def get_sock_fd(self) -> int:
+ def send_fd_scm(self, fd: int) -> None:
"""
- Get the socket file descriptor.
-
- @return The file descriptor number.
- """
- return self.__sock.fileno()
-
- def is_scm_available(self) -> bool:
+ Send a file descriptor to the remote via SCM_RIGHTS.
"""
- Check if the socket allows for SCM_RIGHTS.
+ if self.__sock.family != socket.AF_UNIX:
+ raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
- @return True if SCM_RIGHTS is available, otherwise False.
- """
- return self.__sock.family == socket.AF_UNIX
+ self.__sock.sendmsg(
+ [b' '],
+ [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+ )
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 10/17] python, iotests: remove socket_scm_helper
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (8 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 09/17] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 11/17] python/machine: remove has_quit argument John Snow
` (7 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
It's not used anymore, now.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/socket_scm_helper.c | 136 -------------------------
python/qemu/machine/machine.py | 3 -
python/qemu/machine/qtest.py | 2 -
tests/Makefile.include | 1 -
tests/meson.build | 4 -
tests/qemu-iotests/iotests.py | 3 -
tests/qemu-iotests/meson.build | 5 -
tests/qemu-iotests/testenv.py | 8 +-
8 files changed, 1 insertion(+), 161 deletions(-)
delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
delete mode 100644 tests/qemu-iotests/meson.build
diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
deleted file mode 100644
index eb76d31aa94..00000000000
--- a/tests/qemu-iotests/socket_scm_helper.c
+++ /dev/null
@@ -1,136 +0,0 @@
-/*
- * SCM_RIGHTS with unix socket help program for test
- *
- * Copyright IBM, Inc. 2013
- *
- * Authors:
- * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2 or later.
- * See the COPYING.LIB file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include <sys/socket.h>
-#include <sys/un.h>
-
-/* #define SOCKET_SCM_DEBUG */
-
-/*
- * @fd and @fd_to_send will not be checked for validation in this function,
- * a blank will be sent as iov data to notify qemu.
- */
-static int send_fd(int fd, int fd_to_send)
-{
- struct msghdr msg;
- struct iovec iov[1];
- int ret;
- char control[CMSG_SPACE(sizeof(int))];
- struct cmsghdr *cmsg;
-
- memset(&msg, 0, sizeof(msg));
- memset(control, 0, sizeof(control));
-
- /* Send a blank to notify qemu */
- iov[0].iov_base = (void *)" ";
- iov[0].iov_len = 1;
-
- msg.msg_iov = iov;
- msg.msg_iovlen = 1;
-
- msg.msg_control = control;
- msg.msg_controllen = sizeof(control);
-
- cmsg = CMSG_FIRSTHDR(&msg);
-
- cmsg->cmsg_len = CMSG_LEN(sizeof(int));
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- memcpy(CMSG_DATA(cmsg), &fd_to_send, sizeof(int));
-
- do {
- ret = sendmsg(fd, &msg, 0);
- } while (ret < 0 && errno == EINTR);
-
- if (ret < 0) {
- fprintf(stderr, "Failed to send msg, reason: %s\n", strerror(errno));
- }
-
- return ret;
-}
-
-/* Convert string to fd number. */
-static int get_fd_num(const char *fd_str, bool silent)
-{
- int sock;
- char *err;
-
- errno = 0;
- sock = strtol(fd_str, &err, 10);
- if (errno) {
- if (!silent) {
- fprintf(stderr, "Failed in strtol for socket fd, reason: %s\n",
- strerror(errno));
- }
- return -1;
- }
- if (!*fd_str || *err || sock < 0) {
- if (!silent) {
- fprintf(stderr, "bad numerical value for socket fd '%s'\n", fd_str);
- }
- return -1;
- }
-
- return sock;
-}
-
-/*
- * To make things simple, the caller needs to specify:
- * 1. socket fd.
- * 2. path of the file to be sent.
- */
-int main(int argc, char **argv, char **envp)
-{
- int sock, fd, ret;
-
-#ifdef SOCKET_SCM_DEBUG
- int i;
- for (i = 0; i < argc; i++) {
- fprintf(stderr, "Parameter %d: %s\n", i, argv[i]);
- }
-#endif
-
- if (argc != 3) {
- fprintf(stderr,
- "Usage: %s < socket-fd > < file-path >\n",
- argv[0]);
- return EXIT_FAILURE;
- }
-
-
- sock = get_fd_num(argv[1], false);
- if (sock < 0) {
- return EXIT_FAILURE;
- }
-
- fd = get_fd_num(argv[2], true);
- if (fd < 0) {
- /* Now only open a file in readonly mode for test purpose. If more
- precise control is needed, use python script in file operation, which
- is supposed to fork and exec this program. */
- fd = open(argv[2], O_RDONLY);
- if (fd < 0) {
- fprintf(stderr, "Failed to open file '%s'\n", argv[2]);
- return EXIT_FAILURE;
- }
- }
-
- ret = send_fd(sock, fd);
- if (ret < 0) {
- close(fd);
- return EXIT_FAILURE;
- }
-
- close(fd);
- return EXIT_SUCCESS;
-}
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 1c6532a3d68..056d340e355 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -98,7 +98,6 @@ def __init__(self,
name: Optional[str] = None,
base_temp_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper: Optional[str] = None,
sock_dir: Optional[str] = None,
drain_console: bool = False,
console_log: Optional[str] = None,
@@ -113,7 +112,6 @@ def __init__(self,
@param name: prefix for socket and log file names (default: qemu-PID)
@param base_temp_dir: default location where temp files are created
@param monitor_address: address for QMP monitor
- @param socket_scm_helper: helper program, required for send_fd_scm()
@param sock_dir: where to create socket (defaults to base_temp_dir)
@param drain_console: (optional) True to drain console socket to buffer
@param console_log: (optional) path to console log file
@@ -134,7 +132,6 @@ def __init__(self,
self._base_temp_dir = base_temp_dir
self._sock_dir = sock_dir or self._base_temp_dir
self._log_dir = log_dir
- self._socket_scm_helper = socket_scm_helper
if monitor_address is not None:
self._monitor_address = monitor_address
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 395cc8fbfe9..f2f9aaa5e50 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -115,7 +115,6 @@ def __init__(self,
wrapper: Sequence[str] = (),
name: Optional[str] = None,
base_temp_dir: str = "/var/tmp",
- socket_scm_helper: Optional[str] = None,
sock_dir: Optional[str] = None,
qmp_timer: Optional[float] = None):
# pylint: disable=too-many-arguments
@@ -126,7 +125,6 @@ def __init__(self,
sock_dir = base_temp_dir
super().__init__(binary, args, wrapper=wrapper, name=name,
base_temp_dir=base_temp_dir,
- socket_scm_helper=socket_scm_helper,
sock_dir=sock_dir, qmp_timer=qmp_timer)
self._qtest: Optional[QEMUQtestProtocol] = None
self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6e16c05f10b..5bd487a4030 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -139,7 +139,6 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) get-vm-images
check:
ifeq ($(CONFIG_TOOLS)$(CONFIG_POSIX),yy)
-QEMU_IOTESTS_HELPERS-$(CONFIG_LINUX) = tests/qemu-iotests/socket_scm_helper$(EXESUF)
check: check-block
export PYTHON
check-block: $(SRC_PATH)/tests/check-block.sh qemu-img$(EXESUF) \
diff --git a/tests/meson.build b/tests/meson.build
index 55a7b082751..3f3882748ae 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -67,10 +67,6 @@ if have_tools and 'CONFIG_VHOST_USER' in config_host and 'CONFIG_LINUX' in confi
dependencies: [qemuutil, vhost_user])
endif
-if have_system and 'CONFIG_POSIX' in config_host
- subdir('qemu-iotests')
-endif
-
test('decodetree', sh,
args: [ files('decode/check.sh'), config_host['PYTHON'], files('../scripts/decodetree.py') ],
workdir: meson.current_source_dir() / 'decode',
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf56304..9afa258a405 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -109,8 +109,6 @@
qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
-socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
-
luks_default_secret_object = 'secret,id=keysec0,data=' + \
os.environ.get('IMGKEYSECRET', '')
luks_default_key_secret_opt = 'key-secret=keysec0'
@@ -600,7 +598,6 @@ def __init__(self, path_suffix=''):
super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
name=name,
base_temp_dir=test_dir,
- socket_scm_helper=socket_scm_helper,
sock_dir=sock_dir, qmp_timer=timer)
self._num_drives = 0
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
deleted file mode 100644
index 67aed1e4927..00000000000
--- a/tests/qemu-iotests/meson.build
+++ /dev/null
@@ -1,5 +0,0 @@
-if 'CONFIG_LINUX' in config_host
- socket_scm_helper = executable('socket_scm_helper', 'socket_scm_helper.c')
-else
- socket_scm_helper = []
-endif
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 70da0d60c80..207bafb6493 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -68,7 +68,7 @@ class TestEnv(ContextManager['TestEnv']):
env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
- 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
+ 'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
@@ -137,7 +137,6 @@ def init_binaries(self) -> None:
"""Init binary path variables:
PYTHON (for bash tests)
QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
- SOCKET_SCM_HELPER
"""
self.python = sys.executable
@@ -171,10 +170,6 @@ def root(*names: str) -> str:
if not isxfile(b):
sys.exit('Not executable: ' + b)
- helper_path = os.path.join(self.build_iotests, 'socket_scm_helper')
- if isxfile(helper_path):
- self.socket_scm_helper = helper_path # SOCKET_SCM_HELPER
-
def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
cachemode: Optional[str] = None,
imgopts: Optional[str] = None,
@@ -300,7 +295,6 @@ def print_env(self) -> None:
PLATFORM -- {platform}
TEST_DIR -- {TEST_DIR}
SOCK_DIR -- {SOCK_DIR}
-SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
GDB_OPTIONS -- {GDB_OPTIONS}
VALGRIND_QEMU -- {VALGRIND_QEMU}
PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 11/17] python/machine: remove has_quit argument
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (9 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 10/17] python, iotests: remove socket_scm_helper John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-12 15:30 ` Hanna Reitz
2021-09-23 0:49 ` [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously John Snow
` (6 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 <jsnow@redhat.com>
---
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(self, timeout: Optional[int] = 30) -> None:
:param timeout: Optional timeout in seconds. Default 30 seconds.
A value of `None` is an infinite wait.
"""
- self.shutdown(has_quit=True, timeout=timeout)
+ self._quit_issued = True
+ self.shutdown(timeout=timeout)
def set_qmp_monitor(self, enabled: bool = True) -> None:
"""
@@ -574,7 +572,10 @@ def qmp(self, cmd: str,
conv_keys = True
qmp_args = self._qmp_args(conv_keys, args)
- return self._qmp.cmd(cmd, args=qmp_args)
+ ret = self._qmp.cmd(cmd, args=qmp_args)
+ if cmd == 'quit' and 'error' not in ret and 'return' in ret:
+ self._quit_issued = True
+ return ret
def command(self, cmd: str,
conv_keys: bool = True,
@@ -585,7 +586,10 @@ def command(self, cmd: str,
On failure raise an exception.
"""
qmp_args = self._qmp_args(conv_keys, args)
- return self._qmp.command(cmd, **qmp_args)
+ ret = self._qmp.command(cmd, **qmp_args)
+ if cmd == 'quit':
+ self._quit_issued = True
+ return ret
def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:
"""
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index f3677de9dfd..6af5ab9e764 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -92,10 +92,9 @@ class TestSingleDrive(ImageCommitTestCase):
self.vm.add_device('virtio-scsi')
self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
self.vm.launch()
- self.has_quit = False
def tearDown(self):
- self.vm.shutdown(has_quit=self.has_quit)
+ self.vm.shutdown()
os.remove(test_img)
os.remove(mid_img)
os.remove(backing_img)
@@ -127,8 +126,6 @@ class TestSingleDrive(ImageCommitTestCase):
result = self.vm.qmp('quit')
self.assert_qmp(result, 'return', {})
- self.has_quit = True
-
# Same as above, but this time we add the filter after starting the job
@iotests.skip_if_unsupported(['throttle'])
def test_commit_plus_filter_and_quit(self):
@@ -147,8 +144,6 @@ class TestSingleDrive(ImageCommitTestCase):
result = self.vm.qmp('quit')
self.assert_qmp(result, 'return', {})
- self.has_quit = True
-
def test_device_not_found(self):
result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img)
self.assert_qmp(result, 'error/class', 'DeviceNotFound')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 325d8244fb9..4922b4d3b6f 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -187,4 +187,4 @@ with iotests.VM() as vm, \
log(vm.qmp('quit'))
with iotests.Timeout(5, 'Timeout waiting for VM to quit'):
- vm.shutdown(has_quit=True)
+ vm.shutdown()
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index c43aa9c67ac..3d6d0e80cb5 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -123,4 +123,4 @@ with iotests.FilePath('src.qcow2') as src_path, \
vm.qmp_log('block-job-cancel', device='job0')
vm.qmp_log('quit')
- vm.shutdown(has_quit=True)
+ vm.shutdown()
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 11/17] python/machine: remove has_quit argument
2021-09-23 0:49 ` [PATCH v2 11/17] python/machine: remove has_quit argument John Snow
@ 2021-10-12 15:30 ` Hanna Reitz
0 siblings, 0 replies; 37+ messages in thread
From: Hanna Reitz @ 2021-10-12 15:30 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Cleber Rosa
On 23.09.21 02:49, John Snow wrote:
> 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 <jsnow@redhat.com>
> ---
> 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(-)
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (10 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 11/17] python/machine: remove has_quit argument John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-07 15:07 ` Eric Blake
2021-10-07 16:52 ` John Snow
2021-09-23 0:49 ` [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes John Snow
` (5 subsequent siblings)
17 siblings, 2 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 <jsnow@redhat.com>
---
Yes, I regret that this class has become quite a dumping ground for
complexity around the exit path. It's in need of a refactor to help
separate out the exception handling and cleanup mechanisms from the
VM-related stuff, but it's not a priority to do that just yet -- but
it's on the list.
Signed-off-by: John Snow <jsnow@redhat.com>
---
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..c33a78a2d9f 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._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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously
2021-09-23 0:49 ` [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously John Snow
@ 2021-10-07 15:07 ` Eric Blake
2021-10-07 16:34 ` John Snow
2021-10-07 16:52 ` John Snow
1 sibling, 1 reply; 37+ messages in thread
From: Eric Blake @ 2021-10-07 15:07 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On Wed, Sep 22, 2021 at 08:49:33PM -0400, John Snow wrote:
> 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 <jsnow@redhat.com>
>
> ---
>
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
This second S-o-b won't matter.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously
2021-10-07 15:07 ` Eric Blake
@ 2021-10-07 16:34 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-10-07 16:34 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]
On Thu, Oct 7, 2021 at 11:08 AM Eric Blake <eblake@redhat.com> wrote:
> On Wed, Sep 22, 2021 at 08:49:33PM -0400, John Snow wrote:
> > 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 <jsnow@redhat.com>
> >
> > ---
> >
> > Yes, I regret that this class has become quite a dumping ground for
> > complexity around the exit path. It's in need of a refactor to help
> > separate out the exception handling and cleanup mechanisms from the
> > VM-related stuff, but it's not a priority to do that just yet -- but
> > it's on the list.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> This second S-o-b won't matter.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Sorry, it's a bug with git-publish I've just not invested time into fixing.
It happens when I use a '---' to add an additional note for reviewers.
git-publish likes to add the second line because it doesn't "see" the first
one anymore. It's harmless, ultimately, but ... it sure does make me look
like I have no idea what I'm doing ;)
--js
[-- Attachment #2: Type: text/html, Size: 2593 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously
2021-09-23 0:49 ` [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously John Snow
2021-10-07 15:07 ` Eric Blake
@ 2021-10-07 16:52 ` John Snow
2021-10-12 15:56 ` Hanna Reitz
1 sibling, 1 reply; 37+ messages in thread
From: John Snow @ 2021-10-07 16:52 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 5691 bytes --]
On Wed, Sep 22, 2021 at 8:50 PM John Snow <jsnow@redhat.com> wrote:
> 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 <jsnow@redhat.com>
>
> ---
>
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 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..c33a78a2d9f 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._quit_issued:
>
I strongly suspect that this line needs to actually be "if not
(self._user_killed or self._quit_issued):" to also handle the force-kill
cases.
I wrote a different sync compatibility layer in a yet-to-be-published
branch and noticed this code still allows EOFError through. Why it did not
seem to come up in *this* series I still don't know -- I think the timing
of when exactly the coroutines get scheduled can be finnicky depending on
what else is running. So, sometimes, we manage to close the loop before we
get EOF and sometimes we don't.
I am mulling over adding a "tolerate_eof: bool = False" argument to
disconnect() to allow the caller to handle this stuff with a little less
boilerplate. Anyone have strong feelings on that?
(Ultimately, because there's no canonical "goodbye" in-band message, the
QMP layer itself can never really know if EOF was expected or not -- that's
really up to whomever is managing the connection, but it does add a layer
of complexity around the exit path that I am starting to find is a
nuisance. Not to mention that there are likely many possible cases in which
we might expect the remote to disappear on us that don't involve using QMP
at all -- kill is one, using the guest agent to coerce the guest into
shutdown is another. Networking and migration are other theoretical(?)
avenues for intended disconnects.)
> + 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
>
>
[-- Attachment #2: Type: text/html, Size: 7202 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously
2021-10-07 16:52 ` John Snow
@ 2021-10-12 15:56 ` Hanna Reitz
0 siblings, 0 replies; 37+ messages in thread
From: Hanna Reitz @ 2021-10-12 15:56 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Cleber Rosa, Eric Blake
On 07.10.21 18:52, John Snow wrote:
>
>
> On Wed, Sep 22, 2021 at 8:50 PM John Snow <jsnow@redhat.com> wrote:
>
> 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 <jsnow@redhat.com>
>
> ---
>
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 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..c33a78a2d9f 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._quit_issued:
>
>
> I strongly suspect that this line needs to actually be "if not
> (self._user_killed or self._quit_issued):" to also handle the
> force-kill cases.
Sounds right.
(Other than that, the patch looks good to me.)
> I wrote a different sync compatibility layer in a yet-to-be-published
> branch and noticed this code still allows EOFError through. Why it did
> not seem to come up in *this* series I still don't know -- I think the
> timing of when exactly the coroutines get scheduled can be finnicky
> depending on what else is running. So, sometimes, we manage to close
> the loop before we get EOF and sometimes we don't.
>
> I am mulling over adding a "tolerate_eof: bool = False" argument to
> disconnect() to allow the caller to handle this stuff with a little
> less boilerplate. Anyone have strong feelings on that?
FWIW I don’t.
> (Ultimately, because there's no canonical "goodbye" in-band message,
> the QMP layer itself can never really know if EOF was expected or not
> -- that's really up to whomever is managing the connection, but it
> does add a layer of complexity around the exit path that I am starting
> to find is a nuisance. Not to mention that there are likely many
> possible cases in which we might expect the remote to disappear on us
> that don't involve using QMP at all -- kill is one, using the guest
> agent to coerce the guest into shutdown is another. Networking and
> migration are other theoretical(?) avenues for intended disconnects.)
For the iotests we don’t have a guest, so I suppose this doesn’t concern
me, does it? :)
Hanna
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (11 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-12 16:06 ` Hanna Reitz
2021-09-23 0:49 ` [PATCH v2 14/17] iotests: Conditionally silence certain AQMP errors John Snow
` (4 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
(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 <jsnow@redhat.com>
---
scripts/simplebench/bench_block_job.py | 3 ++-
tests/qemu-iotests/tests/mirror-top-perms | 3 ++-
2 files changed, 4 insertions(+), 2 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 2fc8dd66e0a..9fe315e3b01 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,6 +26,7 @@ from iotests import qemu_img
# Import qemu after iotests.py has amended sys.path
# pylint: disable=wrong-import-order
import qemu
+from qemu.aqmp import ConnectError
image_size = 1 * 1024 * 1024
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
- except qemu.qmp.QMPConnectError:
+ except (qemu.qmp.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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes
2021-09-23 0:49 ` [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes John Snow
@ 2021-10-12 16:06 ` Hanna Reitz
2021-10-12 16:38 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Hanna Reitz @ 2021-10-12 16:06 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Cleber Rosa
On 23.09.21 02:49, John Snow wrote:
> (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 <jsnow@redhat.com>
> ---
> scripts/simplebench/bench_block_job.py | 3 ++-
> tests/qemu-iotests/tests/mirror-top-perms | 3 ++-
> 2 files changed, 4 insertions(+), 2 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 2fc8dd66e0a..9fe315e3b01 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -26,6 +26,7 @@ from iotests import qemu_img
> # Import qemu after iotests.py has amended sys.path
> # pylint: disable=wrong-import-order
> import qemu
> +from qemu.aqmp import ConnectError
With this change, the test emits the “AQMP is in development” warning,
breaking the test. Do we want to pull patch 16 before this patch?
(I also wonder whether we want to import QMPConnectError, too, because
the `except (qemu.qmp.*, *)` below looks so... heterogeneous.)
Hanna
> image_size = 1 * 1024 * 1024
> @@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
> self.vm_b.launch()
> print('ERROR: VM B launched successfully, this should not have '
> 'happened')
> - except qemu.qmp.QMPConnectError:
> + except (qemu.qmp.QMPConnectError, ConnectError):
> assert 'Is another process using the image' in self.vm_b.get_log()
>
> result = self.vm.qmp('block-job-cancel',
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes
2021-10-12 16:06 ` Hanna Reitz
@ 2021-10-12 16:38 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-10-12 16:38 UTC (permalink / raw)
To: Hanna Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]
On Tue, Oct 12, 2021 at 12:06 PM Hanna Reitz <hreitz@redhat.com> wrote:
> On 23.09.21 02:49, John Snow wrote:
> > (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 <jsnow@redhat.com>
> > ---
> > scripts/simplebench/bench_block_job.py | 3 ++-
> > tests/qemu-iotests/tests/mirror-top-perms | 3 ++-
> > 2 files changed, 4 insertions(+), 2 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 2fc8dd66e0a..9fe315e3b01 100755
> > --- a/tests/qemu-iotests/tests/mirror-top-perms
> > +++ b/tests/qemu-iotests/tests/mirror-top-perms
> > @@ -26,6 +26,7 @@ from iotests import qemu_img
> > # Import qemu after iotests.py has amended sys.path
> > # pylint: disable=wrong-import-order
> > import qemu
> > +from qemu.aqmp import ConnectError
>
> With this change, the test emits the “AQMP is in development” warning,
> breaking the test. Do we want to pull patch 16 before this patch?
>
>
Oh, good spot, I hadn't considered this.
Uh, yeah, I'll have to front-load the other patch.
> (I also wonder whether we want to import QMPConnectError, too, because
> the `except (qemu.qmp.*, *)` below looks so... heterogeneous.)
>
Will do.
(I don't think I've ever had code critiqued as "heterogeneous" before !)
>
> Hanna
>
> > image_size = 1 * 1024 * 1024
> > @@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
> > self.vm_b.launch()
> > print('ERROR: VM B launched successfully, this should not
> have '
> > 'happened')
> > - except qemu.qmp.QMPConnectError:
> > + except (qemu.qmp.QMPConnectError, ConnectError):
> > assert 'Is another process using the image' in
> self.vm_b.get_log()
> >
> > result = self.vm.qmp('block-job-cancel',
>
>
[-- Attachment #2: Type: text/html, Size: 4628 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 14/17] iotests: Conditionally silence certain AQMP errors
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (12 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests John Snow
` (3 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 20 +++++++++++++++++++-
tests/qemu-iotests/tests/mirror-top-perms | 9 +++++----
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9afa258a405..4d39b86ed85 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
@@ -116,6 +116,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 9fe315e3b01..5a34ec655e2 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,7 @@
import os
import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
# Import qemu after iotests.py has amended sys.path
# pylint: disable=wrong-import-order
@@ -100,9 +100,10 @@ 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')
+ with change_log_level('qemu.aqmp'):
+ self.vm_b.launch()
+ print('ERROR: VM B launched successfully, '
+ 'this should not have happened')
except (qemu.qmp.QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
--
2.31.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (13 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 14/17] iotests: Conditionally silence certain AQMP errors John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-06 10:13 ` Paolo Bonzini
2021-09-23 0:49 ` [PATCH v2 16/17] python/aqmp: Remove scary message John Snow
` (2 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 <jsnow@redhat.com>
Acked-by: Hanna Reitz <hreitz@redhat.com>
---
python/qemu/aqmp/legacy.py | 135 +++++++++++++++++++++++++++++++++++++
1 file changed, 135 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 00000000000..a75dbc599c0
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,135 @@
+"""
+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 wait is False:
+ # Return None if there's no event ready to go
+ if self._aqmp.events.empty():
+ return None
+
+ timeout = None
+ if 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests
2021-09-23 0:49 ` [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests John Snow
@ 2021-10-06 10:13 ` Paolo Bonzini
2021-10-06 14:24 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2021-10-06 10:13 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa
On 23/09/21 02:49, John Snow wrote:
> 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 <jsnow@redhat.com>
> Acked-by: Hanna Reitz <hreitz@redhat.com>
I don't like the name (of course). qemu-iotests shows that there is a
use for sync wrappers so, with similar reasoning to patch 16, there's no
need to scare people away. Why not just qemu.aqmp.sync?
Paolo
> ---
> python/qemu/aqmp/legacy.py | 135 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 135 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 00000000000..a75dbc599c0
> --- /dev/null
> +++ b/python/qemu/aqmp/legacy.py
> @@ -0,0 +1,135 @@
> +"""
> +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 wait is False:
> + # Return None if there's no event ready to go
> + if self._aqmp.events.empty():
> + return None
> +
> + timeout = None
> + if 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)
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests
2021-10-06 10:13 ` Paolo Bonzini
@ 2021-10-06 14:24 ` John Snow
2021-10-06 14:32 ` Paolo Bonzini
0 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-10-06 14:24 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]
On Wed, Oct 6, 2021 at 6:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/09/21 02:49, John Snow wrote:
> > 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 <jsnow@redhat.com>
> > Acked-by: Hanna Reitz <hreitz@redhat.com>
>
> I don't like the name (of course). qemu-iotests shows that there is a
> use for sync wrappers so, with similar reasoning to patch 16, there's no
> need to scare people away. Why not just qemu.aqmp.sync?
>
> Paolo
>
>
I had plans at one point to make a sync.py, but with an interface that
matched async QMP itself more closely. I spent some time trying to research
how to make a "magic" sync wrapper around async QMP, and hit a few trouble
spots. I've still got the patch, but I felt some pressure to try and switch
iotests over as fast as possible to get more trial-by-fire time this
release cycle. I named them "sync.py" and "legacy.py" in my branch
accordingly. Of course, I made a beeline straight for the iotests version,
so now it looks odd. I may yet try to clean up the other version, possibly
converting legacy.py to work in terms of sync.py, and then converting users
in iotests so that I can drop legacy.py.
(Mayyyybe. I am not heavily committed to any one particular approach here,
other than being very motivated to get AQMP tested widely a good bit before
rc0 to have a chance to smooth over any lingering problems that might
exist.)
Thanks for taking a look! I am more than happy to stage 1-9 myself, but I
will wait for Hanna's approval on 10-14 in particular.
--js
[-- Attachment #2: Type: text/html, Size: 2422 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests
2021-10-06 14:24 ` John Snow
@ 2021-10-06 14:32 ` Paolo Bonzini
2021-10-06 15:12 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2021-10-06 14:32 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
On 06/10/21 16:24, John Snow wrote:
>
> I had plans at one point to make a sync.py, but with an interface that
> matched async QMP itself more closely. I spent some time trying to
> research how to make a "magic" sync wrapper around async QMP, and hit a
> few trouble spots. I've still got the patch, but I felt some pressure to
> try and switch iotests over as fast as possible to get more
> trial-by-fire time this release cycle. I named them "sync.py" and
> "legacy.py" in my branch accordingly. Of course, I made a beeline
> straight for the iotests version, so now it looks odd. I may yet try to
> clean up the other version, possibly converting legacy.py to work in
> terms of sync.py, and then converting users in iotests so that I can
> drop legacy.py.
Got it. So maybe aqmp.qmp_next or aqmp.qmp_experimental? What I mean
is, it's all but legacy. :)
Paolo
> (Mayyyybe. I am not heavily committed to any one particular approach
> here, other than being very motivated to get AQMP tested widely a good
> bit before rc0 to have a chance to smooth over any lingering problems
> that might exist.)
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests
2021-10-06 14:32 ` Paolo Bonzini
@ 2021-10-06 15:12 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-10-06 15:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]
On Wed, Oct 6, 2021 at 10:32 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/10/21 16:24, John Snow wrote:
> >
> > I had plans at one point to make a sync.py, but with an interface that
> > matched async QMP itself more closely. I spent some time trying to
> > research how to make a "magic" sync wrapper around async QMP, and hit a
> > few trouble spots. I've still got the patch, but I felt some pressure to
> > try and switch iotests over as fast as possible to get more
> > trial-by-fire time this release cycle. I named them "sync.py" and
> > "legacy.py" in my branch accordingly. Of course, I made a beeline
> > straight for the iotests version, so now it looks odd. I may yet try to
> > clean up the other version, possibly converting legacy.py to work in
> > terms of sync.py, and then converting users in iotests so that I can
> > drop legacy.py.
>
> Got it. So maybe aqmp.qmp_next or aqmp.qmp_experimental? What I mean
> is, it's all but legacy. :)
>
>
Mmm, yeah I guess I meant "legacy interface" and not "legacy
implementation" ;)
I could do 'compat.py' for the iotests-compatible interface and 'sync.py'
for the "modern" one?
--js
[-- Attachment #2: Type: text/html, Size: 1721 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 16/17] python/aqmp: Remove scary message
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (14 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests John Snow
@ 2021-09-23 0:49 ` John Snow
2021-09-23 0:49 ` [PATCH v2 17/17] python, iotests: replace qmp with aqmp John Snow
2021-10-06 10:14 ` [PATCH v2 00/17] Switch iotests to using Async QMP Paolo Bonzini
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 <jsnow@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 17/17] python, iotests: replace qmp with aqmp
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (15 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 16/17] python/aqmp: Remove scary message John Snow
@ 2021-09-23 0:49 ` John Snow
2021-10-06 10:14 ` [PATCH v2 00/17] Switch iotests to using Async QMP Paolo Bonzini
17 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-09-23 0:49 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa, John Snow
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 implementatin, proving that both implementations work
concurrently.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Tested-by: Hanna Reitz <hreitz@redhat.com>
---
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 c33a78a2d9f..32879faeb40 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 00/17] Switch iotests to using Async QMP
2021-09-23 0:49 [PATCH v2 00/17] Switch iotests to using Async QMP John Snow
` (16 preceding siblings ...)
2021-09-23 0:49 ` [PATCH v2 17/17] python, iotests: replace qmp with aqmp John Snow
@ 2021-10-06 10:14 ` Paolo Bonzini
2021-10-06 15:01 ` John Snow
17 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2021-10-06 10:14 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, Hanna Reitz, Cleber Rosa
On 23/09/21 02:49, John Snow wrote:
> Based-on: <20210915162955.333025-1-jsnow@redhat.com>
> [PATCH v4 00/27] python: introduce Asynchronous QMP package
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/375637927
>
> Hiya,
>
> This series continues where the first 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 extremely minimal.
> (There's actually a net negative SLOC in tests/qemu-iotests.)
>
> 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'd like to try and get this committed early in the 6.2 development
> cycle to give ample time to smooth over any possible regressions. I've
> tested it locally and via gitlab CI, across Python versions 3.6 through
> 3.10, and "worksforme". If something bad happens, we can revert the
> actual switch-flip very trivially.
>
> Layout:
>
> Patches 1-7: ./python/qemu/aqmp changes that serve as pre-requisites.
> Patches 8-12: other ./python changes that ease the transition.
> Patches 13-14: iotest changes to support the new QMP backend.
> Patches 15-17: Make the switch.
>
> V2:
>
> 001/17:[----] [--] 'python/aqmp: add greeting property to QMPClient'
> 002/17:[----] [--] 'python/aqmp: add .empty() method to EventListener'
> 003/17:[----] [--] 'python/aqmp: Return cleared events from EventListener.clear()'
> 004/17:[0007] [FC] 'python/aqmp: add send_fd_scm'
> 005/17:[down] 'python/aqmp: Add dict conversion method to Greeting object'
> 006/17:[down] 'python/aqmp: Reduce severity of EOFError-caused loop terminations'
> 007/17:[down] 'python/aqmp: Disable logging messages by default'
>
> 008/17:[0002] [FC] 'python/qmp: clear events on get_events() call'
> 009/17:[----] [--] 'python/qmp: add send_fd_scm directly to QEMUMonitorProtocol'
> 010/17:[----] [--] 'python, iotests: remove socket_scm_helper'
> 011/17:[0013] [FC] 'python/machine: remove has_quit argument'
> 012/17:[down] 'python/machine: Handle QMP errors on close more meticulously'
>
> 013/17:[0009] [FC] 'iotests: Accommodate async QMP Exception classes'
> 014/17:[down] 'iotests: Conditionally silence certain AQMP errors'
>
> 015/17:[0016] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
> 016/17:[0002] [FC] 'python/aqmp: Remove scary message'
> 017/17:[----] [--] 'python, iotests: replace qmp with aqmp'
>
> - Rebased on jsnow/python, which was recently rebased on origin/master.
> - Make aqmp's send_fd_scm method bark if the socket isn't AF_UNIX (Hanna)
> - Uh... modify send_fd_scm so it doesn't break when Python 3.11 comes out ...
> See the commit message for more detail.
> - Drop the "python/aqmp: Create MessageModel and StandaloneModel class"
> patch and replace with a far simpler method that just adds an
> _asdict() method.
> - Add patches 06 and 07 to change how the AQMP library handles logging.
> - Adjust docstring in patch 08 (Hanna)
> - Rename "_has_quit" attribute to "_quid_issued" (Hanna)
> - Renamed patch 12, simplified the logic in _soft_shutdown a tiny bit.
> - Fixed bad exception handling logic in 13 (Hanna)
> - Introduce a helper in patch 14 to silence log output when it's unwanted.
> - Small addition of _get_greeting() helper in patch 15, coinciding with the
> new patch 05 here.
> - Contextual changes in 16.
>
> John Snow (17):
> python/aqmp: add greeting property to QMPClient
> python/aqmp: add .empty() method to EventListener
> python/aqmp: Return cleared events from EventListener.clear()
> python/aqmp: add send_fd_scm
> python/aqmp: Add dict conversion method to Greeting object
> python/aqmp: Reduce severity of EOFError-caused loop terminations
> python/aqmp: Disable logging messages by default
> python/qmp: clear events on get_events() call
> python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
> python, iotests: remove socket_scm_helper
> python/machine: remove has_quit argument
> python/machine: Handle QMP errors on close more meticulously
> iotests: Accommodate async QMP Exception classes
> iotests: Conditionally silence certain AQMP errors
> python/aqmp: Create sync QMP wrapper for iotests
> python/aqmp: Remove scary message
> python, iotests: replace qmp with aqmp
>
> tests/qemu-iotests/socket_scm_helper.c | 136 ----------------------
> python/qemu/aqmp/__init__.py | 14 +--
> python/qemu/aqmp/events.py | 15 ++-
> python/qemu/aqmp/legacy.py | 135 +++++++++++++++++++++
> python/qemu/aqmp/models.py | 13 +++
> python/qemu/aqmp/protocol.py | 7 +-
> python/qemu/aqmp/qmp_client.py | 27 +++++
> python/qemu/machine/machine.py | 133 +++++++++++----------
> python/qemu/machine/qtest.py | 2 -
> python/qemu/qmp/__init__.py | 27 +++--
> python/qemu/qmp/qmp_shell.py | 1 -
> scripts/simplebench/bench_block_job.py | 3 +-
> tests/Makefile.include | 1 -
> tests/meson.build | 4 -
> tests/qemu-iotests/040 | 7 +-
> tests/qemu-iotests/218 | 2 +-
> tests/qemu-iotests/255 | 2 +-
> tests/qemu-iotests/iotests.py | 23 +++-
> tests/qemu-iotests/meson.build | 5 -
> tests/qemu-iotests/testenv.py | 8 +-
> tests/qemu-iotests/tests/mirror-top-perms | 12 +-
> 21 files changed, 315 insertions(+), 262 deletions(-)
> delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
> create mode 100644 python/qemu/aqmp/legacy.py
> delete mode 100644 tests/qemu-iotests/meson.build
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
apart from the name nit in patch 15. I would really like this to go in
and get rid of socket_scm_helper.c!
Paolo
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 00/17] Switch iotests to using Async QMP
2021-10-06 10:14 ` [PATCH v2 00/17] Switch iotests to using Async QMP Paolo Bonzini
@ 2021-10-06 15:01 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-10-06 15:01 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Eduardo Habkost,
qemu-block, qemu-devel, Hanna Reitz, Cleber Rosa
[-- Attachment #1: Type: text/plain, Size: 6767 bytes --]
On Wed, Oct 6, 2021 at 6:14 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/09/21 02:49, John Snow wrote:
> > Based-on: <20210915162955.333025-1-jsnow@redhat.com>
> > [PATCH v4 00/27] python: introduce Asynchronous QMP package
> > GitLab:
> https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/375637927
> >
> > Hiya,
> >
> > This series continues where the first 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 extremely minimal.
> > (There's actually a net negative SLOC in tests/qemu-iotests.)
> >
> > 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'd like to try and get this committed early in the 6.2 development
> > cycle to give ample time to smooth over any possible regressions. I've
> > tested it locally and via gitlab CI, across Python versions 3.6 through
> > 3.10, and "worksforme". If something bad happens, we can revert the
> > actual switch-flip very trivially.
> >
> > Layout:
> >
> > Patches 1-7: ./python/qemu/aqmp changes that serve as pre-requisites.
> > Patches 8-12: other ./python changes that ease the transition.
> > Patches 13-14: iotest changes to support the new QMP backend.
> > Patches 15-17: Make the switch.
> >
> > V2:
> >
> > 001/17:[----] [--] 'python/aqmp: add greeting property to QMPClient'
> > 002/17:[----] [--] 'python/aqmp: add .empty() method to EventListener'
> > 003/17:[----] [--] 'python/aqmp: Return cleared events from
> EventListener.clear()'
> > 004/17:[0007] [FC] 'python/aqmp: add send_fd_scm'
> > 005/17:[down] 'python/aqmp: Add dict conversion method to Greeting
> object'
> > 006/17:[down] 'python/aqmp: Reduce severity of EOFError-caused loop
> terminations'
> > 007/17:[down] 'python/aqmp: Disable logging messages by default'
> >
> > 008/17:[0002] [FC] 'python/qmp: clear events on get_events() call'
> > 009/17:[----] [--] 'python/qmp: add send_fd_scm directly to
> QEMUMonitorProtocol'
> > 010/17:[----] [--] 'python, iotests: remove socket_scm_helper'
> > 011/17:[0013] [FC] 'python/machine: remove has_quit argument'
> > 012/17:[down] 'python/machine: Handle QMP errors on close more
> meticulously'
> >
> > 013/17:[0009] [FC] 'iotests: Accommodate async QMP Exception classes'
> > 014/17:[down] 'iotests: Conditionally silence certain AQMP errors'
> >
> > 015/17:[0016] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
> > 016/17:[0002] [FC] 'python/aqmp: Remove scary message'
> > 017/17:[----] [--] 'python, iotests: replace qmp with aqmp'
> >
> > - Rebased on jsnow/python, which was recently rebased on origin/master.
> > - Make aqmp's send_fd_scm method bark if the socket isn't AF_UNIX (Hanna)
> > - Uh... modify send_fd_scm so it doesn't break when Python 3.11 comes
> out ...
> > See the commit message for more detail.
> > - Drop the "python/aqmp: Create MessageModel and StandaloneModel class"
> > patch and replace with a far simpler method that just adds an
> > _asdict() method.
> > - Add patches 06 and 07 to change how the AQMP library handles logging.
> > - Adjust docstring in patch 08 (Hanna)
> > - Rename "_has_quit" attribute to "_quid_issued" (Hanna)
> > - Renamed patch 12, simplified the logic in _soft_shutdown a tiny bit.
> > - Fixed bad exception handling logic in 13 (Hanna)
> > - Introduce a helper in patch 14 to silence log output when it's
> unwanted.
> > - Small addition of _get_greeting() helper in patch 15, coinciding with
> the
> > new patch 05 here.
> > - Contextual changes in 16.
> >
> > John Snow (17):
> > python/aqmp: add greeting property to QMPClient
> > python/aqmp: add .empty() method to EventListener
> > python/aqmp: Return cleared events from EventListener.clear()
> > python/aqmp: add send_fd_scm
> > python/aqmp: Add dict conversion method to Greeting object
> > python/aqmp: Reduce severity of EOFError-caused loop terminations
> > python/aqmp: Disable logging messages by default
> > python/qmp: clear events on get_events() call
> > python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
> > python, iotests: remove socket_scm_helper
> > python/machine: remove has_quit argument
> > python/machine: Handle QMP errors on close more meticulously
> > iotests: Accommodate async QMP Exception classes
> > iotests: Conditionally silence certain AQMP errors
> > python/aqmp: Create sync QMP wrapper for iotests
> > python/aqmp: Remove scary message
> > python, iotests: replace qmp with aqmp
> >
> > tests/qemu-iotests/socket_scm_helper.c | 136 ----------------------
> > python/qemu/aqmp/__init__.py | 14 +--
> > python/qemu/aqmp/events.py | 15 ++-
> > python/qemu/aqmp/legacy.py | 135 +++++++++++++++++++++
> > python/qemu/aqmp/models.py | 13 +++
> > python/qemu/aqmp/protocol.py | 7 +-
> > python/qemu/aqmp/qmp_client.py | 27 +++++
> > python/qemu/machine/machine.py | 133 +++++++++++----------
> > python/qemu/machine/qtest.py | 2 -
> > python/qemu/qmp/__init__.py | 27 +++--
> > python/qemu/qmp/qmp_shell.py | 1 -
> > scripts/simplebench/bench_block_job.py | 3 +-
> > tests/Makefile.include | 1 -
> > tests/meson.build | 4 -
> > tests/qemu-iotests/040 | 7 +-
> > tests/qemu-iotests/218 | 2 +-
> > tests/qemu-iotests/255 | 2 +-
> > tests/qemu-iotests/iotests.py | 23 +++-
> > tests/qemu-iotests/meson.build | 5 -
> > tests/qemu-iotests/testenv.py | 8 +-
> > tests/qemu-iotests/tests/mirror-top-perms | 12 +-
> > 21 files changed, 315 insertions(+), 262 deletions(-)
> > delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
> > create mode 100644 python/qemu/aqmp/legacy.py
> > delete mode 100644 tests/qemu-iotests/meson.build
> >
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> apart from the name nit in patch 15. I would really like this to go in
> and get rid of socket_scm_helper.c!
>
> Paolo
>
>
Got a thumbs up from Hanna on IRC to stage patches 1-10 myself. I'll leave
patches 11-17 for further scrutiny.
so, patches 1-10: staged to my Python branch.
https://gitlab.com/jsnow/qemu/-/commits/python
thanks!
--js
[-- Attachment #2: Type: text/html, Size: 8849 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread