On Thu, 2015-01-22 at 11:48 -0500, Anne Mulhern wrote:
----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
> To: anaconda-patches(a)lists.fedorahosted.org
> Sent: Thursday, January 22, 2015 6:00:01 AM
> Subject: [PATCHv2] Do not mix stdout and stderr when running utilities
unless requested
>
> We only gather data from stdout and stderr contents may confuse our parsing
> code. However, we may still be interested in the stderr so log it and allow
> caller code to specify that stderr should go to stdout.
>
> Signed-off-by: Vratislav Podzimek <vpodzime(a)redhat.com>
> ---
> blivet/fcoe.py | 4 ++--
> blivet/util.py | 17 ++++++++++++++---
> 2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/blivet/fcoe.py b/blivet/fcoe.py
> index 707ec50..81c5c97 100644
> --- a/blivet/fcoe.py
> +++ b/blivet/fcoe.py
> @@ -119,13 +119,13 @@ class fcoe(object):
> util.run_program(["dcbtool", "sc", nic,
"app:fcoe",
> "e:1", "a:1",
"w:1"])
> rc, out = util.run_program_and_capture_output(["fipvlan",
"-c",
> "-s", "-f",
> - "-fcoe", nic])
> + "-fcoe", nic],
> stderr_to_stdout=True)
> else:
> if auto_vlan:
> # certain network configrations require the VLAN layer
> module:
> util.run_program(["modprobe", "8021q"])
> rc, out =
util.run_program_and_capture_output(["fipvlan",
> '-c', '-s', '-f',
> - "-fcoe", nic])
> + "-fcoe", nic],
> stderr_to_stdout=True)
> else:
> f = open("/sys/module/libfcoe/parameters/create",
"w")
> f.write(nic)
> diff --git a/blivet/util.py b/blivet/util.py
> index e3670a9..3e60e0b 100644
> --- a/blivet/util.py
> +++ b/blivet/util.py
> @@ -23,7 +23,7 @@ from threading import Lock
> program_log_lock = Lock()
>
>
> -def _run_program(argv, root='/', stdin=None, env_prune=None):
> +def _run_program(argv, root='/', stdin=None, env_prune=None,
> stderr_to_stdout=False):
> if env_prune is None:
> env_prune = []
>
> @@ -40,19 +40,30 @@ def _run_program(argv, root='/', stdin=None,
> env_prune=None):
> for var in env_prune:
> env.pop(var, None)
>
> + if stderr_to_stdout:
> + stderr_dir = subprocess.STDOUT
> + else:
> + stderr_dir = subprocess.PIPE
> try:
> proc = subprocess.Popen(argv,
> stdin=stdin,
> stdout=subprocess.PIPE,
> - stderr=subprocess.STDOUT,
> + stderr=stderr_dir,
> close_fds=True,
> preexec_fn=chroot, cwd=root, env=env)
>
> - out = proc.communicate()[0]
> + out, err = proc.communicate()
> if out:
> + if not stderr_to_stdout:
> + program_log.info("stdout:")
> for line in out.splitlines():
> program_log.info("%s", line)
>
> + if not stderr_to_stdout and err:
Probably better to just have:
if err:
If subprocess.Popen is working, then err implies not stderr_to_stdout.
And if it isn't, we might as well still no what was printed to stderr.
If
subprocess.Popen doesn't work as expected, we are doomed for many
other reasons than this particular one so let's assume it works. I still
like the check of the flag being done in a short-circuit way first. It
just naturally fits in there first and if I were to make up some other
reason, let's say that checking a bool flag is cheaper than checking the
boolean representation of a string.
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic