Hi Wainer, Thanks for your reply. I pretty much agreed with most of it. I just added a comment about that new method that is not used anywere.
On Thu, Nov 29, 2018 at 10:45:33AM -0200, Wainer dos Santos Moschetta wrote: > > 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/qemu-devel@nongnu.org/msg576435.html > [2] https://www.mail-archive.com/qemu-devel@nongnu.org/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 <waine...@redhat.com> > > > Reviewed-by: Caio Carrara <ccarr...@redhat.com> > > > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> > > > --- > > > 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 [...] > > > 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. As you mentioned, for the sake of keeping this patch as simple as possible and also reduce the amount of unused code in the repository, I would suggest you to only send this new method with a patch that actually use it. > > > . > > > > > + """ > > > + 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.1 > > > > > It 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. > Thanks, -- Caio Carrara Software Engineer, Virt Team - Red Hat ccarr...@redhat.com
