Re: [PATCH 2/3] iotests: Allow using QMP with the QSD

[Hanna Reitz]

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