More pychecker. More comments.

10 files changed, 48 insertions, 75 deletions

diff --git a/cobbler/action_check.py b/cobbler/action_check.py
index c310fc6..05718b3 100644
--- a/cobbler/action_check.py
+++ b/cobbler/action_check.py
@@ -6,9 +6,7 @@ Michael DeHaan <mdehaan@redhat.com>
 """
 
 import os
-import sys
 import re
-import config
 
 from msg import *
 
diff --git a/cobbler/action_sync.py b/cobbler/action_sync.py
index e804586..fede994 100644
--- a/cobbler/action_sync.py
+++ b/cobbler/action_sync.py
@@ -5,15 +5,10 @@ This is the code behind 'cobbler sync'.
 Michael DeHaan <mdehaan@redhat.com>
 """
 
-import config
-
 import os
-import sys
 import traceback
-import re
 import shutil
 import syck
-import weakref
 
 import utils
 from msg import *
@@ -41,6 +36,7 @@ class BootSync:
         Syncs the current configuration file with the config tree.
         Using the Check().run_ functions previously is recommended
         """
+        self.verbose = verbose
         self.dry_run = dry_run
         try:
             self.copy_pxelinux()
@@ -113,10 +109,10 @@ class BootSync:
             if kernel is None or not os.path.isfile(kernel):
                utils.set_error("Kernel for distro (%s) cannot be found and needs to be fixed: %s" % (d.name, d.kernel))
                print utils.last_error()
-               raise Exception("error")
+               raise Exception, "error"
             if initrd is None or not os.path.isfile(initrd):
-               utils.last_error("Initrd for distro (%s) cannot be found and needs to be fixed: %s" % (d.name, d.initrd))
-               raise Exception("error")
+               utils.set_error("Initrd for distro (%s) cannot be found and needs to be fixed: %s" % (d.name, d.initrd))
+               raise Exception, "error"
             b_kernel = os.path.basename(kernel)
             b_initrd = os.path.basename(initrd)
             self.copyfile(kernel, os.path.join(distro_dir, b_kernel))
@@ -146,7 +142,7 @@ class BootSync:
                   self.copyfile(g.kickstart, dest)
               except:
                   utils.set_error("err_kickstart2")
-                  raise "error"
+                  raise Exception, "error"
 
     def build_trees(self):
         """
@@ -188,11 +184,11 @@ class BootSync:
             profile = self.profiles.find(system.profile)
             if profile is None:
                 utils.set_error("orphan_profile2")
-                raise "error"
+                raise Exception, "error"
             distro = self.distros.find(profile.distro)
             if distro is None:
                 utils.set_error("orphan_system2")
-                raise "error"
+                raise Exception, "error"
             f1 = self.get_pxelinux_filename(system.name)
             f2 = os.path.join(self.settings.tftpboot, "pxelinux.cfg", f1)
             f3 = os.path.join(self.settings.tftpboot, "systems", f1)
@@ -215,7 +211,7 @@ class BootSync:
             return "01-" + "-".join(name.split(":")).lower()
         else:
             utils.set_error(m("err_resolv" % name))
-            raise "error"
+            raise Exception, "error"
 
 
     def write_pxelinux_file(self,filename,system,profile,distro):
diff --git a/cobbler/api.py b/cobbler/api.py
index 37a5934..d3ea377 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -1,6 +1,11 @@
 """
 python API module for Cobbler
 see source for cobbler.py, or pydoc, for example usage.
+CLI apps and daemons should import api.py, and no other cobbler code.
+
+All functions return True on success, and generally return False on error.
+Exceptions are *not* extended to escape this class, nor should this class
+need to do any exception handling.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -19,8 +24,6 @@ class BootAPI:
        """
        Constructor...
        """
-       # FIXME: deserializer/serializer error
-       # handling probably not up to par yet
        self.deserialize()
 
 
@@ -28,17 +31,13 @@ class BootAPI:
        """
        Forget about current list of profiles, distros, and systems
        """
-       if utils.app_debug:
-           print "BootAPI::clear"
-       _config.clear()
+       return _config.clear()
 
 
     def systems(self):
        """
        Return the current list of systems
        """
-       if utils.app_debug:
-           print "BootAPI::systems"
        return _config.systems()
 
 
@@ -46,8 +45,6 @@ class BootAPI:
        """
        Return the current list of profiles
        """
-       if utils.app_debug:
-           print "BootAPI::profiles"
        return _config.profiles()
 
 
@@ -55,8 +52,6 @@ class BootAPI:
        """
        Return the current list of distributions
        """
-       if utils.app_debug:
-           print "BootAPI::distros"
        return _config.distros()
 
 
@@ -64,8 +59,6 @@ class BootAPI:
        """
        Return a blank, unconfigured system, unattached to a collection
        """
-       if utils.app_debug:
-           print "BootAPI::new_system"
        return _config.new_system()
 
 
@@ -73,8 +66,6 @@ class BootAPI:
        """
        Create a blank, unconfigured distro, unattached to a collection.
        """
-       if utils.app_debug:
-           print "BootAPI::new_distro"
        return _config.new_distro()
 
 
@@ -82,8 +73,6 @@ class BootAPI:
        """
        Create a blank, unconfigured profile, unattached to a collection
        """
-       if utils.app_debug:
-           print "BootAPI::new_profile"
        return _config.new_profile()
 
     def check(self):
@@ -95,8 +84,6 @@ class BootAPI:
        for human admins, who may, for instance, forget to properly set up
        their TFTP servers for PXE, etc.
        """
-       if utils.app_debug:
-           print "BootAPI::check"
        return action_check.BootCheck(_config).run()
 
 
@@ -107,9 +94,6 @@ class BootAPI:
        /tftpboot.  Any operations done in the API that have not been
        saved with serialize() will NOT be synchronized with this command.
        """
-       if utils.app_debug:
-           print "BootAPI::sync"
-       # config.deserialize(); # neccessary?
        return action_sync.BootSync(_config).sync(dry_run)
 
 
@@ -117,20 +101,17 @@ class BootAPI:
        """
        Save the config file(s) to disk.
        """
-       if utils.app_debug:
-           print "BootAPI::serialize"
-       _config.serialize()
+       return _config.serialize()
 
     def deserialize(self):
        """
        Load the current configuration from config file(s)
        """
-       if utils.app_debug:
-           print "BootAPI::deserialize"
-       _config.deserialize()
+       return _config.deserialize()
 
     def last_error(self):
-       if utils.app_debug:
-           print "BootAPI::last_error"
+       """
+       In the event of failure, what went wrong?
+       """
        return utils.last_error()
 
diff --git a/cobbler/cobbler.py b/cobbler/cobbler.py
index 989b50f..a8de380 100755
--- a/cobbler/cobbler.py
+++ b/cobbler/cobbler.py
@@ -302,7 +302,7 @@ def main():
         sys.exit(1)
     try:
         cli = BootCLI(sys.argv)
-    except Exception, e:
+    except Exception:
         traceback.print_exc()
         sys.exit(1)
     sys.exit(cli.run())
diff --git a/cobbler/collection.py b/cobbler/collection.py
index 027d815..24a2691 100644
--- a/cobbler/collection.py
+++ b/cobbler/collection.py
@@ -36,8 +36,6 @@ class Collection(serializable.Serializable):
         """
         Forget about objects in the collection.
         """
-        if utils.app_debug:
-            print "Collection::clear"
         self.listing = {}
 
     def find(self,name):
@@ -45,8 +43,6 @@ class Collection(serializable.Serializable):
         Return anything named 'name' in the collection, else return None if
         no objects can be found.
         """
-        if utils.app_debug:
-            print "Collection::find(%s)" % name
         if name in self.listing.keys():
             return self.listing[name]
         return None
@@ -56,17 +52,11 @@ class Collection(serializable.Serializable):
         """
         Serialize the collection
         """
-        if utils.app_debug:
-            print "Collection::to_datastruct"
         datastruct = [x.to_datastruct() for x in self.listing.values()]
         return datastruct
 
     def from_datastruct(self,datastruct):
-        if utils.app_debug:
-            print "Collection::from_datastruct(%s)" % datastruct
         if datastruct is None:
-            if utils.app_debug:
-                print "DEBUG: from_datastruct -> None, skipping"
             return
         for seed_data in datastruct:
             item = self.factory_produce(self.config,seed_data)
@@ -79,8 +69,6 @@ class Collection(serializable.Serializable):
         object specified by ref deems itself invalid (and therefore
         won't be added to the collection).
         """
-        if utils.app_debug:
-            print "Collection::add(%s)" % ref
         if ref is None or not ref.is_valid():
             if utils.last_error() is None or utils.last_error() == "":
                 utils.set_error("bad_param")
@@ -95,8 +83,6 @@ class Collection(serializable.Serializable):
         for reading by humans or parsing from scripts.  Actually scripts
         would be better off reading the YAML in the config files directly.
         """
-        if utils.app_debug:
-            print "Collection::printable"
         values = map(lambda(a): a.printable(), sorted(self.listing.values()))
         if len(values) > 0:
            return "\n\n".join(values)
@@ -107,8 +93,6 @@ class Collection(serializable.Serializable):
         """
 	Iterator for the collection.  Allows list comprehensions, etc
 	"""
-        if utils.app_debug:
-            print "Collection::__iter__"
         for a in self.listing.values():
 	    yield a
 
@@ -116,7 +100,6 @@ class Collection(serializable.Serializable):
         """
 	Returns size of the collection
 	"""
-        if utils.app_debug:
-            print "Collection::__len__"
         return len(self.listing.values())
 
+
diff --git a/cobbler/config.py b/cobbler/config.py
index 0627abc..cf259a2 100644
--- a/cobbler/config.py
+++ b/cobbler/config.py
@@ -85,6 +85,7 @@ class Config:
        """
        for x in self._classes:
           x.clear()
+       return True
 
    def file_check(self):
        """
@@ -93,21 +94,28 @@ class Config:
        """
        for x in self._classes:
           if not os.path.exists(x.filename()):
-              serializer.serialize(x)
+              if not serializer.serialize(x):
+                  return False
+       return True
+       
 
    def serialize(self):
        """
        Save the object hierarchy to disk, using the filenames referenced in each object.
        """
        for x in self._classes:
-          serializer.serialize(x)
+          if not serializer.serialize(x):
+              return False
+       return True
 
    def deserialize(self):
        """
        Load the object hierachy from disk, using the filenames referenced in each object.
        """
        for x in self._classes:
-          serializer.deserialize(x)
+          if not serializer.deserialize(x):
+              return False
+       return True
 
 
 
diff --git a/cobbler/item_profile.py b/cobbler/item_profile.py
index 4709479..9fd308a 100644
--- a/cobbler/item_profile.py
+++ b/cobbler/item_profile.py
@@ -134,6 +134,8 @@ class Profile(item.Item):
 	'true'/'false' in all cases, or Python True/False.
 	"""
         # truthiness needs to be True or False, or (lcased) string equivalents
+        # yes, we *do* want to explicitly test against True/False
+        # the string "foosball" is True, and that is not a valid argument for this function
         try:
             if (truthiness == False or truthiness.lower() == 'false'):
                 self.xen_paravirt = False
diff --git a/cobbler/msg.py b/cobbler/msg.py
index ceaa0e5..e617e58 100644
--- a/cobbler/msg.py
+++ b/cobbler/msg.py
@@ -47,7 +47,7 @@ msg_table = {
   "check_ok"        : """
 No setup problems found.
 
-Manual editing of /var/lib/cobbler/settings and dhcpd.conf is suggested to tailor them to your specific configuration.  Your dhcpd.conf has some PXE related information in it, but it's imposible to tell automatically that it's totally correct in a general sense.  We'll leave this up to you.
+Manual editing of /var/lib/cobbler/settings and dhcpd.conf is suggested to tailor them to your specific configuration.  Furthermore, it's important to know that cobbler can't completely understnad what you intend to do with dhcpd.conf, but it looks like there is at least some PXE related information in it.  We'll leave this up to you.
 
 Good luck.
 """,
diff --git a/cobbler/settings.py b/cobbler/settings.py
index 48226a0..e740e13 100644
--- a/cobbler/settings.py
+++ b/cobbler/settings.py
@@ -55,8 +55,6 @@ class Settings(serializable.Serializable):
        return self
 
    def __getattr__(self,name):
-       if utils.app_debug:
-           print "Settings::__getattr__(self,%s)" % name
        if self._attributes.has_key(name):
            return self._attributes[name]
        else:
diff --git a/cobbler/utils.py b/cobbler/utils.py
index 341bef5..c6cc019 100644
--- a/cobbler/utils.py
+++ b/cobbler/utils.py
@@ -12,20 +12,27 @@ import subprocess
 
 import msg
 
-app_debug = False # essentially an app level global, but the only one
-
 _re_kernel = re.compile(r'vmlinuz-(\d+)\.(\d+)\.(\d+)-(.*)')
 _re_initrd = re.compile(r'initrd-(\d+)\.(\d+)\.(\d+)-(.*).img')
-utils_last_error = ""
+_utils_last_error = ""
 
 def last_error():
-    return utils_last_error
+    """
+    Return the last error message set with set_error
+    """
+    return _utils_last_error
 
 def set_error(strmsg):
-    global utils_last_error
-    utils_last_error = msg.m(strmsg)
+    """
+    Set the error message for the last failed operation
+    """
+    global _utils_last_error
+    _utils_last_error = msg.m(strmsg)
 
 def get_host_ip(ip):
+    """
+    Return the IP encoding needed for the TFTP boot tree.
+    """
     handle = subprocess.Popen("/usr/bin/gethostip %s" % ip, shell=True, stdout=subprocess.PIPE)
     out = handle.stdout
     results = out.read()