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
 
 # 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
-- 
2.34.1

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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