[node-patches] Change in ovirt-node[master]: process: pipe() can raise exception

[fabiand at fedoraproject.org]

Fabian Deutsch has uploaded a new change for review.

Change subject: process: pipe() can raise exception
......................................................................

process: pipe() can raise exception

Previously the failed start of a service was not detected n el6, because
the fallback of check_output - pipe() - didn't throw exceptions on
failed starts. This is now fixed, pipe() is throwing (if asked)
exceptions to signla if th ecmd failed (the service didi not start).

Change-Id: I588d1b97d31553a1aaf05c0cd0462b118369179d
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=991375
Signed-off-by: Fabian Deutsch <fabiand at fedoraproject.org>
---
M src/ovirt/node/utils/process.py
1 file changed, 79 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/ovirt-node refs/changes/21/19621/1

diff --git a/src/ovirt/node/utils/process.py b/src/ovirt/node/utils/process.py
index 70e1954..56d2e53 100644
--- a/src/ovirt/node/utils/process.py
+++ b/src/ovirt/node/utils/process.py
@@ -44,9 +44,25 @@
 
 
 def __check_for_problems(args, kwargs):
+    """This checks for one well known problem.
+
+    If a string is used as the cmd, then shell=True needs to be passed
+    >>> __check_for_problems(["true"], {"shell": True})
+
+    When the cmd is a list, then shell must be False (which it is by default)
+    >>> __check_for_problems([["true"]], {"shell": False})
+    >>> __check_for_problems([["true"]], {})
+
+    If a list is used as the cmd, then shell is not allowed.
+    >>> __check_for_problems([["true"]],
+    ... {"shell": True}) #doctest: +IGNORE_EXCEPTION_DETAIL
+    Traceback (most recent call last):
+    ...
+    RuntimeError:
+    """
     if ("shell" in kwargs and kwargs["shell"] is True) and \
             (args and type(args[0]) is list):
-        raise RuntimeError("Combining  shell=True and a command list does " +
+        raise RuntimeError("Combining shell=True and a command list does " +
                            "not work. With shell=True the first argument" +
                            "must be a string. A list otherwise.")
 
@@ -62,6 +78,15 @@
 
 def call(*args, **kwargs):
     """subprocess.call wrapper to not leak file descriptors
+
+    >>> call(["true"])
+    0
+    >>> call(["false"])
+    1
+    >>> call(["echo", "42"], stdout=PIPE)
+    0
+    >>> call("echo 42", shell=True, stdout=PIPE)
+    0
     """
     kwargs = __update_kwargs(kwargs)
     LOGGER.debug("Calling with: %s %s" % (args, kwargs))
@@ -80,6 +105,15 @@
 
 def check_output(*args, **kwargs):
     """subprocess.check_output wrapper to not leak file descriptors
+
+    >>> check_output(["echo", "-n", "42"])
+    u'42'
+    >>> check_output("echo -n 42", shell=True)
+    u'42'
+    >>> check_output("false", shell=True)
+    Traceback (most recent call last):
+    ...
+    CalledProcessError: Command 'false' returned non-zero exit status 1
     """
     kwargs = __update_kwargs(kwargs)
     LOGGER.debug("Checking output with: %s %s" % (args, kwargs))
@@ -88,25 +122,63 @@
         return unicode(subprocess.check_output(*args, **kwargs),
                        encoding=sys.stdin.encoding or "utf-8")
     except AttributeError:
-        # We're probably on Python 2.7, which doesn't have check_output
+        # We're probably on Python 2.6, which doesn't have check_output
         # http://docs.python.org/2.6/library/subprocess.html#module-subprocess
-        # Working around by using pipe, which doesn't check, but returns the
-        # output
-        return pipe(*args, **kwargs)
+        # Working around by using pipe with it's check feature
+        return pipe(*args, check=True, **kwargs)
 
 
-def pipe(cmd, stdin=None, **kwargs):
+def pipe(cmd, stdin=None, check=False, **kwargs):
     """Run a non-interactive command and return it's output
 
     Args:
         cmd: Cmdline to be run
         stdin: (optional) Data passed to stdin
+        check: Raise an CalledProcessException if the cmd fails
 
     Returns:
         stdout, stderr of the process (as one blob)
+
+    >>> pipe("echo 1")
+    u'1\\n'
+    >>> pipe("false ; echo -n 42")
+    u'42'
+    >>> pipe("true")
+    u''
+    >>> pipe("false")
+    u''
+
+    When asked, pipe() can also throw Exceptions
+    >>> pipe("false", check=True)
+    Traceback (most recent call last):
+    ...
+    CalledProcessError: Command 'false' returned non-zero exit status 1
+
+    This is how pipe is used in check_output as a fallback, so let's check
+    it here too:
+    >>> args = ["false"]
+    >>> kwargs = {}
+    >>> pipe(*args, check=True, **kwargs)
+    Traceback (most recent call last):
+    ...
+    CalledProcessError: Command 'false' returned non-zero exit status 1
     """
     kwargs.update({"stdin": PIPE,
                    "stdout": PIPE,
                    "stderr": STDOUT})
+    if type(cmd) in [str, unicode]:
+        kwargs["shell"] = True
     __check_for_problems(cmd, kwargs)
-    return unicode(popen(cmd, **kwargs).communicate(stdin)[0])
+    proc = popen(cmd, **kwargs)
+    stdout, stderr = proc.communicate(stdin)
+
+    #
+    # We need to handle the checking ourselfs, mainly for el6 comapatability
+    # as a fallback for check_output
+    #
+    if check and proc.returncode != 0:
+        err = CalledProcessError(proc.returncode, cmd)
+        err.output = stderr
+        raise err
+
+    return unicode(stdout)


-- 
To view, visit http://gerrit.ovirt.org/19621
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I588d1b97d31553a1aaf05c0cd0462b118369179d
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Fabian Deutsch <fabiand at fedoraproject.org>