subp: make ProcessExecutionError have expected types in stderr, stdout.

When subp raised a ProcessExecutionError, that exception's stderr and
stdout might end up being the string '-' rather than bytes.

This mean that:
   try:
       subp(mycommand, decode=False)
   except ProcessExecutionError as e:
       pass

Would have 'e.stdout' set to '-' while the caller would expect bytes.

Also reduce the try/except block in subp to a specifically the two lines
that may raise an OSError.

2 files changed, 66 insertions, 43 deletions

diff --git a/cloudinit/util.py b/cloudinit/util.py
index e42498d9..df0aa5db 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -253,12 +253,18 @@ class ProcessExecutionError(IOError):
             self.exit_code = exit_code
 
         if not stderr:
-            self.stderr = self.empty_attr
+            if stderr is None:
+                self.stderr = self.empty_attr
+            else:
+                self.stderr = stderr
         else:
             self.stderr = self._indent_text(stderr)
 
         if not stdout:
-            self.stdout = self.empty_attr
+            if stdout is None:
+                self.stdout = self.empty_attr
+            else:
+                self.stdout = stdout
         else:
             self.stdout = self._indent_text(stdout)
 
@@ -1829,58 +1835,60 @@ def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
         env = env.copy()
         env.update(update_env)
 
-    try:
-        if target_path(target) != "/":
-            args = ['chroot', target] + list(args)
+    if target_path(target) != "/":
+        args = ['chroot', target] + list(args)
 
-        if not logstring:
-            LOG.debug(("Running command %s with allowed return codes %s"
-                       " (shell=%s, capture=%s)"), args, rcs, shell, capture)
-        else:
-            LOG.debug(("Running hidden command to protect sensitive "
-                       "input/output logstring: %s"), logstring)
-
-        stdin = None
-        stdout = None
-        stderr = None
-        if capture:
-            stdout = subprocess.PIPE
-            stderr = subprocess.PIPE
-        if data is None:
-            # using devnull assures any reads get null, rather
-            # than possibly waiting on input.
-            devnull_fp = open(os.devnull)
-            stdin = devnull_fp
-        else:
-            stdin = subprocess.PIPE
-            if not isinstance(data, bytes):
-                data = data.encode()
+    if not logstring:
+        LOG.debug(("Running command %s with allowed return codes %s"
+                   " (shell=%s, capture=%s)"), args, rcs, shell, capture)
+    else:
+        LOG.debug(("Running hidden command to protect sensitive "
+                   "input/output logstring: %s"), logstring)
+
+    stdin = None
+    stdout = None
+    stderr = None
+    if capture:
+        stdout = subprocess.PIPE
+        stderr = subprocess.PIPE
+    if data is None:
+        # using devnull assures any reads get null, rather
+        # than possibly waiting on input.
+        devnull_fp = open(os.devnull)
+        stdin = devnull_fp
+    else:
+        stdin = subprocess.PIPE
+        if not isinstance(data, bytes):
+            data = data.encode()
 
+    try:
         sp = subprocess.Popen(args, stdout=stdout,
                               stderr=stderr, stdin=stdin,
                               env=env, shell=shell)
         (out, err) = sp.communicate(data)
-
-        # Just ensure blank instead of none.
-        if not out and capture:
-            out = b''
-        if not err and capture:
-            err = b''
-        if decode:
-            def ldecode(data, m='utf-8'):
-                if not isinstance(data, bytes):
-                    return data
-                return data.decode(m, decode)
-
-            out = ldecode(out)
-            err = ldecode(err)
     except OSError as e:
-        raise ProcessExecutionError(cmd=args, reason=e,
-                                    errno=e.errno)
+        raise ProcessExecutionError(
+            cmd=args, reason=e, errno=e.errno,
+            stdout="-" if decode else b"-",
+            stderr="-" if decode else b"-")
     finally:
         if devnull_fp:
             devnull_fp.close()
 
+    # Just ensure blank instead of none.
+    if not out and capture:
+        out = b''
+    if not err and capture:
+        err = b''
+    if decode:
+        def ldecode(data, m='utf-8'):
+            if not isinstance(data, bytes):
+                return data
+            return data.decode(m, decode)
+
+        out = ldecode(out)
+        err = ldecode(err)
+
     rc = sp.returncode
     if rc not in rcs:
         raise ProcessExecutionError(stdout=out, stderr=err,
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index d63b760e..4a92e741 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -623,6 +623,7 @@ class TestSubp(helpers.CiTestCase):
     utf8_valid = b'start \xc3\xa9 end'
     utf8_valid_2 = b'd\xc3\xa9j\xc8\xa7'
     printenv = [BASH, '-c', 'for n in "$@"; do echo "$n=${!n}"; done', '--']
+    bogus_command = 'this-is-not-expected-to-be-a-program-name'
 
     def printf_cmd(self, *args):
         # bash's printf supports \xaa.  So does /usr/bin/printf
@@ -712,6 +713,20 @@ class TestSubp(helpers.CiTestCase):
         self.assertIsNone(err)
         self.assertIsNone(out)
 
+    def test_exception_has_out_err_are_bytes_if_decode_false(self):
+        """Raised exc should have stderr, stdout as bytes if no decode."""
+        with self.assertRaises(util.ProcessExecutionError) as cm:
+            util.subp([self.bogus_command], decode=False)
+        self.assertTrue(isinstance(cm.exception.stdout, bytes))
+        self.assertTrue(isinstance(cm.exception.stderr, bytes))
+
+    def test_exception_has_out_err_are_bytes_if_decode_true(self):
+        """Raised exc should have stderr, stdout as string if no decode."""
+        with self.assertRaises(util.ProcessExecutionError) as cm:
+            util.subp([self.bogus_command], decode=True)
+        self.assertTrue(isinstance(cm.exception.stdout, six.string_types))
+        self.assertTrue(isinstance(cm.exception.stderr, six.string_types))
+
     def test_bunch_of_slashes_in_path(self):
         self.assertEqual("/target/my/path/",
                          util.target_path("/target/", "//my/path/"))