[PATCHv2] Do not mix stdout and stderr when running utilities unless requested

Vratislav Podzimek vpodzime at redhat.com
Fri Jan 23 08:54:52 UTC 2015


On Thu, 2015-01-22 at 11:48 -0500, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: anaconda-patches at 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 at 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



More information about the anaconda-patches mailing list