Amador Pahim <apa...@redhat.com> writes:
> On Tue, Aug 15, 2017 at 10:26 AM, Markus Armbruster <arm...@redhat.com> wrote:
>> Amador Pahim <apa...@redhat.com> 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 <apa...@redhat.com>
>>> ---
>>> 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.
Ignorant question: why aren't we cleaning it up?
>>> + 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.
