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

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

On 11/21/2018 01:56 PM, 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?

It does. I will send a v2 on that line.

Thanks for the review Caio and Eduardo!

- Wainer

>
> {...}
>> -- 
>> Eduardo