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

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

[Caio Carrara]

On Wed, Nov 21, 2018 at 01:39:00PM -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 set_qmp_monitor() that allow to
> launch the VM without creating the monitor machinery. The
> method can be used to re-enable the monitor eventually.
> 
> Tested this change with the following Avocado test:
> 
> from avocado_qemu import Test
> 
> class DisableQmp(Test):

I think it would be fine to have this test added as a real test instead
of just let this here on commit message. Or at least we should have a
real use case that uses this new feature.

>     """
>     :avocado: enable
>     """
>     def test(self):
>         self.vm.add_args('--foobar')
>         self.vm.set_qmp_monitor(disabled=True)
>         self.vm.launch()
>         self.vm.wait()
>         self.assertEqual(self.vm.exitcode(), 1)
>         self.vm.shutdown()
> 
>         self.vm.set_qmp_monitor(disabled=False)
>         self.vm._args.remove('--foobar') # Hack
>         self.vm.launch()
>         res = self.vm.command('human-monitor-command',
>                               command_line='info version')
>         self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes in v2:
>   * Major refactor: replaced disable_qmp() with set_qmp_monitor()
>     that allow to disable the qmp monitor likewise, but also one can
>     re-enable it (set_qmp_monitor(disabled=False)).
>   * The logic to enabled/disable the monitor is back to
>     _base_args(). [ccarrara, ehabkost]
> ---
>  scripts/qemu.py | 72 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 6e3b0e6771..54fe0e65d2 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 monitor 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:
> @@ -247,23 +240,33 @@ class QEMUMachine(object):
>                         self._console_address)
>              device = '%s,chardev=console' % self._console_device_type
>              args.extend(['-chardev', chardev, '-device', device])
> +        if self._with_qmp:
> +            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.extend(['-chardev', moncdev, '-mon', 'chardev=mon,mode=control'])
> +
>          return args
>  
>      def _pre_launch(self):
>          self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> -        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')
>  
> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
> -                                                server=True)
> -
> +        if self._with_qmp:
> +            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._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
> +                                                    server=True)
>      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 +328,8 @@ class QEMUMachine(object):
>          Wait for the VM to power off
>          """
>          self._popen.wait()
> -        self._qmp.close()
> +        if self._qmp:

Isn't the self._with_qmp attribute that should be used here? I think
it's important to standardize the checks for qmp availability in
`with_qmp` attribute and use the `qmp` only for qmp access itself. There
are other places that uses qmp to this kind of check too.

> +            self._qmp.close()
>          self._load_io_log()
>          self._post_shutdown()
>  
> @@ -334,11 +338,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 +362,23 @@ class QEMUMachine(object):
>  
>          self._launched = False
>  
> +    def set_qmp_monitor(self, disabled=False, monitor_address=None):

Where is this new method being used?

> +        """
> +        Set the QMP monitor.
> +
> +        @param disabled: if True, qmp monitor options will be removed from the
> +                         base arguments of the resulting QEMU command line.
> +        @param monitor_address: address for the QMP monitor.
> +        @note: call this function before launch().
> +        """
> +        if disabled:
> +            self._with_qmp = False
> +            self._qmp = None
> +        else:
> +            self._with_qmp = True
> +            if monitor_address:
> +                self._monitor_address = monitor_address
> +
>      def qmp(self, cmd, conv_keys=True, **args):
>          """
>          Invoke a QMP command and return the response dict
> -- 
> 2.19.1
> 

It would be nice see less scattered checks for qmp availability along
the code. 

Thanks.
-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com

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

[Wainer dos Santos Moschetta]

On 11/26/2018 10:58 AM, Caio Carrara wrote:
> On Wed, Nov 21, 2018 at 01:39:00PM -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 set_qmp_monitor() that allow to
>> launch the VM without creating the monitor machinery. The
>> method can be used to re-enable the monitor eventually.
>>
>> Tested this change with the following Avocado test:
>>
>> from avocado_qemu import Test
>>
>> class DisableQmp(Test):
> I think it would be fine to have this test added as a real test instead
> of just let this here on commit message. Or at least we should have a
> real use case that uses this new feature.

Currently we don't have a proper place to put tests for scripts/qemu.py, 
but [1] opens the opportunity to create a self-test structure for the 
avocado_qemu API. For now I suggest to keep that (self)test just on the 
commit message.

The feature I am introducing on this patch could have used on [2], so 
that it wouldn't need to call the qemu binary directly. I'm not changing 
that test on this patch because it was not merged into master yet, so I 
don't know exactly how it could be done.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg576435.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg572744.html

>
>>      """
>>      :avocado: enable
>>      """
>>      def test(self):
>>          self.vm.add_args('--foobar')
>>          self.vm.set_qmp_monitor(disabled=True)
>>          self.vm.launch()
>>          self.vm.wait()
>>          self.assertEqual(self.vm.exitcode(), 1)
>>          self.vm.shutdown()
>>
>>          self.vm.set_qmp_monitor(disabled=False)
>>          self.vm._args.remove('--foobar') # Hack
>>          self.vm.launch()
>>          res = self.vm.command('human-monitor-command',
>>                                command_line='info version')
>>          self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> Changes in v2:
>>    * Major refactor: replaced disable_qmp() with set_qmp_monitor()
>>      that allow to disable the qmp monitor likewise, but also one can
>>      re-enable it (set_qmp_monitor(disabled=False)).
>>    * The logic to enabled/disable the monitor is back to
>>      _base_args(). [ccarrara, ehabkost]
>> ---
>>   scripts/qemu.py | 72 ++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 48 insertions(+), 24 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 6e3b0e6771..54fe0e65d2 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 monitor 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:
>> @@ -247,23 +240,33 @@ class QEMUMachine(object):
>>                          self._console_address)
>>               device = '%s,chardev=console' % self._console_device_type
>>               args.extend(['-chardev', chardev, '-device', device])
>> +        if self._with_qmp:
>> +            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.extend(['-chardev', moncdev, '-mon', 'chardev=mon,mode=control'])
>> +
>>           return args
>>   
>>       def _pre_launch(self):
>>           self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
>> -        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')
>>   
>> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
>> -                                                server=True)
>> -
>> +        if self._with_qmp:
>> +            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._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
>> +                                                    server=True)
>>       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 +328,8 @@ class QEMUMachine(object):
>>           Wait for the VM to power off
>>           """
>>           self._popen.wait()
>> -        self._qmp.close()
>> +        if self._qmp:
> Isn't the self._with_qmp attribute that should be used here? I think
> it's important to standardize the checks for qmp availability in
> `with_qmp` attribute and use the `qmp` only for qmp access itself. There
> are other places that uses qmp to this kind of check too.

Checking self._qmp has same result as self._with_qmp given that 
self._qmp object is only created with qmp enabled. But I agree with you 
that for the sake of readability it would be better to check with 
self._with_qmp.  I will send a v2.

>
>> +            self._qmp.close()
>>           self._load_io_log()
>>           self._post_shutdown()
>>   
>> @@ -334,11 +338,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 +362,23 @@ class QEMUMachine(object):
>>   
>>           self._launched = False
>>   
>> +    def set_qmp_monitor(self, disabled=False, monitor_address=None):
> Where is this new method being used?

It's not used yet, but we will need this on some type of tests (e.g. 
launch the VM and expect an immediate crash) very often. I mentioned 
above one test that I could have used it already.

> .
>
>> +        """
>> +        Set the QMP monitor.
>> +
>> +        @param disabled: if True, qmp monitor options will be removed from the
>> +                         base arguments of the resulting QEMU command line.
>> +        @param monitor_address: address for the QMP monitor.
>> +        @note: call this function before launch().
>> +        """
>> +        if disabled:
>> +            self._with_qmp = False
>> +            self._qmp = None
>> +        else:
>> +            self._with_qmp = True
>> +            if monitor_address:
>> +                self._monitor_address = monitor_address
>> +
>>       def qmp(self, cmd, conv_keys=True, **args):
>>           """
>>           Invoke a QMP command and return the response dict
>> -- 
>> 2.19.1
>>
> It would be nice see less scattered checks for qmp availability along
> the code.

I agree. It will need a major refactor on scripts/qemu.py though. I want 
to keep this patch  as simple as possible so that it can be used on the 
ongoing tests implementation.

Thanks for the review!

- Wainer


>   
>
> Thanks.

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

[Caio Carrara]

Hi Wainer,

Thanks for your reply. I pretty much agreed with most of it. I just
added a comment about that new method that is not used anywere.

On Thu, Nov 29, 2018 at 10:45:33AM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/26/2018 10:58 AM, Caio Carrara wrote:
> > On Wed, Nov 21, 2018 at 01:39:00PM -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 set_qmp_monitor() that allow to
> > > launch the VM without creating the monitor machinery. The
> > > method can be used to re-enable the monitor eventually.
> > > 
> > > Tested this change with the following Avocado test:
> > > 
> > > from avocado_qemu import Test
> > > 
> > > class DisableQmp(Test):
> > I think it would be fine to have this test added as a real test instead
> > of just let this here on commit message. Or at least we should have a
> > real use case that uses this new feature.
> 
> Currently we don't have a proper place to put tests for scripts/qemu.py, but
> [1] opens the opportunity to create a self-test structure for the
> avocado_qemu API. For now I suggest to keep that (self)test just on the
> commit message.
> 
> The feature I am introducing on this patch could have used on [2], so that
> it wouldn't need to call the qemu binary directly. I'm not changing that
> test on this patch because it was not merged into master yet, so I don't
> know exactly how it could be done.
> 
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg576435.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg572744.html
> 
> > 
> > >      """
> > >      :avocado: enable
> > >      """
> > >      def test(self):
> > >          self.vm.add_args('--foobar')
> > >          self.vm.set_qmp_monitor(disabled=True)
> > >          self.vm.launch()
> > >          self.vm.wait()
> > >          self.assertEqual(self.vm.exitcode(), 1)
> > >          self.vm.shutdown()
> > > 
> > >          self.vm.set_qmp_monitor(disabled=False)
> > >          self.vm._args.remove('--foobar') # Hack
> > >          self.vm.launch()
> > >          res = self.vm.command('human-monitor-command',
> > >                                command_line='info version')
> > >          self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes in v2:
> > >    * Major refactor: replaced disable_qmp() with set_qmp_monitor()
> > >      that allow to disable the qmp monitor likewise, but also one can
> > >      re-enable it (set_qmp_monitor(disabled=False)).
> > >    * The logic to enabled/disable the monitor is back to
> > >      _base_args(). [ccarrara, ehabkost]
> > > ---
> > >   scripts/qemu.py | 72 ++++++++++++++++++++++++++++++++-----------------
> > >   1 file changed, 48 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > > index 6e3b0e6771..54fe0e65d2 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
[...]
> > >       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 +328,8 @@ class QEMUMachine(object):
> > >           Wait for the VM to power off
> > >           """
> > >           self._popen.wait()
> > > -        self._qmp.close()
> > > +        if self._qmp:
> > Isn't the self._with_qmp attribute that should be used here? I think
> > it's important to standardize the checks for qmp availability in
> > `with_qmp` attribute and use the `qmp` only for qmp access itself. There
> > are other places that uses qmp to this kind of check too.
> 
> Checking self._qmp has same result as self._with_qmp given that self._qmp
> object is only created with qmp enabled. But I agree with you that for the
> sake of readability it would be better to check with self._with_qmp.  I will
> send a v2.
> 
> > 
> > > +            self._qmp.close()
> > >           self._load_io_log()
> > >           self._post_shutdown()
> > > @@ -334,11 +338,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 +362,23 @@ class QEMUMachine(object):
> > >           self._launched = False
> > > +    def set_qmp_monitor(self, disabled=False, monitor_address=None):
> > Where is this new method being used?
> 
> It's not used yet, but we will need this on some type of tests (e.g. launch
> the VM and expect an immediate crash) very often. I mentioned above one test
> that I could have used it already.

As you mentioned, for the sake of keeping this patch as simple as
possible and also reduce the amount of unused code in the repository, I
would suggest you to only send this new method with a patch that
actually use it.

> 
> > .
> > 
> > > +        """
> > > +        Set the QMP monitor.
> > > +
> > > +        @param disabled: if True, qmp monitor options will be removed from the
> > > +                         base arguments of the resulting QEMU command line.
> > > +        @param monitor_address: address for the QMP monitor.
> > > +        @note: call this function before launch().
> > > +        """
> > > +        if disabled:
> > > +            self._with_qmp = False
> > > +            self._qmp = None
> > > +        else:
> > > +            self._with_qmp = True
> > > +            if monitor_address:
> > > +                self._monitor_address = monitor_address
> > > +
> > >       def qmp(self, cmd, conv_keys=True, **args):
> > >           """
> > >           Invoke a QMP command and return the response dict
> > > -- 
> > > 2.19.1
> > > 
> > It would be nice see less scattered checks for qmp availability along
> > the code.
> 
> I agree. It will need a major refactor on scripts/qemu.py though. I want to
> keep this patch  as simple as possible so that it can be used on the ongoing
> tests implementation.
> 
> Thanks for the review!
> 
> - Wainer
> 
> 
> > 
> > Thanks.
> 

Thanks,
-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com