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?
>
{...}
>
> --
> Eduardo
--
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarr...@redhat.com
