| Message ID | 20210917054047.2042843-14-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Switch iotests to using Async QMP | expand |
On 17.09.21 07:40, 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 | 6 +++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py > index 4f03c12169..a403c35b08 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 451a0666f8..7d448f4d23 100755 > --- a/tests/qemu-iotests/tests/mirror-top-perms > +++ b/tests/qemu-iotests/tests/mirror-top-perms > @@ -103,7 +103,11 @@ class TestMirrorTopPerms(iotests.QMPTestCase): > print('ERROR: VM B launched successfully, this should not have ' > 'happened') > except qemu.qmp.QMPConnectError: > - assert 'Is another process using the image' in self.vm_b.get_log() > + pass > + except qemu.aqmp.ConnectError: > + pass > + > + assert 'Is another process using the image' in self.vm_b.get_log() But this assertion will fail if there was no exception, and so we won’t get to see the real problem, which is the original VM aborting (see the doc string). It doesn’t really matter that much that VM B can start (hence it being a logged error message, not a fatal error), and when it can start, of course it won’t print an error – but what’s important is that the original VM will then abort. I mean, not an absolute showstopper by any means, but still, the assertion was deliberately placed into the `except` block. Hanna
On Fri, Sep 17, 2021 at 10:35 AM Hanna Reitz <hreitz@redhat.com> wrote: > On 17.09.21 07:40, 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 | 6 +++++- > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/simplebench/bench_block_job.py > b/scripts/simplebench/bench_block_job.py > > index 4f03c12169..a403c35b08 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 451a0666f8..7d448f4d23 100755 > > --- a/tests/qemu-iotests/tests/mirror-top-perms > > +++ b/tests/qemu-iotests/tests/mirror-top-perms > > @@ -103,7 +103,11 @@ class TestMirrorTopPerms(iotests.QMPTestCase): > > print('ERROR: VM B launched successfully, this should not > have ' > > 'happened') > > except qemu.qmp.QMPConnectError: > > - assert 'Is another process using the image' in > self.vm_b.get_log() > > + pass > > + except qemu.aqmp.ConnectError: > > + pass > > + > > + assert 'Is another process using the image' in > self.vm_b.get_log() > > But this assertion will fail if there was no exception, and so we won’t > get to see the real problem, which is the original VM aborting (see the > doc string). > Uh, hm. OK, so the intent was that if vm_b somehow starts successfully that we will fail the test based on output in the diff, but we'll continue on to see other kinds of explosions so that the output is more useful for diagnosing the failure. Gotcha. It doesn’t really matter that much that VM B can start (hence it being a > logged error message, not a fatal error), and when it can start, of > course it won’t print an error – but what’s important is that the > original VM will then abort. > > I mean, not an absolute showstopper by any means, but still, the > assertion was deliberately placed into the `except` block. > > Hanna > I misunderstood the "test style" here. I'll fix it. (Uh, I also forgot to explicitly import qemu.aqmp. It happens to work anyway because of $reasons, but it's not very good style. I'll fix that, too.) --js
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py index 4f03c12169..a403c35b08 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 451a0666f8..7d448f4d23 100755 --- a/tests/qemu-iotests/tests/mirror-top-perms +++ b/tests/qemu-iotests/tests/mirror-top-perms @@ -103,7 +103,11 @@ class TestMirrorTopPerms(iotests.QMPTestCase): print('ERROR: VM B launched successfully, this should not have ' 'happened') except qemu.qmp.QMPConnectError: - assert 'Is another process using the image' in self.vm_b.get_log() + pass + except qemu.aqmp.ConnectError: + pass + + assert 'Is another process using the image' in self.vm_b.get_log() result = self.vm.qmp('block-job-cancel', device='mirror')
(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 | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-)