extended usage of Executor class, added client-side timeout options

3 files changed, 25 insertions, 129 deletions

diff --git a/src/lib/Bcfg2/Client/Client.py b/src/lib/Bcfg2/Client/Client.py
index 6aa9ace1f..0488fcf21 100644
--- a/src/lib/Bcfg2/Client/Client.py
+++ b/src/lib/Bcfg2/Client/Client.py
@@ -14,10 +14,9 @@ import Bcfg2.Client.XML
 import Bcfg2.Client.Proxy
 import Bcfg2.Client.Frame
 import Bcfg2.Client.Tools
-from Bcfg2.Utils import locked
+from Bcfg2.Utils import locked, Executor
 from Bcfg2.Compat import xmlrpclib
 from Bcfg2.version import __version__
-from subprocess import Popen, PIPE
 
 
 class Client(object):
@@ -42,6 +41,9 @@ class Client(object):
                                    to_file=self.setup['logging'])
         self.logger = logging.getLogger('bcfg2')
         self.logger.debug(self.setup)
+
+        self.cmd = Executor(self.setup['command_timeout'])
+
         if self.setup['bundle_quick']:
             if not self.setup['bundle'] and not self.setup['skipbundle']:
                 self.logger.error("-Q option requires -b or -B")
@@ -95,16 +97,14 @@ class Client(object):
                          stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH |
                          stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH |
                          stat.S_IWUSR)  # 0755
-                proc = Popen(scriptname, stdin=PIPE, stdout=PIPE, stderr=PIPE)
-                ret.text, err = proc.communicate()
-                rv = proc.wait()
-                if err:
+                rv = self.cmd.run(scriptname, timeout=self.setup['timeout'])
+                if rv.stderr:
                     self.logger.warning("Probe %s has error output: %s" %
-                                        (name, err))
-                if rv:
+                                        (name, rv.stderr))
+                if not rv.success:
                     self._probe_failure(name, "Return value %s" % rv)
                 self.logger.info("Probe %s has result:" % name)
-                self.logger.info(ret.text)
+                self.logger.info(rv.stdout)
             finally:
                 os.unlink(scriptname)
         except SystemExit:
diff --git a/src/lib/Bcfg2/Client/Tools/SELinux.py b/src/lib/Bcfg2/Client/Tools/SELinux.py
index 414ca1f93..08d943251 100644
--- a/src/lib/Bcfg2/Client/Tools/SELinux.py
+++ b/src/lib/Bcfg2/Client/Tools/SELinux.py
@@ -12,7 +12,6 @@ import seobject
 import Bcfg2.Client.XML
 import Bcfg2.Client.Tools
 from Bcfg2.Client.Tools.POSIX.File import POSIXFile
-from subprocess import Popen, PIPE
 
 
 def pack128(int_val):
@@ -734,9 +733,7 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                 self._all = dict()
                 self.logger.debug("SELinux: Getting modules from semodule")
                 try:
-                    proc = Popen(['semodule', '-l'], stdout=PIPE, stderr=PIPE)
-                    out = proc.communicate()[0]
-                    rv = proc.wait()
+                    rv = self.tool.cmd.run(['semodule', '-l'])
                 except OSError:
                     # semanage failed; probably not in $PATH.  try to
                     # get the list of modules from the filesystem
@@ -745,13 +742,9 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                                       err)
                     self._all.update(self._all_records_from_filesystem())
                 else:
-                    if rv:
-                        self.logger.error("SELinux: Failed to run semodule: %s"
-                                          % err)
-                        self._all.update(self._all_records_from_filesystem())
-                    else:
+                    if rv.success:
                         # ran semodule successfully
-                        for line in out.splitlines():
+                        for line in rv.stdout.splitlines():
                             mod, version = line.split()
                             self._all[mod] = (version, 1)
 
@@ -759,6 +752,10 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                         for mod in self._all_records_from_filesystem().keys():
                             if mod not in self._all:
                                 self._all[mod] = ('', 0)
+                    else:
+                        self.logger.error("SELinux: Failed to run semodule: %s"
+                                          % rv.error)
+                        self._all.update(self._all_records_from_filesystem())
         return self._all
 
     def _all_records_from_filesystem(self):
@@ -870,26 +867,23 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
         self.logger.debug("Install SELinux module %s with semodule -i %s" %
                           (entry.get('name'), self._filepath(entry)))
         try:
-            proc = Popen(['semodule', '-i', self._filepath(entry)],
-                         stdout=PIPE, stderr=PIPE)
-            err = proc.communicate()[1]
-            rv = proc.wait()
+            rv = self.tool.cmd.run(['semodule', '-i', self._filepath(entry)])
         except OSError:
             err = sys.exc_info()[1]
             self.logger.error("Failed to install SELinux module %s with "
                               "semodule: %s" % (entry.get("name"), err))
             return False
-        if rv:
-            self.logger.error("Failed to install SELinux module %s with "
-                              "semodule: %s" % (entry.get("name"), err))
-            return False
-        else:
+        if rv.success:
             if entry.get("disabled", "false").lower() == "true":
                 self.logger.warning("SELinux: Cannot disable modules with "
                                     "semodule")
                 return False
             else:
                 return True
+        else:
+            self.logger.error("Failed to install SELinux module %s with "
+                              "semodule: %s" % (entry.get("name"), rv.error))
+            return False
 
     def _addargs(self, entry):
         """ argument list for adding entries """
diff --git a/src/lib/Bcfg2/Client/Tools/__init__.py b/src/lib/Bcfg2/Client/Tools/__init__.py
index 7014f334f..cd86a2a4b 100644
--- a/src/lib/Bcfg2/Client/Tools/__init__.py
+++ b/src/lib/Bcfg2/Client/Tools/__init__.py
@@ -4,9 +4,8 @@ import os
 import sys
 import stat
 import select
-import logging
-import subprocess
 import Bcfg2.Client.XML
+from Bcfg2.Utils import Executor, ClassName
 from Bcfg2.Compat import input, walk_packages  # pylint: disable=W0622
 
 __all__ = [m[1] for m in walk_packages(path=__path__)]
@@ -26,103 +25,6 @@ class ToolInstantiationError(Exception):
     pass
 
 
-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 __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 __nonzero__(self):
-        return self.__bool__()
-
-    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)
-        for line in stderr.splitlines():  # pylint: disable=E1103
-            self.logger.info(line)
-        return ExecutorResult(stdout, stderr, proc.wait())
-
-
-class ClassName(object):
-    """ This very simple descriptor class exists only to get the name
-    of the owner class.  This is used because, for historical reasons,
-    we expect every tool to have a ``name`` attribute that is in
-    almost all cases the same as the ``__class__.__name__`` attribute
-    of the plugin object.  This makes that more dynamic so that each
-    plugin isn't repeating its own name."""
-
-    def __get__(self, inst, owner):
-        return owner.__name__
-
-
 class Tool(object):
     """ The base tool class.  All tools subclass this.
 
@@ -197,9 +99,9 @@ class Tool(object):
         #: The XML configuration for this client
         self.config = config
 
-        #: An :class:`Bcfg2.Client.Tools.Executor` object for
+        #: An :class:`Bcfg2.Utils.Executor` object for
         #: running external commands.
-        self.cmd = Executor()
+        self.cmd = Executor(timeout=self.setup['command_timeout'])
 
         #: A list of entries that have been modified by this tool
         self.modified = []