Adding exception handling to remove the problem of propogating error codes all the way up the stack. Still not quite super-consistant, but getting there. Util functions still return true/false since they just ask questions, but API functions will throw errors to ensure they are being dealt with. Main CLI class needs to take advantage of this fact and become simpler. Tests are already modified to detect new exceptions with one exception :)

13 files changed, 115 insertions, 109 deletions

diff --git a/cobbler/action_sync.py b/cobbler/action_sync.py
index 6fc3c6b..2626e39 100644
--- a/cobbler/action_sync.py
+++ b/cobbler/action_sync.py
@@ -6,13 +6,12 @@ Michael DeHaan <mdehaan@redhat.com>
 """
 
 import os
-import traceback
 import shutil
 import syck
 
 import utils
 import cobbler_msg
-from cobbler_exception import CobblerException
+import cexceptions
 
 """
 Handles conversion of internal state to the tftpboot tree layout
@@ -39,16 +38,12 @@ class BootSync:
         """
         self.verbose = verbose
         self.dry_run = dry_run
-        try:
-            self.copy_pxelinux()
-            self.clean_trees()
-            self.copy_distros()
-            self.validate_kickstarts()
-            self.configure_httpd()
-            self.build_trees()
-        except:
-            traceback.print_exc()
-            return False
+        self.copy_pxelinux()
+        self.clean_trees()
+        self.copy_distros()
+        self.validate_kickstarts()
+        self.configure_httpd()
+        self.build_trees()
         return True
 
 
@@ -108,9 +103,9 @@ class BootSync:
             kernel = utils.find_kernel(d.kernel) # full path
             initrd = utils.find_initrd(d.initrd) # full path
             if kernel is None or not os.path.isfile(kernel):
-               raise CobblerException("sync_kernel", (d.name, d.kernel))
+               raise cexceptions.CobblerException("sync_kernel", (d.name, d.kernel))
             if initrd is None or not os.path.isfile(initrd):
-               raise CobblerException("sync_initrd", (d.name, d.initrd))
+               raise cexceptions.CobblerException("sync_initrd", (d.name, d.initrd))
             b_kernel = os.path.basename(kernel)
             b_initrd = os.path.basename(initrd)
             self.copyfile(kernel, os.path.join(distro_dir, b_kernel))
@@ -139,7 +134,7 @@ class BootSync:
               try:
                   self.copyfile(g.kickstart, dest)
               except:
-                  raise CobblerException("err_kickstart2")
+                  raise cexceptions.CobblerException("err_kickstart2")
 
     def build_trees(self):
         """
@@ -180,10 +175,10 @@ class BootSync:
             self.sync_log(cobbler_msg.lookup("sync_processing") % system.name)
             profile = self.profiles.find(system.profile)
             if profile is None:
-                raise CobblerException("orphan_profile2")
+                raise cexceptions.CobblerException("orphan_profile2")
             distro = self.distros.find(profile.distro)
             if distro is None:
-                raise CobblerException("orphan_system2")
+                raise cexceptions.CobblerException("orphan_system2")
             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)
@@ -205,7 +200,7 @@ class BootSync:
         elif utils.is_mac(name):
             return "01-" + "-".join(name.split(":")).lower()
         else:
-            raise CobblerException("err_resolv", name)
+            raise cexceptions.CobblerException("err_resolv", name)
 
 
     def write_pxelinux_file(self,filename,system,profile,distro):
@@ -342,7 +337,9 @@ class BootSync:
        """
        if self.verbose:
            if self.dry_run:
-               print cobbler_msg.lookup("dry_run") % message
+               if not message:
+                   message = ""
+               print cobbler_msg.lookup("dryrun") % str(message)
            else:
                print message
 
diff --git a/cobbler/api.py b/cobbler/api.py
index a7dbe8b..d15abb9 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -3,10 +3,6 @@ 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>
 """
 
@@ -14,66 +10,83 @@ import config
 import utils
 import action_sync
 import action_check
-
-_config = config.Config()
+import cexceptions
 
 class BootAPI:
 
 
-    def __init__(self):
+    def __init__(self,catch_exceptions=False):
        """
-       Constructor...
+       The API can be invoked in two ways, depending on how it is constructed.
+       The catch_exceptions mode will cause any API method to return false
+       if any CobblerExceptions were thrown, along with setting 'last_error'.  
+       The other mode just lets the exceptions pass through, and is the way
+       most apps should use the API.  catch_exceptions was added for the test hooks,
+       since they are coded to use True/False.
        """
+       self._config = config.Config()
+       self.catch_exceptions = catch_exceptions
+       self.last_error = ""
        self.deserialize()
-
+       
+
+    def __api_call(self,anonymous):
+       if self.catch_exceptions:
+           try:
+               return anonymous()
+           except cexceptions.CobblerException, cobexc:
+               self.last_error = str(cobexc)
+               return False
+       else:
+           return anonymous()
 
     def clear(self):
        """
        Forget about current list of profiles, distros, and systems
        """
-       return _config.clear()
+       return self.__api_call(lambda: self._config.clear())
 
 
     def systems(self):
        """
        Return the current list of systems
        """
-       return _config.systems()
+       return self.__api_call(lambda: self._config.systems())
 
 
     def profiles(self):
        """
        Return the current list of profiles
        """
-       return _config.profiles()
+       return self.__api_call(lambda: self._config.profiles())
 
 
     def distros(self):
        """
        Return the current list of distributions
        """
-       return _config.distros()
+       return self.__api_call(lambda: self._config.distros())
 
 
     def new_system(self):
        """
        Return a blank, unconfigured system, unattached to a collection
        """
-       return _config.new_system()
+       return self.__api_call(lambda: self._config.new_system())
 
 
     def new_distro(self):
        """
        Create a blank, unconfigured distro, unattached to a collection.
        """
-       return _config.new_distro()
+       return self.__api_call(lambda: self._config.new_distro())
 
 
     def new_profile(self):
        """
        Create a blank, unconfigured profile, unattached to a collection
        """
-       return _config.new_profile()
+       return self.__api_call(lambda: self._config.new_profile())
 
     def check(self):
        """
@@ -84,7 +97,7 @@ class BootAPI:
        for human admins, who may, for instance, forget to properly set up
        their TFTP servers for PXE, etc.
        """
-       return action_check.BootCheck(_config).run()
+       return self.__api_call(lambda: action_check.BootCheck(self._config).run())
 
 
     def sync(self,dry_run=True):
@@ -94,18 +107,18 @@ class BootAPI:
        /tftpboot.  Any operations done in the API that have not been
        saved with serialize() will NOT be synchronized with this command.
        """
-       return action_sync.BootSync(_config).sync(dry_run)
+       return self.__api_call(lambda: action_sync.BootSync(self._config).sync(dry_run))
 
 
     def serialize(self):
        """
        Save the config file(s) to disk.
        """
-       return _config.serialize()
+       return self.__api_call(lambda: self._config.serialize())
 
     def deserialize(self):
        """
        Load the current configuration from config file(s)
        """
-       return _config.deserialize()
+       return self.__api_call(lambda: self._config.deserialize())
 
diff --git a/cobbler/cobbler_exception.py b/cobbler/cexceptions.py
index c6abfce..b0d1c8a 100644
--- a/cobbler/cobbler_exception.py
+++ b/cobbler/cexceptions.py
@@ -1,6 +1,5 @@
 """
-Custom error for fatal cobbler exceptions that come with human readable error messages.
-These can be caught and printed without stack traces.
+Custom exceptions for Cobbler
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -11,6 +10,10 @@ import cobbler_msg
 class CobblerException(exceptions.Exception):
 
    def __init__(self, value, args=[]):
+       """
+       This is a translatable exception.  value is an entry in cobbler_msg's
+       lookup table, args will be used for string substitution, if provided
+       """
        if type(args) == str or type(args) == int:
            args = (args)
        self.value = cobbler_msg.lookup(value) % args
diff --git a/cobbler/cobbler.py b/cobbler/cobbler.py
index 33f2253..464b840 100755
--- a/cobbler/cobbler.py
+++ b/cobbler/cobbler.py
@@ -10,10 +10,9 @@ import os
 import sys
 import api
 import syck
-import traceback
 
 import cobbler_msg
-from cobbler_exception import CobblerException
+import cexceptions
 
 class BootCLI:
 
@@ -76,21 +75,14 @@ class BootCLI:
         """
         Run the command line and return system exit code
         """
-        rc = self.deserialize()
-        if rc:
-            rc = self.curry_args(self.args[1:], self.commands['toplevel'])
-        if not rc:
-            print self.api.last_error()
-            return 1
-        return 0
+        self.deserialize()
+        self.curry_args(self.args[1:], self.commands['toplevel'])
 
     def usage(self,args):
         """
         Print out abbreviated help if user gives bad syntax
         """
-        print cobbler_msg.lookup("usage")
-        return False
-
+        raise cexception.CobblerException("usage")
 
     def system_list(self,args):
         """
@@ -214,24 +206,16 @@ class BootCLI:
                 print cobbler_msg.lookup("bad_arg") % x
                 return False
             if key in input_routines:
-                # --argument is recognized, so run the loader
-                # attached to it in the dispatch table
-                if not input_routines[key](value):
-                   # loader does not like passed value
-                   print cobbler_msg.lookup("reject_arg") % key
-                   return False
+                # run the loader for the argument
+                # it will throw CobblerExceptions on bad args
+                input_routines[key](value)
             else:
                 # --argument is not recognized
                 print cobbler_msg.lookup("weird_arg") % key
                 return False
-        # success thus far, so run the success routine for the set of
-        # arguments.  Configuration will only be written to file if the
-        # final routine succeeds.
-        rc = on_ok()
-        if rc and serialize:
-            return self.serialize()
-        return rc
-
+        # no lethal exceptions, so we can run the finalization routine
+        on_ok()
+        self.serialize()
 
     def curry_args(self, args, commands):
         """
@@ -307,15 +291,14 @@ def main():
     """
     CLI entry point
     """
-
-    # verify syck isn't busted (old syck bindings were)
-    if not hasattr(syck,"dump"):
-        raise CobblerException("needs a more-recent PySyck module")
-
+    
     try:
+        # verify syck isn't busted (old syck bindings were)
+        if not hasattr(syck,"dump"):
+            raise cexceptions.CobblerException("needs a more-recent PySyck module")
         cli = BootCLI(sys.argv)
-    except Exception, exc:
-        print exc
+    except cexceptions.CobblerException, exc:
+        print str(exc)[1:-1]  # remove framing air quotes
         sys.exit(1)
     sys.exit(cli.run())
 
diff --git a/cobbler/cobbler_msg.py b/cobbler/cobbler_msg.py
index 69a5dbd..5fc1002 100644
--- a/cobbler/cobbler_msg.py
+++ b/cobbler/cobbler_msg.py
@@ -45,6 +45,14 @@ _msg_table = {
   "no_kernel"       : "the kernel needs to be a directory containing a kernel, or a full path.  Kernels must be named just 'vmlinuz' or in the form 'vmlinuz-AA.BB.CC-something'",
   "sync_kernel"     : "the kernel (%s) for distro (%s) cannot be found and must be fixed",
   "sync_initrd"     : "the initrd (%s) for distro (%s) cannot be found and must be fixed",
+  "sync_mirror_ks"  : "mirroring local kickstarts...",
+  "sync_buildtree"  : "building trees",
+  "sync_processing" : "processing: %s",
+  "writing"         : "writing file: %s",
+  "mkdir"           : "creating: %s",
+  "dryrun"          : "dry run | %s",
+  "copying"         : "copying file: %s to %s",
+  "removing"        : "removing: %s",
   "no_initrd"       : "the initrd needs to be a directory containing an initrd, or a full path.  Initrds must be named just 'initrd.img' or in the form 'initrd-AA.BB.CC-something.img",
   "check_ok"        : """
 No setup problems found.
diff --git a/cobbler/collection.py b/cobbler/collection.py
index 607b344..9c58295 100644
--- a/cobbler/collection.py
+++ b/cobbler/collection.py
@@ -5,7 +5,7 @@ Michael DeHaan <mdehaan@redhat.com>
 """
 
 import exceptions
-
+import cexceptions
 import serializable
 import utils
 import cobbler_msg
@@ -70,7 +70,7 @@ class Collection(serializable.Serializable):
         won't be added to the collection).
         """
         if ref is None or not ref.is_valid():
-            raise CobblerException("bad_param")
+            raise cexceptions.CobblerException("bad_param")
         self.listing[ref.name] = ref
         return True
 
diff --git a/cobbler/collection_distros.py b/cobbler/collection_distros.py
index 5f49d16..9dde74b 100644
--- a/cobbler/collection_distros.py
+++ b/cobbler/collection_distros.py
@@ -8,6 +8,7 @@ Michael DeHaan <mdehaan@redhat.com
 import utils
 import collection
 import item_distro as distro
+import cexceptions
 
 class Distros(collection.Collection):
 
@@ -30,9 +31,9 @@ class Distros(collection.Collection):
         # first see if any Groups use this distro
         for v in self.config.profiles():
             if v.distro == name:
-               raise CobblerException("orphan_files")
+               raise cexceptions.CobblerException("orphan_files")
         if self.find(name):
             del self.listing[name]
             return True
-        raise CobblerException("delete_nothing")
+        raise cexceptions.CobblerException("delete_nothing")
 
diff --git a/cobbler/collection_profiles.py b/cobbler/collection_profiles.py
index b1c92dd..39bff02 100644
--- a/cobbler/collection_profiles.py
+++ b/cobbler/collection_profiles.py
@@ -10,7 +10,7 @@ Michael DeHaan <mdehaan@redhat.com>
 import item_profile as profile
 import utils
 import collection
-from cobbler_exception import CobblerException
+import cexceptions
 
 #--------------------------------------------
 
@@ -28,9 +28,9 @@ class Profiles(collection.Collection):
         """
         for k,v in self.config.systems().listing.items():
            if v.profile == name:
-               raise CobblerException("orphan_system")
+               raise cexceptions.CobblerException("orphan_system")
         if self.find(name):
             del self.listing[name]
             return True
-        raise CobblerException("delete_nothing")
+        raise cexceptions.CobblerException("delete_nothing")
 
diff --git a/cobbler/collection_systems.py b/cobbler/collection_systems.py
index 3147a67..aa26571 100644
--- a/cobbler/collection_systems.py
+++ b/cobbler/collection_systems.py
@@ -8,7 +8,7 @@ Michael DeHaan <mdehaan@redhat.com>
 import item_system as system
 import utils
 import collection
-from cobbler_exception import CobblerException
+import cexceptions
 
 #--------------------------------------------
 
@@ -33,5 +33,5 @@ class Systems(collection.Collection):
         if self.find(name):
             del self.listing[name]
             return True
-        raise CobblerException("delete_nothing")
+        raise cexceptions.CobblerException("delete_nothing")
 
diff --git a/cobbler/item_distro.py b/cobbler/item_distro.py
index 59250af..e61ea7b 100644
--- a/cobbler/item_distro.py
+++ b/cobbler/item_distro.py
@@ -10,7 +10,7 @@ import utils
 import item
 import weakref
 import os
-from cobbler_exception import CobblerException
+import cexceptions
 
 class Distro(item.Item):
 
@@ -51,7 +51,7 @@ class Distro(item.Item):
         if utils.find_kernel(kernel):
             self.kernel = kernel
             return True
-        raise CobblerException("no_kernel")
+        raise cexceptions.CobblerException("no_kernel")
 
     def set_initrd(self,initrd):
         """
@@ -61,7 +61,7 @@ class Distro(item.Item):
         if utils.find_initrd(initrd):
             self.initrd = initrd
             return True
-        raise CobblerException("no_initrd")
+        raise cexceptions.CobblerException("no_initrd")
 
     def is_valid(self):
         """
diff --git a/cobbler/item_profile.py b/cobbler/item_profile.py
index c344fc3..33cd3a3 100644
--- a/cobbler/item_profile.py
+++ b/cobbler/item_profile.py
@@ -7,7 +7,7 @@ Michael DeHaan <mdehaan@redhat.com>
 
 import utils
 import item
-from cobbler_exception import CobblerException
+import cexceptions
 
 class Profile(item.Item):
 
@@ -57,7 +57,7 @@ class Profile(item.Item):
         if self.config.distros().find(distro_name):
             self.distro = distro_name
             return True
-        raise CobblerException("no_distro")
+        raise cexceptions.CobblerException("no_distro")
 
     def set_kickstart(self,kickstart):
         """
@@ -67,7 +67,7 @@ class Profile(item.Item):
         if utils.find_kickstart(kickstart):
             self.kickstart = kickstart
             return True
-        raise CobblerException("no_kickstart")
+        raise cexceptions.CobblerException("no_kickstart")
 
     def set_xen_name(self,str):
         """
@@ -81,7 +81,7 @@ class Profile(item.Item):
         # no slashes or wildcards
         for bad in [ '/', '*', '?' ]:
             if str.find(bad) != -1:
-                return False
+                raise cexceptions.CobblerException("exc_xen_name")
         self.xen_name = str
         return True
 
@@ -98,13 +98,13 @@ class Profile(item.Item):
         try:
             inum = int(num)
             if inum != float(num):
-                return False
-            self.xen_file_size = inum
+                return cexceptions.CobblerException("exc_xen_file")
             if inum >= 0:
+                self.xen_file_size = inum
                 return True
-            return False
+            return cexceptions.CobblerException("exc_xen_file")
         except:
-            return False
+            return cexceptions.CobblerException("exc_xen_file")
 
     def set_xen_mac(self,mac):
         """
@@ -122,7 +122,7 @@ class Profile(item.Item):
             self.xen_mac = mac
             return True
         else:
-            return False
+            raise cexceptions.CobblerException("exc_xen_mac")
 
     def set_xen_paravirt(self,truthiness):
         """
@@ -140,9 +140,9 @@ class Profile(item.Item):
             elif (truthiness == True or truthiness.lower() == 'true'):
                 self.xen_paravirt = True
             else:
-                return False
+                return cexceptions.CobblerException("exc_xen_para")
         except:
-            return False
+            return cexceptions.CobblerException("exc_xen_para")
         return True
 
     def is_valid(self):
diff --git a/cobbler/item_system.py b/cobbler/item_system.py
index fef3743..4607623 100644
--- a/cobbler/item_system.py
+++ b/cobbler/item_system.py
@@ -6,7 +6,7 @@ Michael DeHaan <mdehaan@redhat.com>
 
 import utils
 import item
-from cobbler_exception import CobblerException
+import cexceptions
 
 class System(item.Item):
 
@@ -33,7 +33,7 @@ class System(item.Item):
         """
         new_name = utils.find_system_identifier(name)
         if not new_name:
-            raise CobblerException("bad_sys_name")
+            raise cexceptions.CobblerException("bad_sys_name")
         self.name = name  # we check it add time, but store the original value.
         return True
 
@@ -45,14 +45,14 @@ class System(item.Item):
         if self.config.profiles().find(profile_name):
             self.profile = profile_name
             return True
-        return False
+        raise cexceptions.CobblerException("exc_profile")
 
     def is_valid(self):
         """
 	A system is valid when it contains a valid name and a profile.
 	"""
         if self.name is None:
-            raise CobblerException("bad_sys_name")
+            return False
         if self.profile is None:
             return False
         return True
diff --git a/cobbler/serializer.py b/cobbler/serializer.py
index a63cd6e..bc46125 100644
--- a/cobbler/serializer.py
+++ b/cobbler/serializer.py
@@ -4,6 +4,7 @@ import syck  # PySyck 0.61 or greater, not syck-python 0.55
 import errno
 import os
 
+import cexceptions
 import utils
 
 def serialize(obj):
@@ -16,17 +17,17 @@ def serialize(obj):
     try:
         fd = open(filename,"w+")
     except IOError, ioe:
-        basename = os.path.basename(filename)
-        if not os.path.exists(basename):
+        dirname = os.path.dirname(filename)
+        if not os.path.exists(dirname):
            try:
-               os.makedirs(basename)
-           except:
-               raise CobblerException("need_perms", basename)
-               return False
+               os.makedirs(dirname)
+               # evidentally this doesn't throw exceptions.
+           except OSError, ose:
+               raise cexceptions.CobblerException("need_perms", os.path.dirname(dirname))
         try:
            fd = open(filename,"w+")
-        except:
-           raise CobblerException("need_perms", filename)
+        except IOError, ioe3:
+           raise cexceptions.CobblerException("need_perms", filename)
            return False
     datastruct = obj.to_datastruct()
     encoded = syck.dump(datastruct)