Re: [Qemu-devel] [PATCH v5 6/6] qemu.py: include qemu command line and output on launch error

[Amador Pahim]

On Tue, Jul 25, 2017 at 11:17 PM, Eduardo Habkost <address@hidden> 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 <address@hidden>
>> ---
>>  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