From 6375554363c111fda70b7540c3c56a0e9cf1497b Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Tue, 16 Apr 2013 15:57:58 +0100 Subject: 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. --- check | 5 +++++ morphlib/app.py | 4 ++-- morphlib/builder2.py | 9 +++++++-- morphlib/cachedrepo_tests.py | 9 +++++---- morphlib/extractedtarball.py | 11 ++++++++--- morphlib/mountableimage.py | 19 ++++++++++++------- morphlib/plugins/branch_and_merge_plugin.py | 18 ++++++++++-------- morphlib/remoterepocache.py | 10 +++++++--- 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 -- cgit v1.2.1