On Thu, 2014-10-30 at 09:51 -0400, Anne Mulhern wrote:
>
----- Original Message -----
>> From: "Vratislav Podzimek" <vpodzime(a)redhat.com
>> To: anaconda-patches(a)lists.fedorahosted.org
>> Sent: Thursday, October 30, 2014 8:01:57 AM
>> Subject: [blivet][master] Do not mix stdout and stderr when running utilities
>
>> We only gather data from stdout and stderr contents
may confuse our parsing
>> code. But we may still be interested in the stderr so log it.
>
>> Signed-off-by: Vratislav Podzimek
<vpodzime(a)redhat.com
>> ---
>> blivet/util.py | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>> diff --git a/blivet/util.py b/blivet/util.py
>> index a69b694..01a4548 100644
>> --- a/blivet/util.py
>> +++ b/blivet/util.py
>> @@ -44,15 +44,21 @@ def _run_program(argv, root='/', stdin=None,
>> env_prune=None):
>> proc = subprocess.Popen(argv,
>> stdin=stdin,
>> stdout=subprocess.PIPE,
>> - stderr=subprocess.STDOUT,
>> + stderr=subprocess.PIPE,
>> close_fds=True,
>> preexec_fn=chroot, cwd=root, env=env)
>
>> - out = proc.communicate()[0]
>> + out, err = proc.communicate()
>> if out:
>> + program_log.info("stdout:")
>> for line in out.splitlines():
>> program_log.info("%s", line)
>
>> + if err:
>> + program_log.info("stderr:")
>> + for line in err.splitlines():
>> + program_log.info("%s", line)
>> +
>> except OSError as e:
>> program_log.error("Error running %s: %s", argv[0],
e.strerror)
>> raise
>> --
>> 1.9.3
>
>> _______________________________________________
>> anaconda-patches mailing list
>> anaconda-patches(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
> I do not like conflating
standard out and standard error.
> So, generally speaking, I'm in favor of this change.
> I also do not see any methods in devicelibs that seem to
expect to read
> stderr from stdout. This doesn't mean there are none, but it's heartening.
> I looked around outside devicelibs and found a number of
calls to
> util.*capture_output* methods that were worth investigating.
> However, only fcoe.fcoe.addSan() seems to me to contain
> some calls which seem to expect that
> out will contain stderr.
> This patch isn't really complete until at the least
those calls have
> been investigated and appropriate action taken, however.
Okay so the addSan method returns an error message in case of failure
which is constructed from the mangled stdout+stderr of 'fipvlan'
invocation. Such message is then prepended with the string "Activating
FCoE SAN failed:" and shown to the user by Anaconda. I can confirm that
the 'fipvlan' utility really uses its stderr for error messages.
So we can:
1) drop this proposed patch and pretend that it is okay to mangle stdout
and stderr
2) apply it as it is and live with the fact that users won't see the
fipvlan's stderr output (we will, in the program.log)
3) add a 'err_to_out' parameter to run_program used only by addSan
Any suggestions? I think 3) is easy enough to be the best choice, but
YMMV.
Something along the lines of #3 sounds best, #2 being next best. Another
option would be to add a third item to the return value of _run_program
that contains the stderr output and another wrapper function called
capture_output_with_errors or similar.
David