fix: pass stdin=DEVNULL to Popen to avoid eating stdin from pipes

[xiangce]

• For the first cmd to run in subprocess.call, specified
  "stdin=subprocess.DEVNULL" to avoid it from eating
  the stdin behind it
• fix: Enhance simple_command to close stdin to avoid this class of bugs in the future. #4187

All Pull Requests:

Check all that apply:

• Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• Is this PR to correct an issue?
• Is this PR an enhancement?

Complete Description of Additions/Changes:

Add your description here

[xiangce]

Hi @bfahr, @ryan-blakley, @JoySnow, @ekohl, @ShimShtein - this is a basic change to the util.subproc of the core, would you please help review it whenever free, Thanks in advance.

[ekohl]

The patch is harder to read because of the reformatting changes. I always like to separate those out to separate commits, making it obvious for reviewers what's going on. So the reduced patch is:

---
 insights/util/subproc.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/insights/util/subproc.py b/insights/util/subproc.py
index 547ff0e5ca2a..d3cd46c4c25c 100644
--- a/insights/util/subproc.py
+++ b/insights/util/subproc.py
@@ -10,6 +10,11 @@ from subprocess import Popen, PIPE, STDOUT
 from insights.core.exceptions import CalledProcessError
 from insights.util import which
 
+try:
+    from subprocess import DEVNULL
+except:
+    DEVNULL = open(os.devnull, 'w')
+
 log = logging.getLogger(__name__)
 
 
@@ -57,9 +62,9 @@ class Pipeline(object):
     def _build_pipes(self, out_stream=PIPE):
         log.debug("Executing: %s" % str(self.cmds))
         if len(self.cmds) == 1:
-            return Popen(self.cmds[0], bufsize=self.bufsize, stderr=STDOUT, stdout=out_stream, env=self.env)
+            return Popen(self.cmds[0], bufsize=self.bufsize, stdin=DEVNULL, stderr=STDOUT, stdout=out_stream, env=self.env)
 
-        stdout = Popen(self.cmds[0], bufsize=self.bufsize, stderr=STDOUT, stdout=PIPE, env=self.env).stdout
+        stdout = Popen(self.cmds[0], bufsize=self.bufsize, stdin=DEVNULL, stderr=STDOUT, stdout=PIPE, env=self.env).stdout
         last = len(self.cmds) - 2
         for i, arg in enumerate(self.cmds[1:]):
             if i < last:

That looks ok.

However, I think the function is way too complex and I think this is the most simple version:

    def _build_pipes(self, out_stream=PIPE):
        if len(self.cmds) == 0:
            return

        log.debug("Executing: %s", self.cmds)
        stdout = DEVNULL
        for arg in self.cmds[:-1]:
            stdout = Popen(arg, bufsize=self.bufsize, stdin=stdout, stderr=STDOUT, stdout=PIPE, env=self.env).stdout
        return Popen(self.cmds[-1], bufsize=self.bufsize, stdin=stdout, stderr=STDOUT, stdout=out_stream, env=self.env)

Or if you really want to make it obvious what the differing args are:

    def _build_pipes(self, out_stream=PIPE):
        if len(self.cmds) == 0:
            return

        log.debug("Executing: %s", self.cmds)
        stdout = DEVNULL
        kwargs = {
            'bufsize': self.bufsize,
            'stderr': STDOUT,
            'env': self.env
        }
        for arg in self.cmds[:-1]:
            stdout = Popen(arg, stdin=stdout, stdout=PIPE, **kwargs).stdout
        return Popen(self.cmds[-1], stdin=stdout, stdout=out_stream, **kwargs)

I'm not a maintainer on the project, so take it as you prefer. Your fix is correct and resolves the original problem.

[ekohl]

Completely unrelated, but I just noticed this while reading the surrounding code. Not for this PR, but a general question.

Why isn't this using the interpolation that logging already has built in? That way you don't pay the str() and interpolation overhead if the logging level is above debug.

Suggested change

	log.debug("Executing: %s" % str(self.cmds))
	log.debug("Executing: %s", self.cmds)

[xiangce]

Based on your suggestion, I've un-done the previous line breaks in the new commit to keep the changes minimal for this update. Thanks again @ekohl

[ShimShtein]

Didn't test it, but looks good for me too.

[ekohl]

        if len(self.cmds) == 0:

And this can even be simplified further:

if not self.cmds:

[xiangce]

@ekohl and @ShimShtein - Thanks for the review. Regarding the other comments, I'd like to keep them as they were in this change. And we can update them later if necessary.