Re: [PATCH 1/4] ExecCmd: include stdout data in ExecCmdFail if command failed
Mon, Mar 21, 2016 at 01:27:38PM CET, jtluka(a)redhat.com wrote:
Mon, Mar 21, 2016 at 11:55:02AM CET, olichtne(a)redhat.com wrote:
>On Wed, Mar 16, 2016 at 10:29:13AM +0100, Jan Tluka wrote:
>> ExecCmdFail exception is thrown if a command returns with non-zero
>> returncode. When this happens only stderr is saved in the exception.
>> User might want to work with both the stdout and stderr.
>>
>> This patch saves stdout data along with stderr in this case.
>>
>> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
>> ---
>> lnst/Common/ExecCmd.py | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/lnst/Common/ExecCmd.py b/lnst/Common/ExecCmd.py
>> index d6c92c6..d432c2c 100644
>> --- a/lnst/Common/ExecCmd.py
>> +++ b/lnst/Common/ExecCmd.py
>> @@ -17,10 +17,12 @@ class ExecCmdFail(Exception):
>> _cmd = None
>> _retval = None
>> _stderr = None
>> + _stdout = None
>> _report_stderr = None
>>
>> - def __init__(self, cmd=None, retval=None, err="",
report_stderr=False):
>> - self._stderr = err
>> + def __init__(self, cmd=None, retval=None, outs=["", ""],
report_stderr=False):
>> + self._stdout = outs[0]
>> + self._stderr = outs[1]
>> self._retval = retval
>> self._report_stderr = report_stderr
>>
>> @@ -30,6 +32,9 @@ class ExecCmdFail(Exception):
>> def get_stderr(self):
>> return self._stderr
>>
>> + def get_stdout(self):
>> + return self._stdout
>> +
>> def __str__(self):
>> retval = ""
>> stderr = ""
>
>Could we maybe include the stdout in the __str__ method of the
>exception? I'm not sure if it's used anywhere, but it seems like a
>reasonable idea if we have it available...
Yes and no.
When a command fails it usually writes something really short on its
stderr. Currently LNST uses ExecCmdFail.__str__() when reporting it to
the log when the command fails. If we include stdout this will very
likely break the log output formatting, since stdout could be much
longer.
On the other hand it could be handy in some cases but it will need
slightly bigger changes.
While looking at the code it'd probably doable, since by default
report_stderr is disabled, so I could do it the same way and it won't
break anything atm.
I will send it again.
Taking it back. I don't see any value here. I had a look at the code and
report_stderr is not used at all. It's always set to default or False.
-Jan