[PATCH 0/4] iotests: finalize switch to async QMP

[PATCH 0/4] iotests: finalize switch to async QMP

[John Snow]

Based-on: <20220203015946.1330386-1-jsnow@redhat.com>
          [PULL 0/4] Python patches
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch-pt1b

This tiny series is a spiritual v4 to:
"[PATCH v3 00/31] Python: delete synchronous qemu.qmp package".

I've isolated just the bits that touch iotests, and that's these four
patches. If this series is approved, I'll send the series that renames
"qemu.aqmp" to "qemu.qmp" separately. That series has a lot of churn,
but it doesn't meaningfully alter anything -- so I'll avoid cluttering
up qemu-block list with those.

(Just be aware that I plan to finalize the switch soon!)

John Snow (4):
  python/machine: permanently switch to AQMP
  scripts/bench-block-job: switch to AQMP
  iotests/mirror-top-perms: switch to AQMP
  iotests: switch to AQMP

 python/qemu/machine/machine.py            | 18 +++++++-----------
 python/qemu/machine/qtest.py              |  2 +-
 scripts/simplebench/bench_block_job.py    |  3 +--
 tests/qemu-iotests/iotests.py             |  2 +-
 tests/qemu-iotests/tests/mirror-top-perms |  9 +++------
 5 files changed, 13 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/4] python/machine: permanently switch to AQMP

[John Snow]

Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch permanent. Update Exceptions and import paths as necessary.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Beraldo Leal <bleal@redhat.com>
---
 python/qemu/machine/machine.py | 18 +++++++-----------
 python/qemu/machine/qtest.py   |  2 +-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a5972fab4d..41be025ac7 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
     TypeVar,
 )
 
-from qemu.qmp import (  # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+    QEMUMonitorProtocol,
     QMPMessage,
     QMPReturnValue,
-    SocketAddrT,
 )
 
 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__)
 
 
@@ -743,8 +738,9 @@ def events_wait(self,
         :param timeout: Optional timeout, in seconds.
                         See QEMUMonitorProtocol.pull_event.
 
-        :raise QMPTimeoutError: If timeout was non-zero and no matching events
-                                were found.
+        :raise asyncio.TimeoutError:
+            If timeout was non-zero and no matching events were found.
+
         :return: A QMP event matching the filter criteria.
                  If timeout was 0 and no event matched, None.
         """
@@ -767,7 +763,7 @@ def _match(event: QMPMessage) -> bool:
             event = self._qmp.pull_event(wait=timeout)
             if event is None:
                 # NB: None is only returned when timeout is false-ish.
-                # Timeouts raise QMPTimeoutError instead!
+                # Timeouts raise asyncio.TimeoutError instead!
                 break
             if _match(event):
                 return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e5..13e0aaff84 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
     TextIO,
 )
 
-from qemu.qmp import SocketAddrT  # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
 
 from .machine import QEMUMachine
 
-- 
2.31.1

[PATCH 2/4] scripts/bench-block-job: switch to AQMP

[John Snow]

For this commit, we only need to remove accommodations for the
synchronous QMP library.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Beraldo Leal <bleal@redhat.com>
---
 scripts/simplebench/bench_block_job.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index a403c35b08..af9d1646a4 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
 
 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
 
 
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
         vm.launch()
     except OSError as e:
         return {'error': 'popen failed: ' + str(e)}
-    except (QMPConnectError, ConnectError, socket.timeout):
+    except (ConnectError, socket.timeout):
         return {'error': 'qemu failed: ' + str(vm.get_log())}
 
     try:
-- 
2.31.1

[PATCH 3/4] iotests/mirror-top-perms: switch to AQMP

[John Snow]

We don't have to maintain compatibility with both QMP libraries anymore,
so we can just remove the old exception. While we're here, take
advantage of the extra fields present in the VMLaunchFailure exception
that machine.py now raises.

(Note: I'm leaving the logging suppression here unchanged. I had
suggested previously we use filters to scrub the PID out of the logging
information so it could just be diffed as part of the iotest output, but
that meant *always* scrubbing PID from logger output, which defeated the
point of even offering that information in the output to begin with.

Ultimately, I decided it's fine to just suppress the logger temporarily.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/mirror-top-perms | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index b5849978c4..223f3c1620 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -22,7 +22,6 @@
 import os
 
 from qemu.machine import machine
-from qemu.qmp import QMPConnectError
 
 import iotests
 from iotests import change_log_level, qemu_img
@@ -99,15 +98,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
         self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
         self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
         try:
-            # Silence AQMP errors temporarily.
-            # TODO: Remove this and just allow the errors to be logged when
-            # AQMP fully replaces QMP.
+            # Silence AQMP logging errors temporarily.
             with change_log_level('qemu.aqmp'):
                 self.vm_b.launch()
                 print('ERROR: VM B launched successfully, '
                       'this should not have happened')
-        except (QMPConnectError, machine.VMLaunchFailure):
-            assert 'Is another process using the image' in self.vm_b.get_log()
+        except machine.VMLaunchFailure as exc:
+            assert 'Is another process using the image' in exc.output
 
         result = self.vm.qmp('block-job-cancel',
                              device='mirror')
-- 
2.31.1

Re: [PATCH 3/4] iotests/mirror-top-perms: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

03.02.2022 05:24, John Snow wrote:
> We don't have to maintain compatibility with both QMP libraries anymore,
> so we can just remove the old exception. While we're here, take
> advantage of the extra fields present in the VMLaunchFailure exception
> that machine.py now raises.
> 
> (Note: I'm leaving the logging suppression here unchanged. I had
> suggested previously we use filters to scrub the PID out of the logging
> information so it could just be diffed as part of the iotest output, but
> that meant*always*  scrubbing PID from logger output, which defeated the
> point of even offering that information in the output to begin with.
> 
> Ultimately, I decided it's fine to just suppress the logger temporarily.)
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 4/4] iotests: switch to AQMP

[John Snow]

All that's left is to import type definitions from the new library
instead.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Beraldo Leal <bleal@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8cdb381f2a..46e51ff07d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -38,7 +38,7 @@
 from contextlib import contextmanager
 
 from qemu.machine import qtest
-from qemu.qmp import QMPMessage
+from qemu.aqmp.legacy import QMPMessage
 
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
-- 
2.31.1

Re: [PATCH 0/4] iotests: finalize switch to async QMP

[John Snow]

Squeak Squeak...

...Any objections to me staging this?

(This patchset removes the accommodations in iotests for allowing
either library to run and always forces the new one. Point of no
return for iotests.)

--js

On Wed, Feb 2, 2022 at 9:24 PM John Snow <jsnow@redhat.com> wrote:
>
> Based-on: <20220203015946.1330386-1-jsnow@redhat.com>
>           [PULL 0/4] Python patches
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch-pt1b
>
> This tiny series is a spiritual v4 to:
> "[PATCH v3 00/31] Python: delete synchronous qemu.qmp package".
>
> I've isolated just the bits that touch iotests, and that's these four
> patches. If this series is approved, I'll send the series that renames
> "qemu.aqmp" to "qemu.qmp" separately. That series has a lot of churn,
> but it doesn't meaningfully alter anything -- so I'll avoid cluttering
> up qemu-block list with those.
>
> (Just be aware that I plan to finalize the switch soon!)
>
> John Snow (4):
>   python/machine: permanently switch to AQMP
>   scripts/bench-block-job: switch to AQMP
>   iotests/mirror-top-perms: switch to AQMP
>   iotests: switch to AQMP
>
>  python/qemu/machine/machine.py            | 18 +++++++-----------
>  python/qemu/machine/qtest.py              |  2 +-
>  scripts/simplebench/bench_block_job.py    |  3 +--
>  tests/qemu-iotests/iotests.py             |  2 +-
>  tests/qemu-iotests/tests/mirror-top-perms |  9 +++------
>  5 files changed, 13 insertions(+), 21 deletions(-)
>
> --
> 2.31.1

Re: [PATCH 0/4] iotests: finalize switch to async QMP

[Hanna Reitz]

On 08.02.22 20:52, John Snow wrote:
> Squeak Squeak...
>
> ...Any objections to me staging this?
>
> (This patchset removes the accommodations in iotests for allowing
> either library to run and always forces the new one. Point of no
> return for iotests.)

I took this as “if I don’t reply, that’ll be reply enough” :)

Looks to me like the rebase is minimal (just shuffling the imports in 
patch 4 a bit), so I guess this’ll help even before you resend:

Acked-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 0/4] iotests: finalize switch to async QMP

[John Snow]

On Fri, Mar 18, 2022 at 12:32 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 08.02.22 20:52, John Snow wrote:
> > Squeak Squeak...
> >
> > ...Any objections to me staging this?
> >
> > (This patchset removes the accommodations in iotests for allowing
> > either library to run and always forces the new one. Point of no
> > return for iotests.)
>
> I took this as “if I don’t reply, that’ll be reply enough” :)
>
> Looks to me like the rebase is minimal (just shuffling the imports in
> patch 4 a bit), so I guess this’ll help even before you resend:
>
> Acked-by: Hanna Reitz <hreitz@redhat.com>
>

Great, thanks! I just didn't want to pull the rug out from under
anyone on this and really wanted an explicit "yes, sure".

You're the best!

--js