Stop using bare except: statements

It is almost never a good idea to catch all exceptions, and then do
nothing about them. This patch logs all caught exceptions so that the
user has some possibilty to debug what is happening.

Also, make ./check check for bare excepts and fail the test suite if it
finds anything.

8 files changed, 56 insertions, 29 deletions

diff --git a/check b/check
index 170ed8bf..5a18310a 100755
--- a/check
+++ b/check
@@ -126,6 +126,11 @@ then
                     echo "ERROR: $x has a hashbang" 1>&2
                     errors=1
                 fi
+                if grep except: "$x"
+                then
+                    echo "ERROR: $x has a bare except:" 1>&2
+                    errors=1
+                fi
                 ;;
         esac
     done
diff --git a/morphlib/app.py b/morphlib/app.py
index 859019f9..1d4d6fb0 100755
--- a/morphlib/app.py
+++ b/morphlib/app.py
@@ -253,8 +253,8 @@ class Morph(cliapp.Application):
                                 reponame=reponame,
                                 ref=ref,
                                 chatty=True)
-            except:
-                pass
+            except BaseException, e:
+                logging.warning('Caught (and ignored) exception: %s' % str(e))
         if absref is None:
             if update:
                 self.status(msg='Caching git repository %(reponame)s',
diff --git a/morphlib/builder2.py b/morphlib/builder2.py
index 7ded281e..93af25e7 100644
--- a/morphlib/builder2.py
+++ b/morphlib/builder2.py
@@ -304,7 +304,9 @@ class ChunkBuilder(BuilderBase):
                     log_name = log.real_filename
                     self.run_commands(builddir, destdir, log)
                     self.create_devices(destdir)
-            except:
+            except BaseException, e:
+                logging.error('Caught exception: %s' % str(e))
+                logging.info('Cleaning up staging area')
                 self.staging_area.chroot_close()
                 if log_name:
                     with open(log_name) as f:
@@ -760,8 +762,11 @@ class DiskImageBuilder(SystemKindBuilder):  # pragma: no cover
                     shutil.copyfileobj(ifh, ofh, 1024 * 1024)
                     ofh.close()
 
-            except:
+            except BaseException, e:
+                logging.error('Caught exception: %s' % str(e))
+                logging.info('Removing unfinished disk image %s' % image_name)
                 os.remove(image_name)
+                logging.info('Removing unfinished file from cache')
                 handle.abort()
                 raise
 
diff --git a/morphlib/cachedrepo_tests.py b/morphlib/cachedrepo_tests.py
index 9251a473..3e042b19 100644
--- a/morphlib/cachedrepo_tests.py
+++ b/morphlib/cachedrepo_tests.py
@@ -14,6 +14,7 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 
+import logging
 import os
 import unittest
 
@@ -42,7 +43,7 @@ class CachedRepoTests(unittest.TestCase):
         }
         try:
             return output[ref]
-        except:
+        except KeyError:
             raise cliapp.AppException('git rev-parse --verify %s' % ref)
 
     def show_tree_hash(self, absref):
@@ -56,7 +57,7 @@ class CachedRepoTests(unittest.TestCase):
         }
         try:
             return output[absref]
-        except:
+        except KeyError:
             raise cliapp.AppException('git log -1 --format=format:%%T %s' %
                                       absref)
 
@@ -67,7 +68,7 @@ class CachedRepoTests(unittest.TestCase):
         }
         try:
             return output['%s:%s' % (ref, filename)]
-        except:
+        except KeyError:
             raise cliapp.AppException(
                 'git cat-file blob %s:%s' % (ref, filename))
 
@@ -91,7 +92,7 @@ class CachedRepoTests(unittest.TestCase):
         }
         try:
             return output[ref]
-        except:
+        except KeyError:
             raise cliapp.AppException('git ls-tree --name-only %s' % (ref))
 
     def clone_into(self, target_dir, ref):
diff --git a/morphlib/extractedtarball.py b/morphlib/extractedtarball.py
index e435b1ef..fd98cd92 100644
--- a/morphlib/extractedtarball.py
+++ b/morphlib/extractedtarball.py
@@ -16,6 +16,7 @@
 
 import cliapp
 import gzip
+import logging
 import os
 import tempfile
 import shutil
@@ -41,7 +42,9 @@ class ExtractedTarball(object): # pragma: no cover
         self.tempdir = tempfile.mkdtemp(dir=self.app.settings['tempdir'])
         try:
             morphlib.bins.unpack_binary(self.tarball, self.tempdir)
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
+            logging.debug('Removing temporary directory %s' % self.tempdir)
             shutil.rmtree(self.tempdir)
             raise
         return self.tempdir
@@ -51,8 +54,10 @@ class ExtractedTarball(object): # pragma: no cover
                         tarball=os.path.basename(self.tarball), chatty=True)
         try:
             shutil.rmtree(self.tempdir)
-        except:
-            pass
+        except BaseException, e:
+            logging.warning(
+                'Error when removing temporary directory %s: %s' %
+                    (self.tempdir, str(e)))
 
     def __enter__(self):
         return self.setup()
diff --git a/morphlib/mountableimage.py b/morphlib/mountableimage.py
index 3d29a516..f767228a 100644
--- a/morphlib/mountableimage.py
+++ b/morphlib/mountableimage.py
@@ -15,6 +15,7 @@
 
 
 import cliapp
+import logging
 import os
 import tempfile
 import gzip
@@ -45,7 +46,9 @@ class MountableImage(object): # pragma: no cover
                 infh = gzip.open(path, "rb")
                 morphlib.util.copyfileobj(infh, outfh)
                 infh.close()
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
+            logging.info('Removing temporary file %s' % self.temp_path)
             os.unlink(self.temp_path)
             raise
         self.app.status(msg='  Mounting image at %(path)s',
@@ -62,17 +65,19 @@ class MountableImage(object): # pragma: no cover
                         chatty=True)
         try:
             morphlib.fsutils.unmount(self.app.runcmd, mount_point)
-        except:
-            pass
+        except BaseException, e:
+            logging.info('Ignoring error when unmounting: %s' % str(e))
         try:
             morphlib.fsutils.undo_device_mapping(self.app.runcmd, path)
-        except:
-            pass
+        except BaseException, e:
+            logging.info(
+                'Ignoring error when undoing device mapping: %s' % str(e))
         try:
             os.rmdir(mount_point)
             os.unlink(path)
-        except:
-            pass
+        except BaseException, e:
+            logging.info(
+                'Ignoring error when removing temporary files: %s' % str(e))
 
     def __enter__(self):
         return self.setup(self.artifact_path)
diff --git a/morphlib/plugins/branch_and_merge_plugin.py b/morphlib/plugins/branch_and_merge_plugin.py
index 25f786e0..e39141cc 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -231,7 +231,9 @@ class BranchAndMergePlugin(cliapp.Plugin):
         try:
             return self.app.runcmd(['git', 'rev-parse', '--verify', ref],
                                    cwd=repodir)[0:40]
-        except:
+        except cliapp.AppException, e:
+            logging.info(
+                'Ignoring error executing git rev-parse: %s' % str(e))
             return None
 
     def resolve_reponame(self, reponame):
@@ -558,11 +560,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                                           'directory as a workspace: %s' %
                                           dirname)
         else:
-            try:
-                os.makedirs(dirname)
-            except:
-                raise cliapp.AppException('failed to create workspace: %s' %
-                                          dirname)
+            os.makedirs(dirname)
 
         os.mkdir(os.path.join(dirname, '.morph'))
         self.app.status(msg='Initialized morph workspace', chatty=True)
@@ -600,7 +598,9 @@ class BranchAndMergePlugin(cliapp.Plugin):
                                  original_ref], cwd=repo_dir)
 
             return branch_dir
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
+            logging.info('Removing half-finished branch %s' % branch_name)
             self.remove_branch_dir_safe(workspace, branch_name)
             raise
 
@@ -1513,7 +1513,9 @@ class BranchAndMergePlugin(cliapp.Plugin):
                     "case, and then repeat the 'morph merge' operation." %
                     from_branch)
             self.app.status(msg="Merge successful")
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
+            logging.info('Resetting half-finished merge')
             for repo_dir, sha1 in merged_repos.itervalues():
                 self.reset_work_tree_safe(repo_dir)
             raise
diff --git a/morphlib/remoterepocache.py b/morphlib/remoterepocache.py
index 5c2017de..d6812120 100644
--- a/morphlib/remoterepocache.py
+++ b/morphlib/remoterepocache.py
@@ -16,6 +16,7 @@
 
 import cliapp
 import json
+import logging
 import urllib2
 import urlparse
 
@@ -53,14 +54,16 @@ class RemoteRepoCache(object):
         repo_url = self._resolver.pull_url(repo_name)
         try:
             return self._resolve_ref_for_repo_url(repo_url, ref)
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
             raise ResolveRefError(repo_name, ref)
 
     def cat_file(self, repo_name, ref, filename):
         repo_url = self._resolver.pull_url(repo_name)
         try:
             return self._cat_file_for_repo_url(repo_url, ref, filename)
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
             raise CatFileError(repo_name, ref, filename)
 
     def ls_tree(self, repo_name, ref):
@@ -68,7 +71,8 @@ class RemoteRepoCache(object):
         try:
             info = json.loads(self._ls_tree_for_repo_url(repo_url, ref))
             return info['tree'].keys()
-        except:
+        except BaseException, e:
+            logging.error('Caught exception: %s' % str(e))
             raise LsTreeError(repo_name, ref)
 
     def _resolve_ref_for_repo_url(self, repo_url, ref):  # pragma: no cover