On Fri, Aug 18, 2017 at 3:57 PM, Markus Armbruster <arm...@redhat.com> wrote:
> 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?
The "self._qemu_full_args = None" is the cleanup. Case the next
instruction fails, exception will have "None" instead of arguments
from a previous launch().
>
>>>> + 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.
