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

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

[Eduardo Habkost]

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.
> 
> Tested this change with the following Avocado test:
> 
> from avocado_qemu import Test
> class DisableQmp(Test):
>     """
>     :avocado: enable
>     """
>     def test(self):
>         self.vm.add_args('--foobar')
>         self.vm.disable_qmp()
>         self.vm.launch()
>         self.vm.wait()
>         self.assertEqual(self.vm.exitcode(), 1)
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
[...]
> ---
>  scripts/qemu.py | 58 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 6e3b0e6771..bbe80ac601 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
> +        self._with_qmp = True   # Enable QMP by default.
>          self._qmp = None
>          self._qemu_full_args = None
>          self._test_dir = test_dir
> @@ -229,15 +230,7 @@ class QEMUMachine(object):
>                  self._iolog = iolog.read()
>  
>      def _base_args(self):
> -        if isinstance(self._monitor_address, tuple):
> -            moncdev = "socket,id=mon,host=%s,port=%s" % (
> -                self._monitor_address[0],
> -                self._monitor_address[1])
> -        else:
> -            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
> -        args = ['-chardev', moncdev,
> -                '-mon', 'chardev=mon,mode=control',
> -                '-display', 'none', '-vga', 'none']
> +        args = ['-display', 'none', '-vga', 'none']
>          if self._machine is not None:
>              args.extend(['-machine', self._machine])
>          if self._console_device_type is not None:
> @@ -249,21 +242,35 @@ class QEMUMachine(object):
>              args.extend(['-chardev', chardev, '-device', device])
>          return args
>  
> -    def _pre_launch(self):
> -        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> +    def _setup_qmp(self):
> +
>          if self._monitor_address is not None:
>              self._vm_monitor = self._monitor_address
>          else:
>              self._vm_monitor = os.path.join(self._temp_dir,
>                                              self._name + "-monitor.sock")
> -        self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
> -        self._qemu_log_file = open(self._qemu_log_path, 'wb')
>  
> +        if isinstance(self._monitor_address, tuple):
> +            moncdev = "socket,id=mon,host=%s,port=%s" % (
> +                self._monitor_address[0],
> +                self._monitor_address[1])
> +        else:
> +            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
> +
> +        self._args.extend(['-chardev', moncdev, '-mon', 'chardev=mon,mode=control'])

This will extend self._args multiple times if making a
.shutdown()/.launch() cycle:

  $ cat scripts/test.py
  import qemu
  import logging, sys
  logging.basicConfig(stream=sys.stderr, level=logging.DEBUG)
  vm = qemu.QEMUMachine('./x86_64-softmmu/qemu-system-x86_64')
  vm.launch()
  vm.shutdown()
  vm.launch()
  vm.shutdown()
  $ python3 scripts/test.py
  DEBUG:QMP:>>> {'execute': 'qmp_capabilities'}
  DEBUG:QMP:<<< {'return': {}}
  DEBUG:QMP:>>> {'execute': 'quit'}
  DEBUG:QMP:<<< {'return': {}}
  ^CDEBUG:qemu:Error launching VM
  DEBUG:qemu:Command: './x86_64-softmmu/qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/var/tmp/tmpzny4hpy5/qemu-13737-monitor.sock -mon chardev=mon,mode=control -chardev socket,id=mon,path=/var/tmp/tmpkrlv1d9w/qemu-13737-monitor.sock -mon chardev=mon,mode=control'
  DEBUG:qemu:Output: "qemu-system-x86_64: -chardev socket,id=mon,path=/var/tmp/tmpkrlv1d9w/qemu-13737-monitor.sock: Duplicate ID 'mon' for chardev\n"
  Traceback (most recent call last):
    File "scripts/test.py", line 7, in <module>
      vm.launch()
    File "/home/ehabkost/rh/proj/virt/qemu/scripts/qemu.py", line 302, in launch
      self._launch()
    File "/home/ehabkost/rh/proj/virt/qemu/scripts/qemu.py", line 328, in _launch
      self._post_launch()
    File "/home/ehabkost/rh/proj/virt/qemu/scripts/qemu.py", line 273, in _post_launch
      self._qmp.accept()
    File "/home/ehabkost/rh/proj/virt/qemu/scripts/qmp/qmp.py", line 155, in accept
      self.__sock, _ = self.__sock.accept()
    File "/usr/lib64/python3.6/socket.py", line 205, in accept
      fd, addr = self._accept()
  KeyboardInterrupt

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?



>          self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
>                                                  server=True)
>  
> +    def _pre_launch(self):
> +        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> +        if self._with_qmp:
> +            self._setup_qmp()
> +        self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
> +        self._qemu_log_file = open(self._qemu_log_path, 'wb')
> +
>      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 +332,8 @@ class QEMUMachine(object):
>          Wait for the VM to power off
>          """
>          self._popen.wait()
> -        self._qmp.close()
> +        if self._qmp:
> +            self._qmp.close()
>          self._load_io_log()
>          self._post_shutdown()
>  
> @@ -334,11 +342,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 +366,13 @@ class QEMUMachine(object):
>  
>          self._launched = False
>  
> +    def disable_qmp(self):
> +        """
> +        Disable the QMP monitor.
> +        @note: call this function before launch().
> +        """
> +        self._with_qmp = False
> +
>      def qmp(self, cmd, conv_keys=True, **args):
>          """
>          Invoke a QMP command and return the response dict
> -- 
> 2.19.1
> 

-- 
Eduardo

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
ccarrara@redhat.com

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

[Eduardo Habkost]

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

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

[Wainer dos Santos Moschetta]

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