better Executor class for client tools

19 files changed, 364 insertions, 390 deletions

diff --git a/src/lib/Bcfg2/Client/Tools/APK.py b/src/lib/Bcfg2/Client/Tools/APK.py
index f23fbb119..8a02b7d6d 100644
--- a/src/lib/Bcfg2/Client/Tools/APK.py
+++ b/src/lib/Bcfg2/Client/Tools/APK.py
@@ -19,8 +19,8 @@ class APK(Bcfg2.Client.Tools.PkgTool):
 
     def RefreshPackages(self):
         """Refresh memory hashes of packages."""
-        names = self.cmd.run("/sbin/apk info")[1]
-        nameversions = self.cmd.run("/sbin/apk info -v")[1]
+        names = self.cmd.run("/sbin/apk info").stdout.splitlines()
+        nameversions = self.cmd.run("/sbin/apk info -v").stdout.splitlines()
         for pkg in zip(names, nameversions):
             pkgname = pkg[0]
             version = pkg[1][len(pkgname) + 1:]
@@ -56,7 +56,6 @@ class APK(Bcfg2.Client.Tools.PkgTool):
         """Remove extra packages."""
         names = [pkg.get('name') for pkg in packages]
         self.logger.info("Removing packages: %s" % " ".join(names))
-        self.cmd.run("/sbin/apk del %s" % \
-                     " ".join(names))
+        self.cmd.run("/sbin/apk del %s" % " ".join(names))
         self.RefreshPackages()
         self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/APT.py b/src/lib/Bcfg2/Client/Tools/APT.py
index 879d2720a..0cdefa613 100644
--- a/src/lib/Bcfg2/Client/Tools/APT.py
+++ b/src/lib/Bcfg2/Client/Tools/APT.py
@@ -59,7 +59,8 @@ class APT(Bcfg2.Client.Tools.Tool):
         os.environ["DEBIAN_FRONTEND"] = 'noninteractive'
         self.actions = {}
         if self.setup['kevlar'] and not self.setup['dryrun']:
-            self.cmd.run("%s --force-confold --configure --pending" % self.dpkg)
+            self.cmd.run("%s --force-confold --configure --pending" %
+                         self.dpkg)
             self.cmd.run("%s clean" % self.aptget)
             try:
                 self.pkg_cache = apt.cache.Cache()
@@ -88,13 +89,15 @@ class APT(Bcfg2.Client.Tools.Tool):
                                          for (name, version) in extras]
 
     def VerifyDebsums(self, entry, modlist):
-        output = self.cmd.run("%s -as %s" % (self.debsums,
-                                             entry.get('name')))[1]
+        output = \
+            self.cmd.run("%s -as %s" %
+                         (self.debsums, entry.get('name'))).stdout.splitlines()
         if len(output) == 1 and "no md5sums for" in output[0]:
             self.logger.info("Package %s has no md5sums. Cannot verify" % \
                              entry.get('name'))
-            entry.set('qtext', "Reinstall Package %s-%s to setup md5sums? (y/N) " \
-                      % (entry.get('name'), entry.get('version')))
+            entry.set('qtext',
+                      "Reinstall Package %s-%s to setup md5sums? (y/N) " %
+                      (entry.get('name'), entry.get('version')))
             return False
         files = []
         for item in output:
@@ -250,8 +253,7 @@ class APT(Bcfg2.Client.Tools.Tool):
             self.logger.error(bad_pkgs)
         if not ipkgs:
             return
-        rc = self.cmd.run(self.pkgcmd % (" ".join(ipkgs)))[0]
-        if rc:
+        if not self.cmd.run(self.pkgcmd % (" ".join(ipkgs))):
             self.logger.error("APT command failed")
         self.pkg_cache = apt.cache.Cache()
         self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/Action.py b/src/lib/Bcfg2/Client/Tools/Action.py
index d5caf3231..7e8366928 100644
--- a/src/lib/Bcfg2/Client/Tools/Action.py
+++ b/src/lib/Bcfg2/Client/Tools/Action.py
@@ -48,14 +48,11 @@ class Action(Bcfg2.Client.Tools.Tool):
                                       "to build mode" % entry.get('command'))
                     return False
             self.logger.debug("Running Action %s" % (entry.get('name')))
-            rv = self.cmd.run(entry.get('command'))[0]
+            rv = self.cmd.run(entry.get('command'))
             self.logger.debug("Action: %s got return code %s" %
-                              (entry.get('command'), rv))
-            entry.set('rc', str(rv))
-            if entry.get('status', 'check') == 'ignore':
-                return True
-            else:
-                return rv == 0
+                              (entry.get('command'), rv.retval))
+            entry.set('rc', str(rv.retval))
+            return entry.get('status', 'check') == 'ignore' or rv.success
         else:
             self.logger.debug("In dryrun mode: not running action: %s" %
                               (entry.get('name')))
diff --git a/src/lib/Bcfg2/Client/Tools/Chkconfig.py b/src/lib/Bcfg2/Client/Tools/Chkconfig.py
index e1ad35861..ec7f462b3 100644
--- a/src/lib/Bcfg2/Client/Tools/Chkconfig.py
+++ b/src/lib/Bcfg2/Client/Tools/Chkconfig.py
@@ -24,16 +24,14 @@ class Chkconfig(Bcfg2.Client.Tools.SvcTool):
         if entry.get('status') == 'ignore':
             return True
 
-        try:
-            cmd = "/sbin/chkconfig --list %s " % (entry.get('name'))
-            raw = self.cmd.run(cmd)[1]
-            patterns = ["error reading information", "unknown service"]
-            srvdata = [line.split() for line in raw for pattern in patterns \
-                       if pattern not in line][0]
-        except IndexError:
-            # Ocurrs when no lines are returned (service not installed)
+        rv = self.cmd.run("/sbin/chkconfig --list %s " % entry.get('name'))
+        if rv.success:
+            srvdata = rv.stdout.splitlines()[0].split()
+        else:
+            # service not installed
             entry.set('current_status', 'off')
             return False
+
         if len(srvdata) == 2:
             # This is an xinetd service
             if entry.get('status') == srvdata[1]:
@@ -43,7 +41,7 @@ class Chkconfig(Bcfg2.Client.Tools.SvcTool):
                 return False
 
         try:
-            onlevels = [level.split(':')[0] for level in srvdata[1:] \
+            onlevels = [level.split(':')[0] for level in srvdata[1:]
                         if level.split(':')[1] == 'on']
         except IndexError:
             onlevels = []
@@ -70,25 +68,25 @@ class Chkconfig(Bcfg2.Client.Tools.SvcTool):
         if entry.get('status') == 'off':
             rv &= self.cmd.run((rcmd + " --level 0123456") %
                                (entry.get('name'),
-                                entry.get('status')))[0] == 0
+                                entry.get('status'))).success
             if entry.get("current_status") == "on":
-                rv &= self.stop_service(entry)
+                rv &= self.stop_service(entry).success
         else:
             rv &= self.cmd.run(rcmd % (entry.get('name'),
-                                       entry.get('status')))[0] == 0
+                                       entry.get('status'))).success
             if entry.get("current_status") == "off":
-                rv &= (self.start_service(entry) == 0)
+                rv &= self.start_service(entry).success
         return rv
 
     def FindExtra(self):
         """Locate extra chkconfig Services."""
         allsrv = [line.split()[0]
-                  for line in self.cmd.run("/sbin/chkconfig "
-                                           "--list 2>/dev/null|grep :on")[1]]
+                  for line in self.cmd.run("/sbin/chkconfig",
+                                           "--list").stdout.splitlines()
+                  if ":on" in line]
         self.logger.debug('Found active services:')
         self.logger.debug(allsrv)
         specified = [srv.get('name') for srv in self.getSupportedEntries()]
-        return [Bcfg2.Client.XML.Element('Service',
-                                         type='chkconfig',
-                                         name=name) \
+        return [Bcfg2.Client.XML.Element('Service', type='chkconfig',
+                                         name=name)
                 for name in allsrv if name not in specified]
diff --git a/src/lib/Bcfg2/Client/Tools/DebInit.py b/src/lib/Bcfg2/Client/Tools/DebInit.py
index 7d5af1127..ca556e98b 100644
--- a/src/lib/Bcfg2/Client/Tools/DebInit.py
+++ b/src/lib/Bcfg2/Client/Tools/DebInit.py
@@ -15,7 +15,8 @@ class DebInit(Bcfg2.Client.Tools.SvcTool):
     __execs__ = ['/usr/sbin/update-rc.d', '/usr/sbin/invoke-rc.d']
     __handles__ = [('Service', 'deb')]
     __req__ = {'Service': ['name', 'status']}
-    svcre = re.compile("/etc/.*/(?P<action>[SK])(?P<sequence>\d+)(?P<name>\S+)")
+    svcre = \
+        re.compile("/etc/.*/(?P<action>[SK])(?P<sequence>\d+)(?P<name>\S+)")
 
     # implement entry (Verify|Install) ops
     def VerifyService(self, entry, _):
@@ -28,7 +29,7 @@ class DebInit(Bcfg2.Client.Tools.SvcTool):
         files = []
 
         try:
-            deb_version = open('/etc/debian_version', 'r').read().split('/', 1)[0]
+            deb_version = open('/etc/debian_version').read().split('/', 1)[0]
         except IOError:
             deb_version = 'unknown'
 
@@ -59,20 +60,20 @@ class DebInit(Bcfg2.Client.Tools.SvcTool):
                 return False
             else:
                 return True
+        elif files:
+            if start_sequence:
+                for filename in files:
+                    match = self.svcre.match(filename)
+                    file_sequence = int(match.group('sequence'))
+                    if ((match.group('action') == 'S' and
+                         file_sequence != start_sequence) or
+                        (match.group('action') == 'K' and
+                         file_sequence != kill_sequence)):
+                        return False
+            return True
         else:
-            if files:
-                if start_sequence:
-                    for filename in files:
-                        match = self.svcre.match(filename)
-                        file_sequence = int(match.group('sequence'))
-                        if match.group('action') == 'S' and file_sequence != start_sequence:
-                            return False
-                        if match.group('action') == 'K' and file_sequence != kill_sequence:
-                            return False
-                return True
-            else:
-                entry.set('current_status', 'off')
-                return False
+            entry.set('current_status', 'off')
+            return False
 
     def InstallService(self, entry):
         """Install Service for entry."""
@@ -80,35 +81,35 @@ class DebInit(Bcfg2.Client.Tools.SvcTool):
         try:
             os.stat('/etc/init.d/%s' % entry.get('name'))
         except OSError:
-            self.logger.debug("Init script for service %s does not exist" % entry.get('name'))
+            self.logger.debug("Init script for service %s does not exist" %
+                              entry.get('name'))
             return False
 
         if entry.get('status') == 'off':
             self.cmd.run("/usr/sbin/invoke-rc.d %s stop" % (entry.get('name')))
-            cmdrc = self.cmd.run("/usr/sbin/update-rc.d -f %s remove" % entry.get('name'))[0]
+            return self.cmd.run("/usr/sbin/update-rc.d -f %s remove" %
+                                entry.get('name')).success
         else:
             command = "/usr/sbin/update-rc.d %s defaults" % (entry.get('name'))
             if entry.get('sequence'):
-                cmdrc = self.cmd.run("/usr/sbin/update-rc.d -f %s remove" % entry.get('name'))[0]
-                if cmdrc != 0:
+                if not self.cmd.run("/usr/sbin/update-rc.d -f %s remove" %
+                                    entry.get('name')).success:
                     return False
                 start_sequence = int(entry.get('sequence'))
                 kill_sequence = 100 - start_sequence
                 command = "%s %d %d" % (command, start_sequence, kill_sequence)
-            cmdrc = self.cmd.run(command)[0]
-        return cmdrc == 0
+            return self.cmd.run(command).success
 
     def FindExtra(self):
         """Find Extra Debian Service entries."""
         specified = [entry.get('name') for entry in self.getSupportedEntries()]
-        extra = []
-        for name in [self.svcre.match(fname).group('name') for fname in
-                      glob.glob("/etc/rc[12345].d/S*") \
-                      if self.svcre.match(fname).group('name') not in specified]:
-            if name not in extra:
-                extra.append(name)
-        return [Bcfg2.Client.XML.Element('Service', name=name, type='deb') for name \
-                in extra]
+        extra = set()
+        for fname in glob.glob("/etc/rc[12345].d/S*"):
+            name = self.svcre.match(fname).group('name')
+            if name not in specified:
+                extra.add(name)
+        return [Bcfg2.Client.XML.Element('Service', name=name, type='deb')
+                for name in list(extra)]
 
     def Remove(self, _):
         """Remove extra service entries."""
diff --git a/src/lib/Bcfg2/Client/Tools/Encap.py b/src/lib/Bcfg2/Client/Tools/Encap.py
index ca6fc7653..678e0f00c 100644
--- a/src/lib/Bcfg2/Client/Tools/Encap.py
+++ b/src/lib/Bcfg2/Client/Tools/Encap.py
@@ -33,14 +33,13 @@ class Encap(Bcfg2.Client.Tools.PkgTool):
             self.logger.info("Insufficient information of Package %s; "
                              "cannot Verify" % entry.get('name'))
             return False
-        cmdrc = self.cmd.run("/usr/local/bin/epkg -q -S -k %s-%s >/dev/null" %
-                            (entry.get('name'), entry.get('version')))[0]
-        if cmdrc != 0:
+        success = self.cmd.run("/usr/local/bin/epkg -q -S -k %s-%s" %
+                               (entry.get('name'),
+                                entry.get('version'))).success
+        if not success:
             self.logger.debug("Package %s version incorrect" %
                               entry.get('name'))
-        else:
-            return True
-        return False
+        return success
 
     def Remove(self, packages):
         """Deal with extra configuration detected."""
diff --git a/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py b/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
index ded84bef4..635318805 100644
--- a/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
+++ b/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
@@ -20,7 +20,7 @@ class FreeBSDPackage(Bcfg2.Client.Tools.PkgTool):
 
     def RefreshPackages(self):
         self.installed = {}
-        packages = self.cmd.run("/usr/sbin/pkg_info -a -E")[1]
+        packages = self.cmd.run("/usr/sbin/pkg_info -a -E").stdout.splitlines()
         pattern = re.compile('(.*)-(\d.*)')
         for pkg in packages:
             if pattern.match(pkg):
diff --git a/src/lib/Bcfg2/Client/Tools/MacPorts.py b/src/lib/Bcfg2/Client/Tools/MacPorts.py
index be441135e..bc3765ec6 100644
--- a/src/lib/Bcfg2/Client/Tools/MacPorts.py
+++ b/src/lib/Bcfg2/Client/Tools/MacPorts.py
@@ -19,7 +19,8 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
 
     def RefreshPackages(self):
         """Refresh memory hashes of packages."""
-        pkgcache = self.cmd.run("/opt/local/bin/port installed")[1]
+        pkgcache = self.cmd.run(["/opt/local/bin/port",
+                                 "installed"]).stdout.splitlines()
         self.installed = {}
         for pkg in pkgcache:
             if pkg.startswith("Warning:"):
@@ -65,7 +66,7 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
         """Remove extra packages."""
         names = [pkg.get('name') for pkg in packages]
         self.logger.info("Removing packages: %s" % " ".join(names))
-        self.cmd.run("/opt/local/bin/port uninstall %s" % \
+        self.cmd.run("/opt/local/bin/port uninstall %s" %
                      " ".join(names))
         self.RefreshPackages()
         self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/POSIXUsers.py b/src/lib/Bcfg2/Client/Tools/POSIXUsers.py
index 3248cef9c..e9db33d16 100644
--- a/src/lib/Bcfg2/Client/Tools/POSIXUsers.py
+++ b/src/lib/Bcfg2/Client/Tools/POSIXUsers.py
@@ -1,60 +1,11 @@
 """ A tool to handle creating users and groups with useradd/mod/del
 and groupadd/mod/del """
 
-import sys
 import pwd
 import grp
-import subprocess
-from Bcfg2.Utils import PackedDigitRange
 import Bcfg2.Client.XML
 import Bcfg2.Client.Tools
-
-
-class ExecutionError(Exception):
-    """ Raised when running an external command fails """
-
-    def __init__(self, msg, retval=None):
-        Exception.__init__(self, msg)
-        self.retval = retval
-
-    def __str__(self):
-        return "%s (rv: %s)" % (Exception.__str__(self),
-                                          self.retval)
-
-
-class Executor(object):
-    """ A better version of Bcfg2.Client.Tool.Executor, which captures
-    stderr, raises exceptions on error, and doesn't use the shell to
-    execute by default """
-
-    def __init__(self, logger):
-        self.logger = logger
-        self.stdout = None
-        self.stderr = None
-        self.retval = None
-
-    def run(self, command, inputdata=None, shell=False):
-        """ Run a command, given as a list, optionally giving it the
-        specified input data """
-        self.logger.debug("Running: %s" % " ".join(command))
-        proc = subprocess.Popen(command, shell=shell, bufsize=16384,
-                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE, close_fds=True)
-        if inputdata:
-            for line in inputdata.splitlines():
-                self.logger.debug('> %s' % line)
-            (self.stdout, self.stderr) = proc.communicate(inputdata)
-        else:
-            (self.stdout, self.stderr) = proc.communicate()
-        for line in self.stdout.splitlines():  # pylint: disable=E1103
-            self.logger.debug('< %s' % line)
-        self.retval = proc.wait()
-        if self.retval == 0:
-            for line in self.stderr.splitlines():  # pylint: disable=E1103
-                self.logger.warning(line)
-            return True
-        else:
-            raise ExecutionError(self.stderr, self.retval)
+from Bcfg2.Utils import PackedDigitRange
 
 
 class POSIXUsers(Bcfg2.Client.Tools.Tool):
@@ -82,7 +33,6 @@ class POSIXUsers(Bcfg2.Client.Tools.Tool):
         Bcfg2.Client.Tools.Tool.__init__(self, logger, setup, config)
         self.set_defaults = dict(POSIXUser=self.populate_user_entry,
                                  POSIXGroup=lambda g: g)
-        self.cmd = Executor(logger)
         self._existing = None
         self._whitelist = dict(POSIXUser=None, POSIXGroup=None)
         self._blacklist = dict(POSIXUser=None, POSIXGroup=None)
@@ -273,16 +223,14 @@ class POSIXUsers(Bcfg2.Client.Tools.Tool):
             action = "add"
         else:
             action = "mod"
-        try:
-            self.cmd.run(self._get_cmd(action,
-                                       self.set_defaults[entry.tag](entry)))
+        rv = self.cmd.run(self._get_cmd(action,
+                                        self.set_defaults[entry.tag](entry)))
+        if rv.success:
             self.modified.append(entry)
-            return True
-        except ExecutionError:
+        else:
             self.logger.error("POSIXUsers: Error creating %s %s: %s" %
-                              (entry.tag, entry.get("name"),
-                               sys.exc_info()[1]))
-            return False
+                              (entry.tag, entry.get("name"), rv.error))
+        return rv.success
 
     def _get_cmd(self, action, entry):
         """ Get a command to perform the appropriate action (add, mod,
@@ -337,11 +285,8 @@ class POSIXUsers(Bcfg2.Client.Tools.Tool):
 
     def _remove(self, entry):
         """ Remove an entry """
-        try:
-            self.cmd.run(self._get_cmd("del", entry))
-            return True
-        except ExecutionError:
+        rv = self.cmd.run(self._get_cmd("del", entry))
+        if not rv.success:
             self.logger.error("POSIXUsers: Error deleting %s %s: %s" %
-                              (entry.tag, entry.get("name"),
-                               sys.exc_info()[1]))
-            return False
+                              (entry.tag, entry.get("name"), rv.error))
+        return rv.success
diff --git a/src/lib/Bcfg2/Client/Tools/Pacman.py b/src/lib/Bcfg2/Client/Tools/Pacman.py
index 9c14a3de6..12785afee 100644
--- a/src/lib/Bcfg2/Client/Tools/Pacman.py
+++ b/src/lib/Bcfg2/Client/Tools/Pacman.py
@@ -20,9 +20,8 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
 
     def RefreshPackages(self):
         '''Refresh memory hashes of packages'''
-        pkgcache = self.cmd.run("/usr/bin/pacman -Q")[1]
         self.installed = {}
-        for pkg in pkgcache:
+        for pkg in self.cmd.run("/usr/bin/pacman -Q").stdout.splitlines():
             pkgname = pkg.split(' ')[0].strip()
             version = pkg.split(' ')[1].strip()
             #self.logger.info(" pkgname: %s, version: %s" % (pkgname, version))
@@ -62,7 +61,7 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
         '''Remove extra packages'''
         names = [pkg.get('name') for pkg in packages]
         self.logger.info("Removing packages: %s" % " ".join(names))
-        self.cmd.run("%s --noconfirm --noprogressbar -R %s" % \
+        self.cmd.run("%s --noconfirm --noprogressbar -R %s" %
                      (self.pkgtool, " ".join(names)))
         self.RefreshPackages()
         self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/Portage.py b/src/lib/Bcfg2/Client/Tools/Portage.py
index 9381f44e9..6cbcff2e0 100644
--- a/src/lib/Bcfg2/Client/Tools/Portage.py
+++ b/src/lib/Bcfg2/Client/Tools/Portage.py
@@ -3,6 +3,7 @@
 import re
 import Bcfg2.Client.Tools
 
+
 class Portage(Bcfg2.Client.Tools.PkgTool):
     """The Gentoo toolset implements package and service operations and
     inherits the rest from Toolset.Toolset."""
@@ -35,9 +36,8 @@ class Portage(Bcfg2.Client.Tools.PkgTool):
         if not self._initialised:
             return
         self.logger.info('Getting list of installed packages')
-        cache = self.cmd.run("equery -q list '*'")[1]
         self.installed = {}
-        for pkg in cache:
+        for pkg in self.cmd.run("equery -q list '*'").stdout.splitlines():
             if self._pkg_pattern.match(pkg):
                 name = self._pkg_pattern.match(pkg).group(1)
                 version = self._pkg_pattern.match(pkg).group(2)
@@ -73,12 +73,12 @@ class Portage(Bcfg2.Client.Tools.PkgTool):
 
                 self.logger.debug('Running equery check on %s' %
                                   entry.get('name'))
-                output = self.cmd.run("/usr/bin/equery -N check '=%s-%s' "
-                                      "2>&1 | grep '!!!' | awk '{print $2}'"
-                                      % ((entry.get('name'), version)))[1]
-                if [filename for filename in output \
-                    if filename not in modlist]:
-                    return False
+                for line in self.cmd.run(["/usr/bin/equery", "-N", "check",
+                                          '=%s-%s' %
+                                          (entry.get('name'),
+                                           version)]).stdout.splitlines():
+                    if '!!!' in line and line.split()[1] not in modlist:
+                        return False
 
         # By now the package must be in one of the following states:
         # - Not require checking
diff --git a/src/lib/Bcfg2/Client/Tools/RPM.py b/src/lib/Bcfg2/Client/Tools/RPM.py
index 0964fdb80..e9dff3db5 100644
--- a/src/lib/Bcfg2/Client/Tools/RPM.py
+++ b/src/lib/Bcfg2/Client/Tools/RPM.py
@@ -78,13 +78,12 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
         # Many, if not most package verifies can be caused by out of
         # date prelinking.
         if os.path.isfile('/usr/sbin/prelink') and not self.setup['dryrun']:
-            cmdrc, output = self.cmd.run('/usr/sbin/prelink -a -mR')
-            if cmdrc == 0:
+            rv = self.cmd.run('/usr/sbin/prelink -a -mR')
+            if rv.success:
                 self.logger.debug('Pre-emptive prelink succeeded')
             else:
                 # FIXME : this is dumb - what if the output is huge?
-                self.logger.error('Pre-emptive prelink failed: %s' % output)
-
+                self.logger.error('Pre-emptive prelink failed: %s' % rv.error)
 
     def RefreshPackages(self):
         """
@@ -591,29 +590,26 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
         # Fix installOnlyPackages
         if len(install_only_pkgs) > 0:
             self.logger.info("Attempting to install 'install only packages'")
-            install_args = " ".join([os.path.join(self.instance_status[inst].get('pkg').get('uri'), \
-                                                  inst.get('simplefile')) \
-                                           for inst in install_only_pkgs])
-            self.logger.debug("rpm --install --quiet --oldpackage %s" % install_args)
-            cmdrc, output = self.cmd.run("rpm --install --quiet --oldpackage --replacepkgs %s" % \
-                                                                                     install_args)
-            if cmdrc == 0:
+            install_args = \
+                " ".join(os.path.join(self.instance_status[inst].get('pkg').get('uri'),
+                                      inst.get('simplefile'))
+                         for inst in install_only_pkgs)
+            if self.cmd.run("rpm --install --quiet --oldpackage --replacepkgs "
+                            "%s" % install_args):
                 # The rpm command succeeded.  All packages installed.
                 self.logger.info("Single Pass for InstallOnlyPkgs Succeded")
                 self.RefreshPackages()
-
             else:
                 # The rpm command failed.  No packages installed.
                 # Try installing instances individually.
                 self.logger.error("Single Pass for InstallOnlyPackages Failed")
                 installed_instances = []
                 for inst in install_only_pkgs:
-                    install_args = os.path.join(self.instance_status[inst].get('pkg').get('uri'), \
-                                                     inst.get('simplefile'))
-                    self.logger.debug("rpm --install --quiet --oldpackage %s" % install_args)
-                    cmdrc, output = self.cmd.run("rpm --install --quiet --oldpackage --replacepkgs %s" % \
-                                                                                      install_args)
-                    if cmdrc == 0:
+                    install_args = \
+                        os.path.join(self.instance_status[inst].get('pkg').get('uri'),
+                                     inst.get('simplefile'))
+                    if self.cmd.run("rpm --install --quiet --oldpackage "
+                                    "--replacepkgs %s" % install_args):
                         installed_instances.append(inst)
                     else:
                         self.logger.debug("InstallOnlyPackage %s %s would not install." % \
@@ -630,15 +626,15 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
                 self.logger.info("Installing GPG keys.")
                 key_arg = os.path.join(self.instance_status[inst].get('pkg').get('uri'), \
                                                      inst.get('simplefile'))
-                cmdrc, output = self.cmd.run("rpm --import %s" % key_arg)
-                if cmdrc != 0:
-                    self.logger.debug("Unable to install %s-%s" % \
-                                              (self.instance_status[inst].get('pkg').get('name'), \
-                                               self.str_evra(inst)))
+                if not self.cmd.run("rpm --import %s" % key_arg):
+                    self.logger.debug("Unable to install %s-%s" %
+                                      (self.instance_status[inst].get('pkg').get('name'),
+                                       self.str_evra(inst)))
                 else:
-                    self.logger.debug("Installed %s-%s-%s" % \
-                                              (self.instance_status[inst].get('pkg').get('name'), \
-                                               inst.get('version'), inst.get('release')))
+                    self.logger.debug("Installed %s-%s-%s" %
+                                      (self.instance_status[inst].get('pkg').get('name'),
+                                       inst.get('version'),
+                                       inst.get('release')))
             self.RefreshPackages()
             self.gpg_keyids = self.getinstalledgpg()
             pkg = self.instance_status[gpg_keys[0]].get('pkg')
@@ -650,13 +646,12 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
             upgrade_args = " ".join([os.path.join(self.instance_status[inst].get('pkg').get('uri'), \
                                                   inst.get('simplefile')) \
                                            for inst in upgrade_pkgs])
-            cmdrc, output = self.cmd.run("rpm --upgrade --quiet --oldpackage --replacepkgs %s" % \
-                                                       upgrade_args)
-            if cmdrc == 0:
+            if self.cmd.run("rpm --upgrade --quiet --oldpackage --replacepkgs "
+                            "%s" % upgrade_args):
                 # The rpm command succeeded.  All packages upgraded.
                 self.logger.info("Single Pass for Upgraded Packages Succeded")
-                upgrade_pkg_set = set([self.instance_status[inst].get('pkg') \
-                                                      for inst in upgrade_pkgs])
+                upgrade_pkg_set = set([self.instance_status[inst].get('pkg')
+                                       for inst in upgrade_pkgs])
                 self.RefreshPackages()
             else:
                 # The rpm command failed.  No packages upgraded.
@@ -668,13 +663,13 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
                                                      inst.get('simplefile'))
                     #self.logger.debug("rpm --upgrade --quiet --oldpackage --replacepkgs %s" % \
                     #                                                      upgrade_args)
-                    cmdrc, output = self.cmd.run("rpm --upgrade --quiet --oldpackage --replacepkgs %s" % upgrade_args)
-                    if cmdrc == 0:
+                    if self.cmd.run("rpm --upgrade --quiet --oldpackage "
+                                    "--replacepkgs %s" % upgrade_args):
                         upgraded_instances.append(inst)
                     else:
-                        self.logger.debug("Package %s %s would not upgrade." % \
-                                              (self.instance_status[inst].get('pkg').get('name'), \
-                                               self.str_evra(inst)))
+                        self.logger.debug("Package %s %s would not upgrade." %
+                                          (self.instance_status[inst].get('pkg').get('name'),
+                                           self.str_evra(inst)))
 
                 upgrade_pkg_set = set([self.instance_status[inst].get('pkg') \
                                                       for inst in upgrade_pkgs])
diff --git a/src/lib/Bcfg2/Client/Tools/RcUpdate.py b/src/lib/Bcfg2/Client/Tools/RcUpdate.py
index d5cef6e34..2e58f2564 100644
--- a/src/lib/Bcfg2/Client/Tools/RcUpdate.py
+++ b/src/lib/Bcfg2/Client/Tools/RcUpdate.py
@@ -23,8 +23,7 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
 
         # check if service is enabled
         cmd = '/sbin/rc-update show default | grep %s'
-        rv = self.cmd.run(cmd % entry.get('name'))[0]
-        is_enabled = (rv == 0)
+        is_enabled = self.cmd.run(cmd % entry.get('name')).success
 
         # check if init script exists
         try:
@@ -36,8 +35,7 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
 
         # check if service is enabled
         cmd = '/etc/init.d/%s status | grep started'
-        rv = self.cmd.run(cmd % entry.attrib['name'])[0]
-        is_running = (rv == 0)
+        is_running = self.cmd.run(cmd % entry.attrib['name']).success
 
         if entry.get('status') == 'on' and not (is_enabled and is_running):
             entry.set('current_status', 'off')
@@ -60,27 +58,25 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
                 self.start_service(entry)
             # make sure it's enabled
             cmd = '/sbin/rc-update add %s default'
-            rv = self.cmd.run(cmd % entry.get('name'))[0]
-            return (rv == 0)
-
+            return self.cmd.run(cmd % entry.get('name')).success
         elif entry.get('status') == 'off':
             if entry.get('current_status') == 'on':
                 self.stop_service(entry)
             # make sure it's disabled
             cmd = '/sbin/rc-update del %s default'
-            rv = self.cmd.run(cmd % entry.get('name'))[0]
-            return (rv == 0)
+            return self.cmd.run(cmd % entry.get('name')).success
 
         return False
 
     def FindExtra(self):
         """Locate extra rc-update services."""
-        cmd = '/bin/rc-status -s | grep started'
-        allsrv = [line.split()[0] for line in self.cmd.run(cmd)[1]]
+        cmd = '/bin/rc-status -s'
+        allsrv = [line.split()[0]
+                  for line in self.cmd.run(cmd).stdout.splitlines()
+                  if 'started' in line]
         self.logger.debug('Found active services:')
         self.logger.debug(allsrv)
         specified = [srv.get('name') for srv in self.getSupportedEntries()]
-        return [Bcfg2.Client.XML.Element('Service',
-                                         type='rc-update',
-                                         name=name) \
+        return [Bcfg2.Client.XML.Element('Service', type='rc-update',
+                                         name=name)
                 for name in allsrv if name not in specified]
diff --git a/src/lib/Bcfg2/Client/Tools/SMF.py b/src/lib/Bcfg2/Client/Tools/SMF.py
index 4409b40f3..68d8b2965 100644
--- a/src/lib/Bcfg2/Client/Tools/SMF.py
+++ b/src/lib/Bcfg2/Client/Tools/SMF.py
@@ -26,21 +26,20 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
     def GetFMRI(self, entry):
         """Perform FMRI resolution for service."""
         if not 'FMRI' in entry.attrib:
-            name = self.cmd.run("/usr/bin/svcs -H -o FMRI %s 2>/dev/null" % \
-                                entry.get('name'))[1]
-            if name:
-                entry.set('FMRI', name[0])
-                return True
+            rv = self.cmd.run(["/usr/bin/svcs", "-H", "-o", "FMRI",
+                               entry.get('name')])
+            if rv.success:
+                entry.set('FMRI', rv.stdout.splitlines()[0])
             else:
-                self.logger.info('Failed to locate FMRI for service %s' % \
+                self.logger.info('Failed to locate FMRI for service %s' %
                                  entry.get('name'))
-                return False
+            return rv.success
         return True
 
     def VerifyService(self, entry, _):
         """Verify SMF Service entry."""
         if not self.GetFMRI(entry):
-            self.logger.error("smf service %s doesn't have FMRI set" % \
+            self.logger.error("smf service %s doesn't have FMRI set" %
                               entry.get('name'))
             return False
         if entry.get('FMRI').startswith('lrc'):
@@ -57,8 +56,9 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
                                   (entry.get("FMRI")))
                 return entry.get('status') == 'off'
         try:
-            srvdata = self.cmd.run("/usr/bin/svcs -H -o STA %s" % \
-                                   entry.get('FMRI'))[1][0].split()
+            srvdata = \
+                self.cmd.run("/usr/bin/svcs -H -o STA %s" %
+                             entry.get('FMRI')).stdout.splitlines()[0].split()
         except IndexError:
             # Occurs when no lines are returned (service not installed)
             return False
@@ -85,31 +85,30 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
                                       (loc))
                     return False
             else:
-                cmdrc = self.cmd.run("/usr/sbin/svcadm disable %s" % \
-                                     (entry.get('FMRI')))[0]
+                return self.cmd.run("/usr/sbin/svcadm disable %s" %
+                                    entry.get('FMRI')).success
+        elif entry.get('FMRI').startswith('lrc'):
+            loc = entry.get("FMRI")[4:].replace('_', '.')
+            try:
+                os.stat(loc.replace('/S', '/Disabled.'))
+                self.logger.debug("Renaming file %s to %s" %
+                                  (loc.replace('/S', '/DISABLED.S'), loc))
+                os.rename(loc.replace('/S', '/DISABLED.S'), loc)
+                return True
+            except OSError:
+                self.logger.debug("Failed to rename %s to %s" %
+                                  (loc.replace('/S', '/DISABLED.S'), loc))
+                return False
         else:
-            if entry.get('FMRI').startswith('lrc'):
-                loc = entry.get("FMRI")[4:].replace('_', '.')
-                try:
-                    os.stat(loc.replace('/S', '/Disabled.'))
-                    self.logger.debug("Renaming file %s to %s" % \
-                                      (loc.replace('/S', '/DISABLED.S'), loc))
-                    os.rename(loc.replace('/S', '/DISABLED.S'), loc)
-                    cmdrc = 0
-                except OSError:
-                    self.logger.debug("Failed to rename %s to %s" % \
-                                      (loc.replace('/S', '/DISABLED.S'), loc))
-                    cmdrc = 1
+            srvdata = \
+                self.cmd.run("/usr/bin/svcs -H -o STA %s" %
+                             entry.get('FMRI'))[1].splitlines()[0].split()
+            if srvdata[0] == 'MNT':
+                cmdarg = 'clear'
             else:
-                srvdata = self.cmd.run("/usr/bin/svcs -H -o STA %s" %
-                                       entry.get('FMRI'))[1][0].split()
-                if srvdata[0] == 'MNT':
-                    cmdarg = 'clear'
-                else:
-                    cmdarg = 'enable'
-                cmdrc = self.cmd.run("/usr/sbin/svcadm %s -r %s" % \
-                                     (cmdarg, entry.get('FMRI')))[0]
-        return cmdrc == 0
+                cmdarg = 'enable'
+            return self.cmd.run("/usr/sbin/svcadm %s -r %s" %
+                                (cmdarg, entry.get('FMRI'))).success
 
     def Remove(self, svcs):
         """Remove Extra SMF entries."""
@@ -120,12 +119,14 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
     def FindExtra(self):
         """Find Extra SMF Services."""
         allsrv = [name for name, version in \
-                  [srvc.split() for srvc in
-                   self.cmd.run("/usr/bin/svcs -a -H -o FMRI,STATE")[1]]
+                  [srvc.split()
+                   for srvc in self.cmd.run([
+                        "/usr/bin/svcs", "-a", "-H",
+                        "-o", "FMRI,STATE"]).stdout.splitlines()]
                   if version != 'disabled']
 
         for svc in self.getSupportedEntries():
             if svc.get("FMRI") in allsrv:
                 allsrv.remove(svc.get('FMRI'))
-        return [Bcfg2.Client.XML.Element("Service", type='smf', name=name) \
+        return [Bcfg2.Client.XML.Element("Service", type='smf', name=name)
                 for name in allsrv]
diff --git a/src/lib/Bcfg2/Client/Tools/SYSV.py b/src/lib/Bcfg2/Client/Tools/SYSV.py
index 9b84a14cc..38072c52e 100644
--- a/src/lib/Bcfg2/Client/Tools/SYSV.py
+++ b/src/lib/Bcfg2/Client/Tools/SYSV.py
@@ -1,11 +1,11 @@
 """This provides bcfg2 support for Solaris SYSV packages."""
 
 import tempfile
-
+from Bcfg2.Compat import any  # pylint: disable=W0622
 import Bcfg2.Client.Tools
 import Bcfg2.Client.XML
 
-
+# pylint: disable=C0103
 noask = '''
 mail=
 instance=overwrite
@@ -19,6 +19,7 @@ conflict=nocheck
 action=nocheck
 basedir=default
 '''
+# pylint: enable=C0103
 
 
 class SYSV(Bcfg2.Client.Tools.PkgTool):
@@ -42,14 +43,14 @@ class SYSV(Bcfg2.Client.Tools.PkgTool):
             self.noaskfile.flush()
             self.pkgtool = (self.pkgtool[0] % ("-a %s" % (self.noaskname)), \
                             self.pkgtool[1])
-        except:
-            self.pkgtool = (self.pkgtool[0] % (""), self.pkgtool[1])
+        except:  # pylint: disable=W0702
+            self.pkgtool = (self.pkgtool[0] % "", self.pkgtool[1])
 
     def RefreshPackages(self):
         """Refresh memory hashes of packages."""
         self.installed = {}
         # Build list of packages
-        lines = self.cmd.run("/usr/bin/pkginfo -x")[1]
+        lines = self.cmd.run("/usr/bin/pkginfo -x").stdout.splitlines()
         while lines:
             # Splitting on whitespace means that packages with spaces in
             # their version numbers don't work right.  Found this with
@@ -62,35 +63,36 @@ class SYSV(Bcfg2.Client.Tools.PkgTool):
 
     def VerifyPackage(self, entry, modlist):
         """Verify Package status for entry."""
-        if not entry.get('version'):
-            self.logger.info("Insufficient information of Package %s; cannot Verify" % entry.get('name'))
-            return False
-
-        desiredVersion = entry.get('version')
-        if desiredVersion == 'any':
-            desiredVersion = self.installed.get(entry.get('name'), desiredVersion)
-
-        cmdrc = self.cmd.run("/usr/bin/pkginfo -q -v \"%s\" %s" % \
-                             (desiredVersion, entry.get('name')))[0]
+        desired_version = entry.get('version')
+        if desired_version == 'any':
+            desired_version = self.installed.get(entry.get('name'),
+                                                desired_version)
 
-        if cmdrc != 0:
+        if not self.cmd.run(["/usr/bin/pkginfo", "-q", "-v",
+                             desired_version, entry.get('name')]):
             if entry.get('name') in self.installed:
-                self.logger.debug("Package %s version incorrect: have %s want %s" \
-                                  % (entry.get('name'), self.installed[entry.get('name')],
-                                     desiredVersion))
+                self.logger.debug("Package %s version incorrect: "
+                                  "have %s want %s" %
+                                  (entry.get('name'),
+                                   self.installed[entry.get('name')],
+                                   desired_version))
             else:
-                self.logger.debug("Package %s not installed" % (entry.get("name")))
+                self.logger.debug("Package %s not installed" %
+                                  entry.get("name"))
         else:
-            if self.setup['quick'] or entry.attrib.get('verify', 'true') == 'false':
+            if (self.setup['quick'] or
+                entry.attrib.get('verify', 'true') == 'false'):
                 return True
-            (vstat, odata) = self.cmd.run("/usr/sbin/pkgchk -n %s" % (entry.get('name')))
-            if vstat == 0:
+            rv = self.cmd.run("/usr/sbin/pkgchk -n %s" % entry.get('name'))
+            if rv.success:
                 return True
             else:
-                output = [line for line in odata if line[:5] == 'ERROR']
-                if len([name for name in output if name.split()[-1] not in modlist]):
-                    self.logger.debug("Package %s content verification failed" % \
-                                      (entry.get('name')))
+                output = [line for line in rv.stdout.splitlines()
+                          if line[:5] == 'ERROR']
+                if any(name for name in output
+                       if name.split()[-1] not in modlist):
+                    self.logger.debug("Package %s content verification failed"
+                                      % entry.get('name'))
                 else:
                     return True
         return False
@@ -99,7 +101,7 @@ class SYSV(Bcfg2.Client.Tools.PkgTool):
         """Remove specified Sysv packages."""
         names = [pkg.get('name') for pkg in packages]
         self.logger.info("Removing packages: %s" % (names))
-        self.cmd.run("/usr/sbin/pkgrm -a %s -n %s" % \
+        self.cmd.run("/usr/sbin/pkgrm -a %s -n %s" %
                      (self.noaskname, names))
         self.RefreshPackages()
         self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/Systemd.py b/src/lib/Bcfg2/Client/Tools/Systemd.py
index 43eca2eac..027d91c71 100644
--- a/src/lib/Bcfg2/Client/Tools/Systemd.py
+++ b/src/lib/Bcfg2/Client/Tools/Systemd.py
@@ -5,6 +5,7 @@
 import Bcfg2.Client.Tools
 import Bcfg2.Client.XML
 
+
 class Systemd(Bcfg2.Client.Tools.SvcTool):
     """Systemd support for Bcfg2."""
     name = 'Systemd'
@@ -21,35 +22,25 @@ class Systemd(Bcfg2.Client.Tools.SvcTool):
             return True
 
         cmd = "/bin/systemctl status %s.service " % (entry.get('name'))
-        raw = ''.join(self.cmd.run(cmd)[1])
+        rv = self.cmd.run(cmd)
 
-        if raw.find('Loaded: error') >= 0:
+        if 'Loaded: error' in rv.stdout:
             entry.set('current_status', 'off')
-            status = False
-
-        elif raw.find('Active: active') >= 0:
+            return False
+        elif 'Active: active' in rv.stdout:
             entry.set('current_status', 'on')
-            if entry.get('status') == 'off':
-                status = False
-            else:
-                status = True
-
+            return entry.get('status') == 'on'
         else:
             entry.set('current_status', 'off')
-            if entry.get('status') == 'on':
-                status = False
-            else:
-                status = True
-
-        return status
+            return entry.get('status') == 'off'
 
     def InstallService(self, entry):
         """Install Service entry."""
         if entry.get('status') == 'on':
-            rv = self.cmd.run(self.get_svc_command(entry, 'enable'))[0] == 0
-            rv &= self.cmd.run(self.get_svc_command(entry, 'start'))[0] == 0
+            rv = self.cmd.run(self.get_svc_command(entry, 'enable')).success
+            rv &= self.cmd.run(self.get_svc_command(entry, 'start')).success
         else:
-            rv = self.cmd.run(self.get_svc_command(entry, 'stop'))[0] == 0
-            rv &= self.cmd.run(self.get_svc_command(entry, 'disable'))[0] == 0
+            rv = self.cmd.run(self.get_svc_command(entry, 'stop')).success
+            rv &= self.cmd.run(self.get_svc_command(entry, 'disable')).success
 
         return rv
diff --git a/src/lib/Bcfg2/Client/Tools/Upstart.py b/src/lib/Bcfg2/Client/Tools/Upstart.py
index 02ed52544..cd1c4a2bc 100644
--- a/src/lib/Bcfg2/Client/Tools/Upstart.py
+++ b/src/lib/Bcfg2/Client/Tools/Upstart.py
@@ -39,7 +39,8 @@ class Upstart(Bcfg2.Client.Tools.SvcTool):
 
         try:
             output = self.cmd.run('/usr/sbin/service %s status %s' %
-                                  (entry.get('name'), params))[1][0]
+                                  (entry.get('name'),
+                                   params)).stdout.splitlines()[0]
         except IndexError:
             self.logger.error("Service %s not an Upstart service" %
                               entry.get('name'))
@@ -71,11 +72,10 @@ class Upstart(Bcfg2.Client.Tools.SvcTool):
     def InstallService(self, entry):
         """Install Service for entry."""
         if entry.get('status') == 'on':
-            pstatus = self.cmd.run(self.get_svc_command(entry, 'start'))[0]
+            cmd = "start"
         elif entry.get('status') == 'off':
-            pstatus = self.cmd.run(self.get_svc_command(entry, 'stop'))[0]
-        # pstatus is true if command failed
-        return not pstatus
+            cmd = "stop"
+        return self.cmd.run(self.get_svc_command(entry, cmd)).success
 
     def FindExtra(self):
         """Locate extra Upstart services."""
diff --git a/src/lib/Bcfg2/Client/Tools/__init__.py b/src/lib/Bcfg2/Client/Tools/__init__.py
index c0dd60c1e..7014f334f 100644
--- a/src/lib/Bcfg2/Client/Tools/__init__.py
+++ b/src/lib/Bcfg2/Client/Tools/__init__.py
@@ -4,7 +4,8 @@ import os
 import sys
 import stat
 import select
-from subprocess import Popen, PIPE
+import logging
+import subprocess
 import Bcfg2.Client.XML
 from Bcfg2.Compat import input, walk_packages  # pylint: disable=W0622
 
@@ -25,34 +26,89 @@ class ToolInstantiationError(Exception):
     pass
 
 
-class Executor:
-    """ This class runs shell commands. """
+class ExecutorResult(object):
+    """ Returned as the result of a call to
+    :func:`Bcfg2.Client.Tools.Executor.run`. The result can be
+    accessed via the instance variables, documented below, as a
+    boolean (which is equivalent to
+    :attr:`Bcfg2.Client.Tools.ExecutorResult.success`), or as a tuple,
+    which, for backwards compatibility, is equivalent to
+    ``(result.retval, result.stdout.splitlines())``. """
+
+    def __init__(self, stdout, stderr, retval):
+        #: The output of the command
+        self.stdout = stdout
+
+        #: The error produced by the command
+        self.stderr = stderr
+
+        #: The return value of the command.
+        self.retval = retval
+
+        #: Whether or not the command was successful.  If the
+        #: ExecutorResult is used as a boolean, ``success`` is
+        #: returned.
+        self.success = retval == 0
+
+        #: A friendly error message
+        self.error = None
+        if self.retval:
+            if self.stderr:
+                self.error = "%s (rv: %s)" % (self.stderr, self.retval)
+            elif self.stdout:
+                self.error = "%s (rv: %s)" % (self.stdout, self.retval)
+            else:
+                self.error = "No output or error; return value %s" % \
+                    self.retval
+
+    def __repr__(self):
+        if self.error:
+            return "Errored command result: %s" % self.error
+        elif self.stdout:
+            return "Successful command result: %s" % self.stdout
+        else:
+            return "Successful command result: No output"
 
-    def __init__(self, logger):
-        """
-        :param logger: The logger to use to produce debug logging
-        :type logger: logging.Logger
-        """
-        self.logger = logger
+    def __getitem__(self, idx):
+        """ This provides compatibility with the old Executor, which
+        returned a tuple of (return value, stdout split by lines). """
+        return (self.retval, self.stdout.splitlines())[idx]
 
-    def run(self, command):
-        """ Run a command inside a shell.
+    def __nonzero__(self):
+        return self.__bool__()
 
-        :param command: The command to run, given as a list as to
-                        :class:`subprocess.Popen`.  Since the command
-                        will be run within a shell it is particularly
-                        important to pass it as a list.
-        :type command: list
-        :returns: tuple of return value (integer) and output (list of
-                  lines)
-        """
-        self.logger.debug("Running: %s" % command)
-        proc = Popen(command, shell=True, bufsize=16384,
-                     stdin=PIPE, stdout=PIPE, close_fds=True)
-        output = proc.communicate()[0].splitlines()
-        for line in output:
+    def __bool__(self):
+        return self.success
+
+
+class Executor(object):
+    """ A better version of Bcfg2.Client.Tool.Executor, which captures
+    stderr, raises exceptions on error, and doesn't use the shell to
+    execute by default """
+
+    def __init__(self):
+        self.logger = logging.getLogger(self.__class__.__name__)
+
+    def run(self, command, inputdata=None, shell=False):
+        """ Run a command, given as a list, optionally giving it the
+        specified input data """
+        if isinstance(command, str):
+            cmdstr = command
+        else:
+            cmdstr = " ".join(command)
+        self.logger.debug("Running: %s" % cmdstr)
+        proc = subprocess.Popen(command, shell=shell, bufsize=16384,
+                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE, close_fds=True)
+        if inputdata:
+            for line in inputdata.splitlines():
+                self.logger.debug('> %s' % line)
+        (stdout, stderr) = proc.communicate(input=inputdata)
+        for line in stdout.splitlines():  # pylint: disable=E1103
             self.logger.debug('< %s' % line)
-        return (proc.wait(), output)
+        for line in stderr.splitlines():  # pylint: disable=E1103
+            self.logger.info(line)
+        return ExecutorResult(stdout, stderr, proc.wait())
 
 
 class ClassName(object):
@@ -143,7 +199,7 @@ class Tool(object):
 
         #: An :class:`Bcfg2.Client.Tools.Executor` object for
         #: running external commands.
-        self.cmd = Executor(logger)
+        self.cmd = Executor()
 
         #: A list of entries that have been modified by this tool
         self.modified = []
@@ -469,10 +525,7 @@ class PkgTool(Tool):
         pkgargs = " ".join([self.pkgtool[1][0] % datum for datum in data])
 
         self.logger.debug("Installing packages: %s" % pkgargs)
-        self.logger.debug("Running command: %s" % (self.pkgtool[0] % pkgargs))
-
-        cmdrc = self.cmd.run(self.pkgtool[0] % pkgargs)[0]
-        if cmdrc == 0:
+        if self.cmd.run(self.pkgtool[0] % pkgargs):
             self.logger.info("Single Pass Succeded")
             # set all package states to true and flush workqueues
             pkgnames = [pkg.get('name') for pkg in packages]
@@ -497,12 +550,11 @@ class PkgTool(Tool):
                 else:
                     self.logger.info("Installing pkg %s version %s" %
                                      (pkg.get('name'), pkg.get('version')))
-                    cmdrc = self.cmd.run(
+                    if self.cmd.run(
                         self.pkgtool[0] %
                         (self.pkgtool[1][0] %
                          tuple([pkg.get(field)
-                                for field in self.pkgtool[1][1]])))
-                    if cmdrc[0] == 0:
+                                for field in self.pkgtool[1][1]]))):
                         states[pkg] = True
                     else:
                         self.logger.error("Failed to install package %s" %
@@ -557,7 +609,7 @@ class SvcTool(Tool):
                   :class:`Bcfg2.Client.Tools.Executor.run`
         """
         self.logger.debug('Starting service %s' % service.get('name'))
-        return self.cmd.run(self.get_svc_command(service, 'start'))[0]
+        return self.cmd.run(self.get_svc_command(service, 'start'))
 
     def stop_service(self, service):
         """ Stop a service.
@@ -568,7 +620,7 @@ class SvcTool(Tool):
                   :class:`Bcfg2.Client.Tools.Executor.run`
         """
         self.logger.debug('Stopping service %s' % service.get('name'))
-        return self.cmd.run(self.get_svc_command(service, 'stop'))[0]
+        return self.cmd.run(self.get_svc_command(service, 'stop'))
 
     def restart_service(self, service):
         """ Restart a service.
@@ -580,7 +632,7 @@ class SvcTool(Tool):
         """
         self.logger.debug('Restarting service %s' % service.get('name'))
         restart_target = service.get('target', 'restart')
-        return self.cmd.run(self.get_svc_command(service, restart_target))[0]
+        return self.cmd.run(self.get_svc_command(service, restart_target))
 
     def check_service(self, service):
         """ Check the status a service.
@@ -590,7 +642,7 @@ class SvcTool(Tool):
         :returns: bool - True if the status command returned 0, False
                   otherwise
         """
-        return self.cmd.run(self.get_svc_command(service, 'status'))[0] == 0
+        return self.cmd.run(self.get_svc_command(service, 'status'))
 
     def Remove(self, services):
         if self.setup['servicemode'] != 'disabled':
@@ -610,10 +662,10 @@ class SvcTool(Tool):
                  not self.setup['interactive'])):
                 continue
 
-            rv = None
+            success = False
             if entry.get('status') == 'on':
                 if self.setup['servicemode'] == 'build':
-                    rv = self.stop_service(entry)
+                    success = self.stop_service(entry)
                 elif entry.get('name') not in self.restarted:
                     if self.setup['interactive']:
                         prompt = ('Restart service %s?: (y/N): ' %
@@ -625,12 +677,12 @@ class SvcTool(Tool):
                         ans = input(prompt)
                         if ans not in ['y', 'Y']:
                             continue
-                    rv = self.restart_service(entry)
-                    if not rv:
+                    success = self.restart_service(entry)
+                    if success:
                         self.restarted.append(entry.get('name'))
             else:
-                rv = self.stop_service(entry)
-            if rv:
+                success = self.stop_service(entry)
+            if not success:
                 self.logger.error("Failed to manipulate service %s" %
                                   (entry.get('name')))
     BundleUpdated.__doc__ = Tool.BundleUpdated.__doc__
diff --git a/src/lib/Bcfg2/Client/Tools/launchd.py b/src/lib/Bcfg2/Client/Tools/launchd.py
index 0a587da2e..b0661b26b 100644
--- a/src/lib/Bcfg2/Client/Tools/launchd.py
+++ b/src/lib/Bcfg2/Client/Tools/launchd.py
@@ -1,61 +1,58 @@
 """launchd support for Bcfg2."""
 
 import os
-
 import Bcfg2.Client.Tools
 
 
-class launchd(Bcfg2.Client.Tools.Tool):
-    """Support for Mac OS X launchd services."""
+class launchd(Bcfg2.Client.Tools.Tool):  # pylint: disable=C0103
+    """Support for Mac OS X launchd services.  Currently requires the
+    path to the plist to load/unload, and Name is acually a
+    reverse-fqdn (or the label)."""
     __handles__ = [('Service', 'launchd')]
     __execs__ = ['/bin/launchctl', '/usr/bin/defaults']
-    name = 'launchd'
     __req__ = {'Service': ['name', 'status']}
 
-    '''
-    Currently requires the path to the plist to load/unload,
-    and Name is acually a reverse-fqdn (or the label).
-    '''
-
     def __init__(self, logger, setup, config):
         Bcfg2.Client.Tools.Tool.__init__(self, logger, setup, config)
 
-        '''Locate plist file that provides given reverse-fqdn name
-        /Library/LaunchAgents          Per-user agents provided by the administrator.
-        /Library/LaunchDaemons         System wide daemons provided by the administrator.
-        /System/Library/LaunchAgents   Mac OS X Per-user agents.
-        /System/Library/LaunchDaemons  Mac OS X System wide daemons.'''
-        plistLocations = ["/Library/LaunchDaemons",
-                          "/System/Library/LaunchDaemons"]
-        self.plistMapping = {}
-        for directory in plistLocations:
+        # Locate plist file that provides given reverse-fqdn name:
+        #
+        # * ``/Library/LaunchAgents``: Per-user agents provided by the
+        #   administrator.
+        # * ``/Library/LaunchDaemons``: System-wide daemons provided
+        #   by the administrator.
+        # * ``/System/Library/LaunchAgents``: Mac OS X per-user
+        #   agents.
+        # * ``/System/Library/LaunchDaemons``: Mac OS X system-wide
+        #   daemons.
+        plist_locations = ["/Library/LaunchDaemons",
+                           "/System/Library/LaunchDaemons"]
+        self.plist_mapping = {}
+        for directory in plist_locations:
             for daemon in os.listdir(directory):
-                try:
-                    if daemon.endswith(".plist"):
-                        d = daemon[:-6]
-                    else:
-                        d = daemon
-                    label = self.cmd.run('defaults read %s/%s Label' %
-                                         (directory, d))[1][0]
-                    self.plistMapping[label] = "%s/%s" % (directory, daemon)
-                except KeyError:
-                    self.logger.warning("Could not get label from %s/%s" %
-                                        (directory, daemon))
+                if daemon.endswith(".plist"):
+                    daemon = daemon[:-6]
+                dpath = os.path.join(directory, daemon)
+                rv = self.cmd.run(['defaults', 'read', dpath, 'Label'])
+                if rv.success:
+                    label = rv.stdout.splitlines()[0]
+                    self.plist_mapping[label] = dpath
+                else:
+                    self.logger.warning("Could not get label from %s" % dpath)
 
     def FindPlist(self, entry):
-        return self.plistMapping.get(entry.get('name'), None)
+        """ Find the location of the plist file for the given entry """
+        return self.plist_mapping.get(entry.get('name'), None)
 
     def os_version(self):
-        version = ""
-        try:
-            vers = self.cmd.run('sw_vers')[1]
-        except:
-            return version
-
-        for line in vers:
-            if line.startswith("ProductVersion"):
-                version = line.split()[-1]
-        return version
+        """ Determine the OS version """
+        rv = self.cmd.run('sw_vers')
+        if rv:
+            for line in rv.stdout.splitlines():
+                if line.startswith("ProductVersion"):
+                    return line.split()[-1]
+        else:
+            return ''
 
     def VerifyService(self, entry, _):
         """Verify launchd service entry."""
@@ -63,7 +60,7 @@ class launchd(Bcfg2.Client.Tools.Tool):
             return True
 
         try:
-            services = self.cmd.run("/bin/launchctl list")[1]
+            services = self.cmd.run("/bin/launchctl list").stdout.splitlines()
         except IndexError:
             # happens when no services are running (should be never)
             services = []
@@ -93,15 +90,13 @@ class launchd(Bcfg2.Client.Tools.Tool):
         name = entry.get('name')
         if entry.get('status') == 'on':
             self.logger.error("Installing service %s" % name)
-            cmdrc = self.cmd.run("/bin/launchctl load -w %s" %
-                                 self.FindPlist(entry))
-            cmdrc = self.cmd.run("/bin/launchctl start %s" % name)
+            self.cmd.run("/bin/launchctl load -w %s" % self.FindPlist(entry))
+            return self.cmd.run("/bin/launchctl start %s" % name).success
         else:
             self.logger.error("Uninstalling service %s" % name)
-            cmdrc = self.cmd.run("/bin/launchctl stop %s" % name)
-            cmdrc = self.cmd.run("/bin/launchctl unload -w %s" %
-                                 self.FindPlist(entry))
-        return cmdrc[0] == 0
+            self.cmd.run("/bin/launchctl stop %s" % name)
+            return self.cmd.run("/bin/launchctl unload -w %s" %
+                                self.FindPlist(entry)).success
 
     def Remove(self, svcs):
         """Remove Extra launchd entries."""
@@ -110,23 +105,24 @@ class launchd(Bcfg2.Client.Tools.Tool):
     def FindExtra(self):
         """Find Extra launchd services."""
         try:
-            allsrv = self.cmd.run("/bin/launchctl list")[1]
+            allsrv = self.cmd.run("/bin/launchctl list").stdout.splitlines()
         except IndexError:
             allsrv = []
 
-        [allsrv.remove(svc) for svc in [entry.get("name") for entry
-                                        in self.getSupportedEntries()] if svc in allsrv]
-        return [Bcfg2.Client.XML.Element("Service",
-                                         type='launchd',
-                                         name=name,
-                                         status='on') for name in allsrv]
+        for entry in self.getSupportedEntries():
+            svc = entry.get("name")
+            if svc in allsrv:
+                allsrv.remove(svc)
+        return [Bcfg2.Client.XML.Element("Service", type='launchd', name=name,
+                                         status='on')
+                for name in allsrv]
 
     def BundleUpdated(self, bundle, states):
         """Reload launchd plist."""
         for entry in [entry for entry in bundle if self.handlesEntry(entry)]:
             if not self.canInstall(entry):
-                self.logger.error("Insufficient information to restart service %s" %
-                                  (entry.get('name')))
+                self.logger.error("Insufficient information to restart "
+                                  "service %s" % entry.get('name'))
             else:
                 name = entry.get('name')
                 if entry.get('status') == 'on' and self.FindPlist(entry):