Re: [Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error

2017-08-31 Thread Lukáš Doktor
Dne 18.8.2017 v 19:05 Amador Pahim napsal(a):
> When launching a VM, if an exception happens and the VM is not
> initiated, it might be useful to see the qemu command line and
> the qemu command output.
> 
> This patch creates that message. Notice that self._iolog needs to be
> cleaned up in the beginning of the launch() to make sure we will not
> expose the qemu log from a previous launch if the current one fails.
> 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 0bcec4b3b1..29fd2469f9 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -147,6 +147,7 @@ class QEMUMachine(object):
>  
>  def launch(self):
>  '''Launch the VM and establish a QMP connection'''
> +self._iolog = None
>  self._qemu_full_args = None
>  devnull = open(os.path.devnull, 'rb')
>  qemulog = open(self._qemu_log_path, 'wb')
> @@ -162,6 +163,13 @@ class QEMUMachine(object):
>  self._post_launch()
>  except:
>  self.shutdown()
> +
> +LOG.debug('Error launching VM')
> +if self._qemu_full_args:
> +LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
> +if self._iolog:
> +LOG.debug('Output: %r', self._iolog)
> +
>  raise
>  

I don't want to open the cave, but anybody else would actually prefer custom 
Exception with the qemu_full_args, iolog and details about the original 
exception to this? Users who don't care would see/log all the details from the 
`exception.__str__` and those who are interested in the original exception 
would query for it eg. via `exception.original_exception`.

Anyway even this change is IMO step into the right direction and we can (and 
should) refine the use of exceptions in following patches (as Markus spotted, 
pure Exceptions and other ugliness lives in those waters).

As for the debug vs. error I'd actually stick to "debug", because in negative 
testing the logged message is only an expected debug and in positive testing it 
expands the knowledge of the flow, but the actual error message should come 
from the exception. Not a strong opinion, though...

Regards,
Lukáš

>  def shutdown(self):
> 




signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error

2017-08-30 Thread Fam Zheng
On Wed, 08/30 11:55, Cleber Rosa wrote:
> 
> 
> On 08/18/2017 01:05 PM, Amador Pahim wrote:
> > When launching a VM, if an exception happens and the VM is not
> > initiated, it might be useful to see the qemu command line and
> > the qemu command output.
> > 
> > This patch creates that message. Notice that self._iolog needs to be
> > cleaned up in the beginning of the launch() to make sure we will not
> > expose the qemu log from a previous launch if the current one fails.
> > 
> > Signed-off-by: Amador Pahim 
> > ---
> >  scripts/qemu.py | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index 0bcec4b3b1..29fd2469f9 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -147,6 +147,7 @@ class QEMUMachine(object):
> >  
> >  def launch(self):
> >  '''Launch the VM and establish a QMP connection'''
> > +self._iolog = None
> >  self._qemu_full_args = None
> >  devnull = open(os.path.devnull, 'rb')
> >  qemulog = open(self._qemu_log_path, 'wb')
> > @@ -162,6 +163,13 @@ class QEMUMachine(object):
> >  self._post_launch()
> >  except:
> >  self.shutdown()
> > +
> > +LOG.debug('Error launching VM')
> > +if self._qemu_full_args:
> > +LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
> > +if self._iolog:
> > +LOG.debug('Output: %r', self._iolog)
> > +
> 
> Based on Fam's comment about the signal message being a warning (worth
> showing by default), I also think this deserves more than a "debug"
> classification.

Yes, LOG.error, please.

Fam



Re: [Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error

2017-08-30 Thread Cleber Rosa


On 08/18/2017 01:05 PM, Amador Pahim wrote:
> When launching a VM, if an exception happens and the VM is not
> initiated, it might be useful to see the qemu command line and
> the qemu command output.
> 
> This patch creates that message. Notice that self._iolog needs to be
> cleaned up in the beginning of the launch() to make sure we will not
> expose the qemu log from a previous launch if the current one fails.
> 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 0bcec4b3b1..29fd2469f9 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -147,6 +147,7 @@ class QEMUMachine(object):
>  
>  def launch(self):
>  '''Launch the VM and establish a QMP connection'''
> +self._iolog = None
>  self._qemu_full_args = None
>  devnull = open(os.path.devnull, 'rb')
>  qemulog = open(self._qemu_log_path, 'wb')
> @@ -162,6 +163,13 @@ class QEMUMachine(object):
>  self._post_launch()
>  except:
>  self.shutdown()
> +
> +LOG.debug('Error launching VM')
> +if self._qemu_full_args:
> +LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
> +if self._iolog:
> +LOG.debug('Output: %r', self._iolog)
> +

Based on Fam's comment about the signal message being a warning (worth
showing by default), I also think this deserves more than a "debug"
classification.

- Cleber.

>  raise
>  
>  def shutdown(self):
> 



signature.asc
Description: OpenPGP digital signature


[Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error

2017-08-18 Thread Amador Pahim
When launching a VM, if an exception happens and the VM is not
initiated, it might be useful to see the qemu command line and
the qemu command output.

This patch creates that message. Notice that self._iolog needs to be
cleaned up in the beginning of the launch() to make sure we will not
expose the qemu log from a previous launch if the current one fails.

Signed-off-by: Amador Pahim 
---
 scripts/qemu.py | 8 
 1 file changed, 8 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 0bcec4b3b1..29fd2469f9 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -147,6 +147,7 @@ class QEMUMachine(object):
 
 def launch(self):
 '''Launch the VM and establish a QMP connection'''
+self._iolog = None
 self._qemu_full_args = None
 devnull = open(os.path.devnull, 'rb')
 qemulog = open(self._qemu_log_path, 'wb')
@@ -162,6 +163,13 @@ class QEMUMachine(object):
 self._post_launch()
 except:
 self.shutdown()
+
+LOG.debug('Error launching VM')
+if self._qemu_full_args:
+LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
+if self._iolog:
+LOG.debug('Output: %r', self._iolog)
+
 raise
 
 def shutdown(self):
-- 
2.13.5