[v5,6/6] qemu.py: include qemu command line and output on launch error

When launching a VM, if an exception happens and the VM is not
initiated, it is useful to see the qemu command line that was executed
and the output of that command.

Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

On Tue, Jul 25, 2017 at 07:10:14PM +0200, Amador Pahim wrote:
> When launching a VM, if an exception happens and the VM is not
> initiated, it is useful to see the qemu command line that was executed
> and the output of that command.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index e18e8ec657..abfa3eebe9 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -131,7 +131,8 @@ class QEMUMachine(object):
>  
>      def launch(self):
>          '''
> -        Try to launch the VM and make sure we cleanup on exception.
> +        Try to launch the VM and make sure we cleanup and expose the
> +        command line/output in case of exception.
>          '''
>          if self.is_running():
>              return
> @@ -142,18 +143,24 @@ class QEMUMachine(object):
>              self.shutdown()
>  
>          try:
> -            self._launch()
> +            args = None
> +            args = self._wrapper + [self._binary] + self._base_args() + self._args
> +            self._launch(args)

I think it would be easier to set a self._full_qemu_args
attribute instead of passing data around through local variables
and method arguments.

>              self._shutdown_pending = True
>          except:
>              self.shutdown()
> +            sys.stderr.write('Error launching VM.\n%s%s' %
> +                             ('Command:\n%s\n' %
> +                              ' '.join(args) if args else '',
> +                              'Output:\n%s\n' %
> +                              ''.join(self._iolog) if self._iolog else ''))

I suggest making this conditional on self._debug.

We could also introduce a QEMULaunchError exception class
containing this info, but I'm unsure if we should raise it on
every exception, or raise more specific custom exceptions on
specific cases (e.g. one exception for QMP timeout, another for
QEMU binary not found, another for QEMU killed by a signal, etc).


>              raise
>  
> -    def _launch(self):
> +    def _launch(self, args):
>          '''Launch the VM and establish a QMP connection.'''
>          devnull = open('/dev/null', 'rb')
>          qemulog = open(self._qemu_log_path, 'wb')
>          self._pre_launch()
> -        args = self._wrapper + [self._binary] + self._base_args() + self._args
>          self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
>                                         stderr=subprocess.STDOUT, shell=False)
>          self._post_launch()
> -- 
> 2.13.3
>

On Tue, Jul 25, 2017 at 11:17 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Jul 25, 2017 at 07:10:14PM +0200, Amador Pahim wrote:
>> When launching a VM, if an exception happens and the VM is not
>> initiated, it is useful to see the qemu command line that was executed
>> and the output of that command.
>>
>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> ---
>>  scripts/qemu.py | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index e18e8ec657..abfa3eebe9 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -131,7 +131,8 @@ class QEMUMachine(object):
>>
>>      def launch(self):
>>          '''
>> -        Try to launch the VM and make sure we cleanup on exception.
>> +        Try to launch the VM and make sure we cleanup and expose the
>> +        command line/output in case of exception.
>>          '''
>>          if self.is_running():
>>              return
>> @@ -142,18 +143,24 @@ class QEMUMachine(object):
>>              self.shutdown()
>>
>>          try:
>> -            self._launch()
>> +            args = None
>> +            args = self._wrapper + [self._binary] + self._base_args() + self._args
>> +            self._launch(args)
>
> I think it would be easier to set a self._full_qemu_args
> attribute instead of passing data around through local variables
> and method arguments.

Ok, matter of preference. I'm ok with both.

>
>>              self._shutdown_pending = True
>>          except:
>>              self.shutdown()
>> +            sys.stderr.write('Error launching VM.\n%s%s' %
>> +                             ('Command:\n%s\n' %
>> +                              ' '.join(args) if args else '',
>> +                              'Output:\n%s\n' %
>> +                              ''.join(self._iolog) if self._iolog else ''))
>
> I suggest making this conditional on self._debug.

Ok.

>
> We could also introduce a QEMULaunchError exception class
> containing this info, but I'm unsure if we should raise it on
> every exception, or raise more specific custom exceptions on
> specific cases (e.g. one exception for QMP timeout, another for
> QEMU binary not found, another for QEMU killed by a signal, etc).

I'd prefer to keep the original exceptions and make this a debug message.

>
>
>>              raise
>>
>> -    def _launch(self):
>> +    def _launch(self, args):
>>          '''Launch the VM and establish a QMP connection.'''
>>          devnull = open('/dev/null', 'rb')
>>          qemulog = open(self._qemu_log_path, 'wb')
>>          self._pre_launch()
>> -        args = self._wrapper + [self._binary] + self._base_args() + self._args
>>          self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
>>                                         stderr=subprocess.STDOUT, shell=False)
>>          self._post_launch()
>> --
>> 2.13.3
>>
>
> --
> Eduardo