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

[Caio Carrara]

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
address@hidden