Re: [PATCH 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

On 12/10/19 3:17 AM, Cleber Rosa wrote:
> On Tue, Nov 12, 2019 at 08:58:00AM -0500, Wainer dos Santos Moschetta wrote:
>> The QEMUMachine VM has a monitor setup on which an QMP
>> connection is always attempted on _post_launch() (executed
>> by launch()). In case the QEMU process immediatly exits
>> then the qmp.accept() (used to establish the connection) stalls
>> until it reaches timeout and consequently an exception raises.
>>
>> That behavior is undesirable when, for instance, it needs to
>> gather information from the QEMU binary ($ qemu -cpu list) or a
>> test which launches the VM expecting its failure.
>>
>> This patch adds the set_qmp_monitor() method to QEMUMachine that
>> allows turn off the creation of the monitor machinery on VM launch.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   python/qemu/machine.py | 68 ++++++++++++++++++++++++++++--------------
>>   1 file changed, 45 insertions(+), 23 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index a4631d6934..04ee86e1ba 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -104,6 +104,7 @@ class QEMUMachine(object):
>>           self._events = []
>>           self._iolog = None
>>           self._socket_scm_helper = socket_scm_helper
>> +        self._qmp_set = True   # Enable QMP monitor by default.
>>           self._qmp = None
>>           self._qemu_full_args = None
>>           self._test_dir = test_dir
>> @@ -228,15 +229,16 @@ class QEMUMachine(object):
>>                   self._iolog = iolog.read()
>>   
>>       def _base_args(self):
>> -        if isinstance(self._monitor_address, tuple):
>> -            moncdev = "socket,id=mon,host=%s,port=%s" % (
>> +        args = ['-display', 'none', '-vga', 'none']
>> +        if self._qmp_set:
>> +            if isinstance(self._monitor_address, tuple):
>> +                moncdev = "socket,id=mon,host=%s,port=%s" % (
>>                   self._monitor_address[0],
>>                   self._monitor_address[1])
>> -        else:
>> -            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
>> -        args = ['-chardev', moncdev,
>> -                '-mon', 'chardev=mon,mode=control',
>> -                '-display', 'none', '-vga', 'none']
>> +            else:
>> +                moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
>> +            args.extend(['-chardev', moncdev, '-mon',
>> +                         'chardev=mon,mode=control'])
>>           if self._machine is not None:
>>               args.extend(['-machine', self._machine])
>>           if self._console_set:
>> @@ -255,20 +257,21 @@ class QEMUMachine(object):
>>   
>>       def _pre_launch(self):
>>           self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
>> -        if self._monitor_address is not None:
>> -            self._vm_monitor = self._monitor_address
>> -        else:
>> -            self._vm_monitor = os.path.join(self._sock_dir,
>> -                                            self._name + "-monitor.sock")
>> -            self._remove_files.append(self._vm_monitor)
>>           self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
>>           self._qemu_log_file = open(self._qemu_log_path, 'wb')
>>   
>> -        self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
>> -                                            server=True)
>> +        if self._qmp_set:
>> +            if self._monitor_address is not None:
>> +                self._vm_monitor = self._monitor_address
>> +            else:
>> +                self._vm_monitor = os.path.join(self._sock_dir,
>> +                                                self._name + "-monitor.sock")
>> +                self._remove_files.append(self._vm_monitor)
>> +            self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True)
>>   
>>       def _post_launch(self):
>> -        self._qmp.accept()
>> +        if self._qmp:
>> +            self._qmp.accept()
>>   
>>       def _post_shutdown(self):
>>           if self._qemu_log_file is not None:
>> @@ -330,7 +333,8 @@ class QEMUMachine(object):
>>           Wait for the VM to power off
>>           """
>>           self._popen.wait()
>> -        self._qmp.close()
>> +        if self._qmp:
>> +            self._qmp.close()
>>           self._load_io_log()
>>           self._post_shutdown()
>>   
>> @@ -346,12 +350,13 @@ class QEMUMachine(object):
>>               self._console_socket = None
>>   
>>           if self.is_running():
>> -            try:
>> -                if not has_quit:
>> -                    self._qmp.cmd('quit')
>> -                self._qmp.close()
>> -            except:
>> -                self._popen.kill()
>> +            if self._qmp:
>> +                try:
>> +                    if not has_quit:
>> +                        self._qmp.cmd('quit')
>> +                    self._qmp.close()
>> +                except:
>> +                    self._popen.kill()
>>               self._popen.wait()
>>   
>>           self._load_io_log()
>> @@ -368,6 +373,23 @@ class QEMUMachine(object):
>>   
>>           self._launched = False
>>   
>> +    def set_qmp_monitor(self, disabled=False, monitor_address=None):
>> +        """
>> +        Set the QMP monitor.
>> +
>> +        @param disabled: if True, qmp monitor options will be removed from the
>> +                         base arguments of the resulting QEMU command line.
> I personally tend avoid double negatives as long as I'm aware of them.
> Wouldn't "enabled=True" be more straightforward?  Just my personal
> preference though.

I don't have a strong opinion about double negatives. So I'm fine with 
use 'enabled' instead. Besides make you happier. ;)

>
>> +        @param monitor_address: address for the QMP monitor.
> Do you have a use case for passing the monitor address here too, in
> addition to also being available as a parameter to __init__()?  In the
> next patch, I don't see it being used (or here for that matter).

I thought it would be useful in case an acceptance test needs to set the 
monitor address. And because avocado_qemu transparently creates the 
QEMUMachine instance, the test doesn't have access to the constructor. 
But no, I don't have a concrete case. I'm just being overcareful, so I 
don't mind to remove that parameter.

Good catch. Thanks!

- Wainer

>
>> +        @note: call this function before launch().
>> +        """
>> +        if disabled:
>> +            self._qmp_set = False
>> +            self._qmp = None
>> +        else:
>> +            self._qmp_set = True
>> +            if monitor_address:
>> +                self._monitor_address = monitor_address
>> +
>>       def qmp(self, cmd, conv_keys=True, **args):
>>           """
>>           Invoke a QMP command and return the response dict
>> -- 
>> 2.18.1
>>
> Other than those comments, it LGTM.
>
> - Cleber.