doc: wrote devel docs for client tool base objects

14 files changed, 390 insertions, 141 deletions

diff --git a/src/lib/Bcfg2/Client/Tools/APK.py b/src/lib/Bcfg2/Client/Tools/APK.py
index 539a0fddb..f23fbb119 100644
--- a/src/lib/Bcfg2/Client/Tools/APK.py
+++ b/src/lib/Bcfg2/Client/Tools/APK.py
@@ -52,11 +52,11 @@ class APK(Bcfg2.Client.Tools.PkgTool):
         entry.set('current_exists', 'false')
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """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.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/Blast.py b/src/lib/Bcfg2/Client/Tools/Blast.py
index 5d5e74ab2..2627c42fe 100644
--- a/src/lib/Bcfg2/Client/Tools/Blast.py
+++ b/src/lib/Bcfg2/Client/Tools/Blast.py
@@ -11,7 +11,7 @@ class Blast(Bcfg2.Client.Tools.SYSV.SYSV):
     name = 'Blast'
     __execs__ = ['/opt/csw/bin/pkg-get', "/usr/bin/pkginfo"]
     __handles__ = [('Package', 'blast')]
-    __ireq__ = {'Package': ['name', 'version', 'bname']}
+    __req__ = {'Package': ['name', 'version', 'bname']}
 
     def __init__(self, logger, setup, config):
         # dont use the sysv constructor
@@ -27,6 +27,6 @@ class Blast(Bcfg2.Client.Tools.SYSV.SYSV):
     # Install comes from Bcfg2.Client.Tools.PkgTool
     # Extra comes from Bcfg2.Client.Tools.Tool
     # Remove comes from Bcfg2.Client.Tools.SYSV
-    def FindExtraPackages(self):
+    def FindExtra(self):
         """Pass through to null FindExtra call."""
         return []
diff --git a/src/lib/Bcfg2/Client/Tools/Encap.py b/src/lib/Bcfg2/Client/Tools/Encap.py
index b5057786f..ca6fc7653 100644
--- a/src/lib/Bcfg2/Client/Tools/Encap.py
+++ b/src/lib/Bcfg2/Client/Tools/Encap.py
@@ -42,10 +42,10 @@ class Encap(Bcfg2.Client.Tools.PkgTool):
             return True
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """Deal with extra configuration detected."""
         names = " ".join([pkg.get('name') for pkg in packages])
         self.logger.info("Removing packages: %s" % (names))
         self.cmd.run("/usr/local/bin/epkg -l -q -r %s" % (names))
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/MacPorts.py b/src/lib/Bcfg2/Client/Tools/MacPorts.py
index 22f06ce9a..be441135e 100644
--- a/src/lib/Bcfg2/Client/Tools/MacPorts.py
+++ b/src/lib/Bcfg2/Client/Tools/MacPorts.py
@@ -61,11 +61,11 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
         entry.set('current_exists', 'false')
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """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" % \
                      " ".join(names))
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/OpenCSW.py b/src/lib/Bcfg2/Client/Tools/OpenCSW.py
index 6aafe316f..60e362e64 100644
--- a/src/lib/Bcfg2/Client/Tools/OpenCSW.py
+++ b/src/lib/Bcfg2/Client/Tools/OpenCSW.py
@@ -12,7 +12,7 @@ class OpenCSW(Bcfg2.Client.Tools.SYSV.SYSV):
     name = 'OpenCSW'
     __execs__ = ['/opt/csw/bin/pkgutil', "/usr/bin/pkginfo"]
     __handles__ = [('Package', 'opencsw')]
-    __ireq__ = {'Package': ['name', 'version', 'bname']}
+    __req__ = {'Package': ['name', 'version', 'bname']}
 
     def __init__(self, logger, setup, config):
         # dont use the sysv constructor
@@ -28,6 +28,6 @@ class OpenCSW(Bcfg2.Client.Tools.SYSV.SYSV):
     # Install comes from Bcfg2.Client.Tools.PkgTool
     # Extra comes from Bcfg2.Client.Tools.Tool
     # Remove comes from Bcfg2.Client.Tools.SYSV
-    def FindExtraPackages(self):
+    def FindExtra(self):
         """Pass through to null FindExtra call."""
         return []
diff --git a/src/lib/Bcfg2/Client/Tools/Pacman.py b/src/lib/Bcfg2/Client/Tools/Pacman.py
index 75dd62ede..9c14a3de6 100644
--- a/src/lib/Bcfg2/Client/Tools/Pacman.py
+++ b/src/lib/Bcfg2/Client/Tools/Pacman.py
@@ -58,14 +58,14 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
         self.logger.info("attribname: %s" % (entry.attrib['name']))
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         '''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.pkgtool, " ".join(names)))
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
 
     def Install(self, packages, states):
         '''
diff --git a/src/lib/Bcfg2/Client/Tools/Portage.py b/src/lib/Bcfg2/Client/Tools/Portage.py
index 36d48b8d3..f6332c575 100644
--- a/src/lib/Bcfg2/Client/Tools/Portage.py
+++ b/src/lib/Bcfg2/Client/Tools/Portage.py
@@ -92,7 +92,7 @@ class Portage(Bcfg2.Client.Tools.PkgTool):
         # Something got skipped. Indicates a bug
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """Deal with extra configuration detected."""
         pkgnames = " ".join([pkg.get('name') for pkg in packages])
         if len(packages) > 0:
@@ -101,4 +101,4 @@ class Portage(Bcfg2.Client.Tools.PkgTool):
             self.cmd.run("emerge --unmerge --quiet %s" %
                          " ".join(pkgnames.split(' ')))
             self.RefreshPackages()
-            self.extra = self.FindExtraPackages()
+            self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/RPM.py b/src/lib/Bcfg2/Client/Tools/RPM.py
index 1e54dc449..3d93149ff 100644
--- a/src/lib/Bcfg2/Client/Tools/RPM.py
+++ b/src/lib/Bcfg2/Client/Tools/RPM.py
@@ -408,15 +408,15 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
             return False
         return True
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """
            Remove specified entries.
 
            packages is a list of Package Entries with Instances generated
-           by FindExtraPackages().
+           by FindExtra().
 
         """
-        self.logger.debug('Running RPM.RemovePackages()')
+        self.logger.debug('Running RPM.Remove()')
 
         pkgspec_list = []
         for pkg in packages:
@@ -478,7 +478,7 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
                     self.modified.append(pkg)
 
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
 
     def FixInstance(self, instance, inst_status):
         """
@@ -570,7 +570,7 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
             if (self.setup.get('remove') == 'all' or \
                 self.setup.get('remove') == 'packages') and\
                 not self.setup.get('dryrun'):
-                self.RemovePackages(self.extra_instances)
+                self.Remove(self.extra_instances)
             else:
                 self.logger.info("The following extra package instances will be removed by the '-r' option:")
                 for pkg in self.extra_instances:
@@ -843,7 +843,7 @@ class RPM(Bcfg2.Client.Tools.PkgTool):
                             return False
         return True
 
-    def FindExtraPackages(self):
+    def FindExtra(self):
         """Find extra packages."""
         packages = [entry.get('name') for entry in self.getSupportedEntries()]
         extras = []
diff --git a/src/lib/Bcfg2/Client/Tools/SMF.py b/src/lib/Bcfg2/Client/Tools/SMF.py
index 43e4b3bf5..4409b40f3 100644
--- a/src/lib/Bcfg2/Client/Tools/SMF.py
+++ b/src/lib/Bcfg2/Client/Tools/SMF.py
@@ -10,9 +10,7 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
     """Support for Solaris SMF Services."""
     __handles__ = [('Service', 'smf')]
     __execs__ = ['/usr/sbin/svcadm', '/usr/bin/svcs']
-    name = 'SMF'
-    __req__ = {'Service': ['name', 'status']}
-    __ireq__ = {'Service': ['name', 'status', 'FMRI']}
+    __req__ = {'Service': ['name', 'status', 'FMRI']}
 
     def get_svc_command(self, service, action):
         if service.get('type') == 'lrc':
@@ -128,6 +126,6 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
 
         for svc in self.getSupportedEntries():
             if svc.get("FMRI") in allsrv:
-                allsrv.remove(svc.get('FMRI')) 
+                allsrv.remove(svc.get('FMRI'))
         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 eb4a13dfb..9b84a14cc 100644
--- a/src/lib/Bcfg2/Client/Tools/SYSV.py
+++ b/src/lib/Bcfg2/Client/Tools/SYSV.py
@@ -95,11 +95,11 @@ class SYSV(Bcfg2.Client.Tools.PkgTool):
                     return True
         return False
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """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.noaskname, names))
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/YUM.py b/src/lib/Bcfg2/Client/Tools/YUM.py
index 928aba1e1..c8414b4b2 100644
--- a/src/lib/Bcfg2/Client/Tools/YUM.py
+++ b/src/lib/Bcfg2/Client/Tools/YUM.py
@@ -125,7 +125,6 @@ class YUM(Bcfg2.Client.Tools.PkgTool):
 
     __req__ = {'Package': ['name'],
                'Path': ['type']}
-    __ireq__ = {'Package': ['name']}
 
     conflicts = ['YUM24', 'RPM', 'RPMng', 'YUMng']
 
@@ -675,7 +674,7 @@ class YUM(Bcfg2.Client.Tools.PkgTool):
         else:
             return extra_entry
 
-    def FindExtraPackages(self):
+    def FindExtra(self):
         """Find extra packages."""
         packages = [e.get('name') for e in self.getSupportedEntries()]
         extras = []
@@ -830,7 +829,7 @@ class YUM(Bcfg2.Client.Tools.PkgTool):
         if self.extra_instances is not None and len(self.extra_instances) > 0:
             if (self.setup.get('remove') == 'all' or \
                 self.setup.get('remove') == 'packages'):
-                self.RemovePackages(self.extra_instances)
+                self.Remove(self.extra_instances)
             else:
                 self.logger.info("The following extra package instances will "
                                  "be removed by the '-r' option:")
@@ -944,14 +943,14 @@ class YUM(Bcfg2.Client.Tools.PkgTool):
         for entry in [ent for ent in packages if states[ent]]:
             self.modified.append(entry)
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """
            Remove specified entries.
 
            packages is a list of Package Entries with Instances generated
-           by FindExtraPackages().
+           by FindExtra().
         """
-        self.logger.debug('Running Yum.RemovePackages()')
+        self.logger.debug('Running Yum.Remove()')
 
         for pkg in packages:
             for inst in pkg:
@@ -966,7 +965,7 @@ class YUM(Bcfg2.Client.Tools.PkgTool):
                                       nevra['release']))
 
         self._runYumTransaction()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
 
     def VerifyPath(self, entry, _):  # pylint: disable=W0613
         """Do nothing here since we only verify Path type=ignore"""
diff --git a/src/lib/Bcfg2/Client/Tools/YUM24.py b/src/lib/Bcfg2/Client/Tools/YUM24.py
index 9107d7a0d..cd25ecf37 100644
--- a/src/lib/Bcfg2/Client/Tools/YUM24.py
+++ b/src/lib/Bcfg2/Client/Tools/YUM24.py
@@ -197,7 +197,7 @@ class YUM24(RPM):
         if len(self.extra_instances) > 0:
             if (self.setup.get('remove') == 'all' or \
                 self.setup.get('remove') == 'packages'):
-                self.RemovePackages(self.extra_instances)
+                self.Remove(self.extra_instances)
             else:
                 self.logger.info("The following extra package instances will be removed by the '-r' option:")
                 for pkg in self.extra_instances:
@@ -332,14 +332,14 @@ class YUM24(RPM):
         for entry in [ent for ent in packages if states[ent]]:
             self.modified.append(entry)
 
-    def RemovePackages(self, packages):
+    def Remove(self, packages):
         """
            Remove specified entries.
 
            packages is a list of Package Entries with Instances generated
-           by FindExtraPackages().
+           by FindExtra().
         """
-        self.logger.debug('Running YUM24.RemovePackages()')
+        self.logger.debug('Running YUM24.Remove()')
 
         if self.autodep:
             pkgtool = "/usr/bin/yum -d0 -y erase %s"
@@ -401,4 +401,4 @@ class YUM24(RPM):
                     self.modified.append(pkg)
 
         self.RefreshPackages()
-        self.extra = self.FindExtraPackages()
+        self.extra = self.FindExtra()
diff --git a/src/lib/Bcfg2/Client/Tools/__init__.py b/src/lib/Bcfg2/Client/Tools/__init__.py
index d5f55759f..a4592b52e 100644
--- a/src/lib/Bcfg2/Client/Tools/__init__.py
+++ b/src/lib/Bcfg2/Client/Tools/__init__.py
@@ -11,30 +11,48 @@ from Bcfg2.Compat import input, walk_packages  # pylint: disable=W0622
 __all__ = [m[1] for m in walk_packages(path=__path__)]
 
 # pylint: disable=C0103
+#: All available tools
 drivers = [item for item in __all__ if item not in ['rpmtools']]
+
+#: The default set of tools that will be used if "drivers" is not set
+#: in bcfg2.conf
 default = drivers[:]
 # pylint: enable=C0103
 
 
 class ToolInstantiationError(Exception):
-    """This error is called if the toolset cannot be instantiated."""
+    """ This error is raised if the toolset cannot be instantiated. """
     pass
 
 
 class Executor:
-    """This class runs stuff for us"""
+    """ This class runs shell commands. """
 
     def __init__(self, logger):
+        """
+        :param logger: The logger to use to produce debug logging
+        :type logger: logging.Logger
+        """
         self.logger = logger
 
     def run(self, command):
-        """Run a command in a pipe dealing with stdout buffer overloads."""
+        """ Run a command inside a shell.
+
+        :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]
-        for line in output.splitlines():
+        output = proc.communicate()[0].splitlines
+        for line in output:
             self.logger.debug('< %s' % line)
-        return (proc.returncode, output.splitlines())
+        return (proc.wait(), output)
 
 
 class ClassName(object):
@@ -49,31 +67,94 @@ class ClassName(object):
         return owner.__name__
 
 
-# pylint: disable=W0702
-# in the base tool class we frequently want to catch all exceptions,
-# regardless of type, so disable the pylint rule that catches that.
 class Tool(object):
-    """ All tools subclass this. It defines all interfaces that need
-    to be defined. """
+    """ The base tool class.  All tools subclass this.
+
+    .. private-include: _entry_is_complete
+    .. autoattribute:: Bcfg2.Client.Tools.Tool.__execs__
+    .. autoattribute:: Bcfg2.Client.Tools.Tool.__handles__
+    .. autoattribute:: Bcfg2.Client.Tools.Tool.__req__
+    .. autoattribute:: Bcfg2.Client.Tools.Tool.__important__
+    """
+
+    #: The name of the tool.  By default this uses
+    #: :class:`Bcfg2.Client.Tools.ClassName` to ensure that it is the
+    #: same as the name of the class.
     name = ClassName()
+
+    #: Full paths to all executables the tool uses.  When the tool is
+    #: instantiated it will check to ensure that all of these files
+    #: exist and are executable.
     __execs__ = []
+
+    #: A list of 2-tuples of entries handled by this tool.  Each
+    #: 2-tuple should contain ``(<tag>, <type>)``, where ``<type>`` is
+    #: the ``type`` attribute of the entry.  If this tool handles
+    #: entries with no ``type`` attribute, specify None.
     __handles__ = []
+
+    #: A dict that describes the required attributes for entries
+    #: handled by this tool.  The keys are the names of tags.  The
+    #: values may either be lists of attribute names (if the same
+    #: attributes are required by all tags of that name), or dicts
+    #: whose keys are the ``type`` attribute and whose values are
+    #: lists of attributes required by tags with that ``type``
+    #: attribute.  In that case, the ``type`` attribute will also be
+    #: required.
     __req__ = {}
+
+    #: A list of entry names that will be treated as important and
+    #: installed before other entries.
     __important__ = []
+
+    #: This tool is deprecated, and a warning will be produced if it
+    #: is used.
     deprecated = False
+
+    #: This tool is experimental, and a warning will be produced if it
+    #: is used.
     experimental = False
 
+    #: List of other tools (by name) that this tool conflicts with.
+    #: If any of the listed tools are loaded, they will be removed at
+    #: runtime with a warning.
+    conflicts = []
+
     def __init__(self, logger, setup, config):
+        """
+        :param logger: Logger that will be used for logging by this tool
+        :type logger: logging.Logger
+        :param setup: The option set Bcfg2 was invoked with
+        :type setup: Bcfg2.Options.OptionParser
+        :param config: The XML configuration for this client
+        :type config: lxml.etree._Element
+        :raises: :exc:`Bcfg2.Client.Tools.ToolInstantiationError`
+        """
+        #: A :class:`Bcfg2.Options.OptionParser` object describing the
+        #: option set Bcfg2 was invoked with
         self.setup = setup
+
+        #: A :class:`logging.Logger` object that will be used by this
+        #: tool for logging
         self.logger = logger
-        if not hasattr(self, '__ireq__'):
-            self.__ireq__ = self.__req__
+
+        #: The XML configuration for this client
         self.config = config
+
+        #: An :class:`Bcfg2.Client.Tools.Executor` object for
+        #: running external commands.
         self.cmd = Executor(logger)
+
+        #: A list of entries that have been modified by this tool
         self.modified = []
+
+        #: A list of extra entries that are not listed in the
+        #: configuration
         self.extra = []
-        self.__important__ = []
+
+        #: A list of all entries handled by this tool
         self.handled = []
+
         for struct in config:
             for entry in struct:
                 if (entry.tag == 'Path' and
@@ -85,25 +166,59 @@ class Tool(object):
             try:
                 mode = stat.S_IMODE(os.stat(filename)[stat.ST_MODE])
                 if mode & stat.S_IEXEC != stat.S_IEXEC:
-                    self.logger.debug("%s: %s not executable" %
-                                      (self.name, filename))
-                    raise ToolInstantiationError
+                    raise ToolInstantiationError("%s: %s not executable" %
+                                                 (self.name, filename))
             except OSError:
-                raise ToolInstantiationError
+                raise ToolInstantiationError(sys.exc_info()[1])
             except:
-                self.logger.debug("%s failed" % filename, exc_info=1)
-                raise ToolInstantiationError
+                raise ToolInstantiationError("%s: Failed to stat %s" %
+                                             (self.name, filename),
+                                             exc_info=1)
 
     def BundleUpdated(self, bundle, states):  # pylint: disable=W0613
-        """This callback is used when bundle updates occur."""
+        """ Callback that is invoked when a bundle has been updated.
+
+        :param bundle: The bundle that has been updated
+        :type bundle: lxml.etree._Element
+        :param states: The :attr:`Bcfg2.Client.Frame.Frame.states` dict
+        :type states: dict
+        :returns: None """
         return
 
     def BundleNotUpdated(self, bundle, states):  # pylint: disable=W0613
-        """This callback is used when a bundle is not updated."""
+        """ Callback that is invoked when a bundle has been updated.
+
+        :param bundle: The bundle that has been updated
+        :type bundle: lxml.etree._Element
+        :param states: The :attr:`Bcfg2.Client.Frame.Frame.states` dict
+        :type states: dict
+        :returns: None """
         return
 
     def Inventory(self, states, structures=None):
-        """Dispatch verify calls to underlying methods."""
+        """ Take an inventory of the system as it exists.  This
+        involves two steps:
+
+        * Call the appropriate entry-specific Verify method for each
+          entry this tool verifies;
+        * Call :func:`Bcfg2.Client.Tools.Tool.FindExtra` to populate
+          :attr:`Bcfg2.Client.Tools.Tool.extra` with extra entries.
+
+        This implementation of
+        :func:`Bcfg2.Client.Tools.Tool.Inventory` calls a
+        ``Verify<tag>`` method to verify each entry, where ``<tag>``
+        is the entry tag.  E.g., a Path entry would be verified by
+        calling :func:`VerifyPath`.
+
+        :param states: The :attr:`Bcfg2.Client.Frame.Frame.states` dict
+        :type states: dict
+        :param structures: The list of structures (i.e., bundles) to
+                           get entries from.  If this is not given,
+                           all children of
+                           :attr:`Bcfg2.Client.Tools.Tool.config` will
+                           be used.
+        :type structures: list of lxml.etree._Element
+        :returns: None """
         if not structures:
             structures = self.config.getchildren()
         mods = self.buildModlist()
@@ -113,31 +228,57 @@ class Tool(object):
                     try:
                         func = getattr(self, "Verify%s" % entry.tag)
                         states[entry] = func(entry, mods)
+                    except AttributeError:
+                        self.logger.error("%s: Cannot verify %s entries" %
+                                          (self.name, entry.tag))
                     except:
-                        self.logger.error("Unexpected failure of verification "
-                                          "method for entry type %s" %
-                                          entry.tag, exc_info=1)
+                        self.logger.error("%s: Unexpected failure verifying %s"
+                                          % (self.name,
+                                             self.primarykey(entry)),
+                                          exc_info=1)
         self.extra = self.FindExtra()
 
     def Install(self, entries, states):
-        """Install all entries in sublist."""
+        """ Install entries.  'Install' in this sense means either
+        initially install, or update as necessary to match the
+        specification.
+
+        This implementation of :func:`Bcfg2.Client.Tools.Tool.Install`
+        calls a ``Install<tag>`` method to install each entry, where
+        ``<tag>`` is the entry tag.  E.g., a Path entry would be
+        installed by calling :func:`InstallPath`.
+
+        :param entries: The entries to install
+        :type entries: list of lxml.etree._Element
+        :param states: The :attr:`Bcfg2.Client.Frame.Frame.states` dict
+        :type states: dict
+        :returns: None """
         for entry in entries:
             try:
                 func = getattr(self, "Install%s" % (entry.tag))
                 states[entry] = func(entry)
                 if states[entry]:
                     self.modified.append(entry)
+            except AttributeError:
+                self.logger.error("%s: Cannot install %s entries" %
+                                  (self.name, entry.tag))
             except:
-                self.logger.error("Unexpected failure of install method for "
-                                  "entry type %s" % entry.tag,
+                self.logger.error("%s: Unexpected failure installing %s" %
+                                  (self.name, self.primarykey(entry)),
                                   exc_info=1)
 
     def Remove(self, entries):
-        """Remove specified extra entries"""
+        """ Remove specified extra entries.
+
+        :param entries: The entries to remove
+        :type entries: list of lxml.etree._Element
+        :returns: None """
         pass
 
     def getSupportedEntries(self):
-        """Return a list of supported entries."""
+        """ Get all entries that are handled by this tool.
+
+        :returns: list of lxml.etree._Element """
         rv = []
         for struct in self.config.getchildren():
             rv.extend([entry for entry in struct.getchildren()
@@ -145,25 +286,33 @@ class Tool(object):
         return rv
 
     def handlesEntry(self, entry):
-        """Return if entry is handled by this tool."""
+        """ Return True if the entry is handled by this tool.
+
+        :param entry: Determine if this entry is handled.
+        :type entry: lxml.etree._Element
+        :returns: bool
+        """
         return (entry.tag, entry.get('type')) in self.__handles__
 
     def buildModlist(self):
-        """ Build a list of potentially modified POSIX paths for this
-        entry """
+        """ Build a list of all Path entries in the configuration.
+        (This can be used to determine which paths might be modified
+        from their original state, useful for verifying packages)
+
+        :returns: list of lxml.etree._Element """
         rv = []
         for struct in self.config.getchildren():
             rv.extend([entry.get('name') for entry in struct.getchildren()
                        if entry.tag == 'Path'])
         return rv
 
-    def gatherCurrentData(self, entry):
-        """Default implementation of the information gathering routines."""
-        pass
-
     def missing_attrs(self, entry):
-        """ Return a list of attributes that were expected on entry
-        but not found """
+        """ Return a list of attributes that were expected on an entry
+        (from :attr:`Bcfg2.Client.Tools.Tool.__req__`), but not found.
+
+        :param entry: The entry to find missing attributes on
+        :type entry: lxml.etree._Element
+        :returns: list of strings """
         required = self.__req__[entry.tag]
         if isinstance(required, dict):
             required = ["type"]
@@ -176,80 +325,143 @@ class Tool(object):
                 if attr not in entry.attrib or not entry.attrib[attr]]
 
     def canVerify(self, entry):
-        """Test if entry has enough information to be verified."""
-        if not self.handlesEntry(entry):
-            return False
+        """ Test if entry can be verified by calling
+        :func:`Bcfg2.Client.Tools.Tool._entry_is_complete`.
 
-        if 'failure' in entry.attrib:
-            self.logger.error("Entry %s:%s reports bind failure: %s" %
-                              (entry.tag, entry.get('name'),
-                               entry.get('failure')))
-            return False
-
-        missing = self.missing_attrs(entry)
-        if missing:
-            self.logger.error("Cannot verify entry %s:%s due to missing "
-                              "required attribute(s): %s" %
-                              (entry.tag, entry.get('name'),
-                               ", ".join(missing)))
-            try:
-                self.gatherCurrentData(entry)
-            except:
-                self.logger.error("Unexpected error in gatherCurrentData",
-                                  exc_info=1)
-            return False
-        return True
+        :param entry: The entry to evaluate
+        :type entry: lxml.etree._Element
+        :returns: bool - True if the entry can be verified, False
+                  otherwise.
+        """
+        return self._entry_is_complete(entry, action="verify")
 
     def FindExtra(self):
-        """Return a list of extra entries."""
+        """ Return a list of extra entries, i.e., entries that exist
+        on the client but are not in the configuration.
+
+        :returns: list of lxml.etree._Element """
         return []
 
     def primarykey(self, entry):
-        """ return a string that should be unique amongst all entries
-        in the specification """
+        """ Return a string that describes the entry uniquely amongst
+        all entries in the configuration.
+
+        :param entry: The entry to describe
+        :type entry: lxml.etree._Element
+        :returns: string """
         return "%s:%s" % (entry.tag, entry.get("name"))
 
     def canInstall(self, entry):
-        """Test if entry has enough information to be installed."""
+        """ Test if entry can be installed by calling
+        :func:`Bcfg2.Client.Tools.Tool._entry_is_complete`.
+
+        :param entry: The entry to evaluate
+        :type entry: lxml.etree._Element
+        :returns: bool - True if the entry can be installed, False
+                  otherwise.
+        """
+        return self._entry_is_complete(entry, action="install")
+
+    def _entry_is_complete(self, entry, action=None):
+        """ Test if the entry is complete.  This involves three
+        things:
+
+        * The entry is handled by this tool (as reported by
+          :func:`Bcfg2.Client.Tools.Tool.handlesEntry`;
+        * The entry does not report a bind failure;
+        * The entry is not missing any attributes (as reported by
+          :func:`Bcfg2.Client.Tools.Tool.missing_attrs`).
+
+        :param entry: The entry to evaluate
+        :type entry: lxml.etree._Element
+        :param action: The action being performed on the entry (e.g.,
+                      "install", "verify").  This is used to produce
+                      error messages; if not provided, generic error
+                      messages will be used.
+        :type action: string
+        :returns: bool - True if the entry can be verified, False
+                  otherwise.
+        """
         if not self.handlesEntry(entry):
             return False
 
         if 'failure' in entry.attrib:
-            self.logger.error("Cannot install entry %s:%s with bind failure" %
-                              (entry.tag, entry.get('name')))
+            if action is None:
+                msg = "%s: %s reports bind failure"
+            else:
+                msg = "%%s: Cannot %s entry %%s with bind failure" % action
+            self.logger.error(msg % (self.name, self.primarykey(entry)))
             return False
 
         missing = self.missing_attrs(entry)
         if missing:
-            self.logger.error("Incomplete information for entry %s:%s; cannot "
-                              "install due to absence of attribute(s): %s" %
-                              (entry.tag, entry.get('name'),
-                               ", ".join(missing)))
+            if action is None:
+                msg = "%s: %s is missing required attribute(s): %s"
+            else:
+                msg = "%%s: Cannot %s %%s due to missing required " + \
+                    "attribute(s): %%s" % action
+            self.logger.error(msg % (self.name, self.primarykey(entry),
+                                     ", ".join(missing)))
             return False
         return True
-# pylint: enable=W0702
 
 
 class PkgTool(Tool):
     """ PkgTool provides a one-pass install with fallback for use with
-    packaging systems """
+    packaging systems.  PkgTool makes a number of assumptions that may
+    need to be overridden by a subclass.  For instance, it assumes
+    that packages are installed by a shell command; that only one
+    version of a given package can be installed; etc.  Nonetheless, it
+    offers a strong base for writing simple package tools. """
+
+    #: A tuple describing the format of the command to run to install
+    #: a single package.  The first element of the tuple is a string
+    #: giving the format of the command, with a single '%s' for the
+    #: name of the package or packages to be installed.  The second
+    #: element is a tuple whose first element is the format of the
+    #: name of the package, and whose second element is a list whose
+    #: members are the names of attributes that will be used when
+    #: formatting the package name format string.
     pkgtool = ('echo %s', ('%s', ['name']))
+
+    #: The ``type`` attribute of Packages handled by this tool.
     pkgtype = 'echo'
 
     def __init__(self, logger, setup, config):
         Tool.__init__(self, logger, setup, config)
+
+        #: A dict of installed packages; the keys should be package
+        #: names and the values should be simple strings giving the
+        #: installed version.
         self.installed = {}
         self.RefreshPackages()
-        self.Remove = self.RemovePackages  # pylint: disable=C0103
-        self.FindExtra = self.FindExtraPackages  # pylint: disable=C0103
 
-    def VerifyPackage(self, dummy, _):
-        """Dummy verification method"""
-        return False
+    def VerifyPackage(self, entry, modlist):
+        """ Verify the given Package entry.
+
+        :param entry: The Package entry to verify
+        :type entry: lxml.etree._Element
+        :param modlist: A list of all Path entries in the
+                        configuration, which may be considered when
+                        verifying a package.  For instance, a package
+                        should verify successfully if paths in
+                        ``modlist`` have been modified outside the
+                        package.
+        :type modlist: list of strings
+        :returns: bool - True if the package verifies, false otherwise.
+        """
+        raise NotImplementedError
 
     def Install(self, packages, states):
-        """ Run a one-pass install, followed by single pkg installs in
-        case of failure. """
+        """ Run a one-pass install where all required packages are
+        installed with a single command, followed by single package
+        installs in case of failure.
+
+        :param entries: The entries to install
+        :type entries: list of lxml.etree._Element
+        :param states: The :attr:`Bcfg2.Client.Frame.Frame.states` dict
+        :type states: dict
+        :returns: None """
         self.logger.info("Trying single pass package install for pkgtype %s" %
                          self.pkgtype)
 
@@ -301,63 +513,94 @@ class PkgTool(Tool):
             self.modified.append(entry)
 
     def RefreshPackages(self):
-        """Dummy state refresh method."""
-        pass
+        """ Refresh the internal representation of the package
+        database (:attr:`Bcfg2.Client.Tools.PkgTool.installed`).
 
-    def RemovePackages(self, packages):
-        """Dummy implementation of package removal method."""
-        pass
+        :returns: None"""
+        raise NotImplementedError
 
-    def FindExtraPackages(self):
-        """Find extra packages."""
+    def FindExtra(self):
         packages = [entry.get('name') for entry in self.getSupportedEntries()]
         extras = [data for data in list(self.installed.items())
                   if data[0] not in packages]
         return [Bcfg2.Client.XML.Element('Package', name=name,
                                          type=self.pkgtype, version=version)
                 for (name, version) in extras]
+    FindExtra.__doc__ = Tool.FindExtra.__doc__
 
 
 class SvcTool(Tool):
-    """This class defines basic Service behavior"""
+    """ Base class for tools that handle Service entries """
 
     def __init__(self, logger, setup, config):
         Tool.__init__(self, logger, setup, config)
+        #: List of services that have been restarted
         self.restarted = []
+    __init__.__doc__ = Tool.__init__.__doc__
 
     def get_svc_command(self, service, action):
-        """Return the basename of the command used to start/stop services."""
+        """ Return a command that can be run to start or stop a service.
+
+        :param service: The service entry to modify
+        :type service: lxml.etree._Element
+        :param action: The action to take (e.g., "stop", "start")
+        :type action: string
+        :returns: string - The command to run
+        """
         return '/etc/init.d/%s %s' % (service.get('name'), action)
 
     def start_service(self, service):
-        """ Start a service """
+        """ Start a service.
+
+        :param service: The service entry to modify
+        :type service: lxml.etree._Element
+        :returns: tuple - The return value from
+                  :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]
 
     def stop_service(self, service):
-        """ Stop a service """
+        """ Stop a service.
+
+        :param service: The service entry to modify
+        :type service: lxml.etree._Element
+        :returns: tuple - The return value from
+                  :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]
 
     def restart_service(self, service):
-        """ Restart a service """
+        """ Restart a service.
+
+        :param service: The service entry to modify
+        :type service: lxml.etree._Element
+        :returns: tuple - The return value from
+                  :class:`Bcfg2.Client.Tools.Executor.run`
+        """
         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]
 
     def check_service(self, service):
-        """ Get the status of a service """
+        """ Check the status a service.
+
+        :param service: The service entry to modify
+        :type service: lxml.etree._Element
+        :returns: bool - True if the status command returned 0, False
+                  otherwise
+        """
         return self.cmd.run(self.get_svc_command(service, 'status'))[0] == 0
 
     def Remove(self, services):
-        """ Dummy implementation of service removal method """
         if self.setup['servicemode'] != 'disabled':
             for entry in services:
                 entry.set("status", "off")
                 self.InstallService(entry)
+    Remove.__doc__ = Tool.Remove.__doc__
 
     def BundleUpdated(self, bundle, states):
-        """The Bundle has been updated."""
         if self.setup['servicemode'] == 'disabled':
             return
 
@@ -391,9 +634,9 @@ class SvcTool(Tool):
             if rv:
                 self.logger.error("Failed to manipulate service %s" %
                                   (entry.get('name')))
+    BundleUpdated.__doc__ = Tool.BundleUpdated.__doc__
 
     def Install(self, entries, states):
-        """Install all entries in sublist."""
         install_entries = []
         for entry in entries:
             if entry.get('install', 'true').lower() == 'false':
@@ -402,7 +645,15 @@ class SvcTool(Tool):
             else:
                 install_entries.append(entry)
         return Tool.Install(self, install_entries, states)
+    Install.__doc__ = Tool.Install.__doc__
 
     def InstallService(self, entry):
-        """ Install a single service entry """
+        """ Install a single service entry.  See
+        :func:`Bcfg2.Client.Tools.Tool.Install`.
+
+        :param entry: The Service entry to install
+        :type entry: lxml.etree._Element
+        :returns: bool - True if installation was successful, False
+                  otherwise
+        """
         raise NotImplementedError
diff --git a/src/lib/Bcfg2/Server/Plugins/Cfg/__init__.py b/src/lib/Bcfg2/Server/Plugins/Cfg/__init__.py
index f8712213e..ff4a1ab14 100644
--- a/src/lib/Bcfg2/Server/Plugins/Cfg/__init__.py
+++ b/src/lib/Bcfg2/Server/Plugins/Cfg/__init__.py
@@ -61,11 +61,12 @@ class CfgBaseFileMatcher(Bcfg2.Server.Plugin.SpecificData):
     #: Cfg handlers are checked in ascending order of priority to see
     #: if they handle a given event.  If this explicit priority is not
     #: set, then
-    #: :class:`Bcfg2.Server.Plugins.Cfg.CfgPlaintextGenerator` would
-    #: match against nearly every other sort of generator file if it
-    #: comes first.  It's not necessary to set ``__priority`` on
-    #: handlers where :attr:`__specific__` is False, since they don't
-    #: have a potentially open-ended regex
+    #: :class:`Bcfg2.Server.Plugins.Cfg.CfgPlaintextGenerator.CfgPlaintextGenerator`
+    #: would match against nearly every other sort of generator file
+    #: if it comes first.  It's not necessary to set ``__priority`` on
+    #: handlers where
+    #: :attr:`Bcfg2.Server.Plugins.Cfg.CfgBaseFileMatcher.__specific__`
+    #: is False, since they don't have a potentially open-ended regex
     __priority__ = 0
 
     #: Flag to indicate a deprecated handler.