|
From: | Wainer dos Santos Moschetta |
Subject: | Re: [Qemu-devel] [PATCH v2] scripts/qemu.py: allow to launch the VM without a monitor |
Date: | Thu, 29 Nov 2018 10:45:33 -0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
On 11/26/2018 10:58 AM, Caio Carrara wrote:
On Wed, Nov 21, 2018 at 01:39:00PM -0500, Wainer dos Santos Moschetta wrote:QEMUMachine launches the VM with a monitor enabled, afterwards a qmp connection is attempted on _post_launch(). In case the QEMU process exits with an error, qmp.accept() reaches timeout and raises an exception. But sometimes you don't need that monitor. As an example, when a test launches the VM expects its immediate crash, and only intend to check the process's return code. In this case the fact that launch() tries to establish the qmp connection (ending up in an exception) is troublesome. So this patch adds the set_qmp_monitor() that allow to launch the VM without creating the monitor machinery. The method can be used to re-enable the monitor eventually. Tested this change with the following Avocado test: from avocado_qemu import Test class DisableQmp(Test):I think it would be fine to have this test added as a real test instead of just let this here on commit message. Or at least we should have a real use case that uses this new feature.
Currently we don't have a proper place to put tests for scripts/qemu.py, but [1] opens the opportunity to create a self-test structure for the avocado_qemu API. For now I suggest to keep that (self)test just on the commit message.
The feature I am introducing on this patch could have used on [2], so that it wouldn't need to call the qemu binary directly. I'm not changing that test on this patch because it was not merged into master yet, so I don't know exactly how it could be done.
[1] https://www.mail-archive.com/address@hidden/msg576435.html [2] https://www.mail-archive.com/address@hidden/msg572744.html
""" :avocado: enable """ def test(self): self.vm.add_args('--foobar') self.vm.set_qmp_monitor(disabled=True) self.vm.launch() self.vm.wait() self.assertEqual(self.vm.exitcode(), 1) self.vm.shutdown() self.vm.set_qmp_monitor(disabled=False) self.vm._args.remove('--foobar') # Hack self.vm.launch() res = self.vm.command('human-monitor-command', command_line='info version') self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)') Signed-off-by: Wainer dos Santos Moschetta <address@hidden> Reviewed-by: Caio Carrara <address@hidden> Reviewed-by: Eduardo Habkost <address@hidden> --- Changes in v2: * Major refactor: replaced disable_qmp() with set_qmp_monitor() that allow to disable the qmp monitor likewise, but also one can re-enable it (set_qmp_monitor(disabled=False)). * The logic to enabled/disable the monitor is back to _base_args(). [ccarrara, ehabkost] --- scripts/qemu.py | 72 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 6e3b0e6771..54fe0e65d2 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -115,6 +115,7 @@ class QEMUMachine(object): self._events = [] self._iolog = None self._socket_scm_helper = socket_scm_helper + self._with_qmp = True # Enable QMP monitor by default. self._qmp = None self._qemu_full_args = None self._test_dir = test_dir @@ -229,15 +230,7 @@ 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" % ( - 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'] + args = ['-display', 'none', '-vga', 'none'] if self._machine is not None: args.extend(['-machine', self._machine]) if self._console_device_type is not None: @@ -247,23 +240,33 @@ class QEMUMachine(object): self._console_address) device = '%s,chardev=console' % self._console_device_type args.extend(['-chardev', chardev, '-device', device]) + if self._with_qmp: + 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.extend(['-chardev', moncdev, '-mon', 'chardev=mon,mode=control']) + return argsdef _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._temp_dir, - self._name + "-monitor.sock") 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.qmp.QEMUMonitorProtocol(self._vm_monitor,- server=True) - + if self._with_qmp: + if self._monitor_address is not None: + self._vm_monitor = self._monitor_address + else: + self._vm_monitor = os.path.join(self._temp_dir, + self._name + "-monitor.sock") + self._qmp = 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: @@ -325,7 +328,8 @@ class QEMUMachine(object): Wait for the VM to power off """ self._popen.wait() - self._qmp.close() + if self._qmp:Isn't the self._with_qmp attribute that should be used here? I think it's important to standardize the checks for qmp availability in `with_qmp` attribute and use the `qmp` only for qmp access itself. There are other places that uses qmp to this kind of check too.
Checking self._qmp has same result as self._with_qmp given that self._qmp object is only created with qmp enabled. But I agree with you that for the sake of readability it would be better to check with self._with_qmp. I will send a v2.
+ self._qmp.close() self._load_io_log() self._post_shutdown()@@ -334,11 +338,14 @@ class QEMUMachine(object):Terminate the VM and clean up """ if self.is_running(): - try: - self._qmp.cmd('quit') - self._qmp.close() - except: - self._popen.kill() + if self._qmp: + try: + self._qmp.cmd('quit') + self._qmp.close() + except: + self._popen.kill() + else: + self._popen.terminate() self._popen.wait()self._load_io_log()@@ -355,6 +362,23 @@ class QEMUMachine(object):self._launched = False + def set_qmp_monitor(self, disabled=False, monitor_address=None):Where is this new method being used?
It's not used yet, but we will need this on some type of tests (e.g. launch the VM and expect an immediate crash) very often. I mentioned above one test that I could have used it already.
.+ """ + Set the QMP monitor. + + @param disabled: if True, qmp monitor options will be removed from the + base arguments of the resulting QEMU command line. + @param monitor_address: address for the QMP monitor. + @note: call this function before launch(). + """ + if disabled: + self._with_qmp = False + self._qmp = None + else: + self._with_qmp = 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.19.1It would be nice see less scattered checks for qmp availability along the code.
I agree. It will need a major refactor on scripts/qemu.py though. I want to keep this patch as simple as possible so that it can be used on the ongoing tests implementation.
Thanks for the review! - Wainer
Thanks.
[Prev in Thread] | Current Thread | [Next in Thread] |