Re: [Qemu-devel] [PATCH v2] scripts/qemu.py: allow to launch the VM without a monitor

[Caio Carrara]

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/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
[...]
> > >       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
address@hidden