Re: [Qemu-devel] [PATCH v6 4/7] qemu.py: improve message on negative exit code

[Amador Pahim]

On Tue, Aug 15, 2017 at 10:26 AM, Markus Armbruster <address@hidden> wrote:
> Amador Pahim <address@hidden> writes:
>
>> The current message shows 'self._args', which contains only part of the
>> options used in the qemu command line.
>>
>> This patch makes the qemu full args list an instance variable and then
>> uses it in the negative exit code message.
>>
>> Signed-off-by: Amador Pahim <address@hidden>
>> ---
>>  scripts/qemu.py | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index e3ea534ec4..9434ccc30b 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -48,6 +48,7 @@ class QEMUMachine(object):
>>          self._iolog = None
>>          self._socket_scm_helper = socket_scm_helper
>>          self._debug = debug
>> +        self._qemu_full_args = None
>>
>>      # This can be used to add an unused monitor instance.
>>      def add_monitor_telnet(self, ip, port):
>> @@ -140,9 +141,14 @@ class QEMUMachine(object):
>>          qemulog = open(self._qemu_log_path, 'wb')
>>          try:
>>              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._qemu_full_args = None
>> +            self._qemu_full_args = (self._wrapper + [self._binary] +
>> +                                    self._base_args() + self._args)
>
> Why set self._qemu_full_args twice?

If it's not cleaned up and an exception happens in
"self._qemu_full_args = (self._wrapper ...", the message logged in the
"except" will expose an outdated version of it.

>
>> +            self._popen = subprocess.Popen(self._qemu_full_args,
>> +                                           stdin=devnull,
>> +                                           stdout=qemulog,
>> +                                           stderr=subprocess.STDOUT,
>> +                                           shell=False)
>>              self._post_launch()
>>          except:
>>              if self.is_running():
>> @@ -163,8 +169,9 @@ class QEMUMachine(object):
>>
>>              exitcode = self._popen.wait()
>>              if exitcode < 0:
>> -                LOG.error('qemu received signal %i: %s', -exitcode,
>> -                          ' '.join(self._args))
>> +                LOG.error('qemu received signal %i:%s', -exitcode,
>> +                          ' Command: %r.' % ' '.join(self._qemu_full_args)
>> +                          if self._qemu_full_args else '')
>
> The last argument is hard to read.

Ok, I'm making it easier.

>
>>              self._load_io_log()
>>              self._post_shutdown()
>
> The subprocess module appears to keep track of the full command with
> methods check_call() and check_output(): its in exception
> CalledProcessError right when you need it.  Sadly, it doesn't appear to
> be tracked with method Popen().

Indeed.