| Message ID | 20220215135727.28521-3-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Make bdrv_refresh_limits() non-recursive | expand |
On Tue, Feb 15, 2022 at 02:57:26PM +0100, Hanna Reitz wrote: > Add a parameter to optionally open a QMP connection when creating a > QemuStorageDaemon instance. > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > tests/qemu-iotests/iotests.py | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 6ba65eb1ff..47e3808ab9 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -39,6 +39,7 @@ > > from qemu.machine import qtest > from qemu.qmp import QMPMessage > +from qemu.aqmp.legacy import QEMUMonitorProtocol I thought we were trying to get rid of aqmp.legacy usage, so this feels like a temporary regression. Oh well, not the end of the testing world. > def stop(self, kill_signal=15): > self._p.send_signal(kill_signal) > self._p.wait() > self._p = None > > + if self._qmp: > + self._qmp.close() > + > try: > + if self._qmpsock is not None: > + os.remove(self._qmpsock) > os.remove(self.pidfile) > except OSError: > pass Do we need two try: blocks here, to remove self.pidfile even if os.remove(self._qmpsock) failed? Otherwise, makes sense to me.
On 15.02.22 23:19, Eric Blake wrote: > On Tue, Feb 15, 2022 at 02:57:26PM +0100, Hanna Reitz wrote: >> Add a parameter to optionally open a QMP connection when creating a >> QemuStorageDaemon instance. >> >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 29 ++++++++++++++++++++++++++++- >> 1 file changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index 6ba65eb1ff..47e3808ab9 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -39,6 +39,7 @@ >> >> from qemu.machine import qtest >> from qemu.qmp import QMPMessage >> +from qemu.aqmp.legacy import QEMUMonitorProtocol > I thought we were trying to get rid of aqmp.legacy usage, so this > feels like a temporary regression. Oh well, not the end of the > testing world. I fiddled around with the non-legacy interface and wasn’t very successful... I thought since machine.py still uses qemu.aqmp.legacy for QEMUMachine, when one is reworked to get rid of it (if that ever becomes necessary), then we can just do it here, too. > >> def stop(self, kill_signal=15): >> self._p.send_signal(kill_signal) >> self._p.wait() >> self._p = None >> >> + if self._qmp: >> + self._qmp.close() >> + >> try: >> + if self._qmpsock is not None: >> + os.remove(self._qmpsock) >> os.remove(self.pidfile) >> except OSError: >> pass > Do we need two try: blocks here, to remove self.pidfile even if > os.remove(self._qmpsock) failed? Honestly, no reason not to use two blocks except it being longer. You’re right, I should’ve just done that. > Otherwise, makes sense to me. Thanks for reviewing! Hanna
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6ba65eb1ff..47e3808ab9 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -39,6 +39,7 @@ from qemu.machine import qtest from qemu.qmp import QMPMessage +from qemu.aqmp.legacy import QEMUMonitorProtocol # Use this logger for logging messages directly from the iotests module logger = logging.getLogger('qemu.iotests') @@ -348,14 +349,30 @@ def cmd(self, cmd): class QemuStorageDaemon: - def __init__(self, *args: str, instance_id: str = 'a'): + _qmp: Optional[QEMUMonitorProtocol] = None + _qmpsock: Optional[str] = None + # Python < 3.8 would complain if this type were not a string literal + # (importing `annotations` from `__future__` would work; but not on <= 3.6) + _p: 'Optional[subprocess.Popen[bytes]]' = None + + def __init__(self, *args: str, instance_id: str = 'a', qmp: bool = False): assert '--pidfile' not in args self.pidfile = os.path.join(test_dir, f'qsd-{instance_id}-pid') all_args = [qsd_prog] + list(args) + ['--pidfile', self.pidfile] + if qmp: + self._qmpsock = os.path.join(sock_dir, f'qsd-{instance_id}.sock') + all_args += ['--chardev', + f'socket,id=qmp-sock,path={self._qmpsock}', + '--monitor', 'qmp-sock'] + + self._qmp = QEMUMonitorProtocol(self._qmpsock, server=True) + # Cannot use with here, we want the subprocess to stay around # pylint: disable=consider-using-with self._p = subprocess.Popen(all_args) + if self._qmp is not None: + self._qmp.accept() while not os.path.exists(self.pidfile): if self._p.poll() is not None: cmd = ' '.join(all_args) @@ -370,12 +387,22 @@ def __init__(self, *args: str, instance_id: str = 'a'): assert self._pid == self._p.pid + def qmp(self, cmd: str, args: Optional[Dict[str, object]] = None) \ + -> QMPMessage: + assert self._qmp is not None + return self._qmp.cmd(cmd, args) + def stop(self, kill_signal=15): self._p.send_signal(kill_signal) self._p.wait() self._p = None + if self._qmp: + self._qmp.close() + try: + if self._qmpsock is not None: + os.remove(self._qmpsock) os.remove(self.pidfile) except OSError: pass
Add a parameter to optionally open a QMP connection when creating a QemuStorageDaemon instance. Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)