On Wed, Nov 21, 2018 at 01:56:00PM -0200, Caio Carrara wrote:
> Hello Wainer and Eduardo,
>
> On Tue, Nov 20, 2018 at 05:02:38PM -0200, Eduardo Habkost wrote:
> > On Tue, Nov 20, 2018 at 11:53:00AM -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 disable_qmp() that allow to
> > > launch the VM without creating the monitor machinery.
> > >
> {...}
> > > +
> > > + self._args.extend(['-chardev', moncdev, '-mon',
> > > 'chardev=mon,mode=control'])
> >
> > This will extend self._args multiple times if making a
> > .shutdown()/.launch() cycle:
> >
> {...}
> >
> > Why did you move this code outside _base_args(), where it already
> > worked without relying on method side-effects and required no
> > extra state to be kept inside self._args?
>
> Eduardo, I think the purpose was to get a way to set up the monitor
> conditionally. So the arguments related with monitor was moved out
> _base_args() method and put into the _setup_qmp(). However, as you
> showed, this implementation has undesired side-effects.
>
> Wainer, probably the most straightforward way to add this capability to
> QEMUMachine is to change the _base_args() method to only include monitor
> arguments when necessary. Something like this:
>
> ```
> # create a proper method to get moncdev_args
> def _get_moncdev_args():
> if isinstance(self._monitor_address, tuple):
> return "socket,id=mon,host=%s,port=%s" % (
> self._monitor_address[0],
> self._monitor_address[1])
> else:
> return 'socket,id=mon,path=%s' % self._vm_monitor
>
> # update _base_args method to use new attribute _with_qmp
> def _base_args(self):
> args = ['-display', 'none', '-vga', 'none']
> if self._with_qmp:
> args.extend(['-chardev', self._get_moncdev_args(),
> '-mon', 'chardev=mon,mode=control'])
> if self._machine is not None:
> ```
>
> Does it make sense?
Yes, this is what I was expecting. Thanks!
--
Eduardo
