Merge lp:~bjornt/landscape-client/disable-apport-dpkg-crash into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Prevent Apport from generating crash reports when dpkg returns an error.

When dpkg returns an error, a SystemError is raised and the error is
being caught and cleaned up in the Apt C binding. However, the Apport
hook was still being executed, generating a crash reports, even though
nothing crashed.

Now we install a custom excepthook, which passes all errors to Apport,
except for SystemErrrors. We install the excepthook only in the child
process that executes dpkg after a fork.

[Chris Glass (tribaal)]

Looks good! +1

review: Approve

[Jerry Seutter (jseutter)]

+1 looks good :)

review: Approve

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2012-05-08 23:54:37 +0000
+++ landscape/package/facade.py	2012-11-20 13:45:23 +0000
@@ -1,6 +1,7 @@
 import hashlib
 import os
 import subprocess
+import sys
 import tempfile
 from cStringIO import StringIO
 from operator import attrgetter
@@ -59,6 +60,7 @@
 class LandscapeInstallProgress(InstallProgress):
 
     dpkg_exited = None
+    old_excepthook = None
 
     def wait_child(self):
         """Override to find out whether dpkg exited or not.
@@ -76,6 +78,32 @@
         self.dpkg_exited = os.WIFEXITED(res)
         return res
 
+    def fork(self):
+        """Fork and override the excepthook in the child process."""
+        pid = super(LandscapeInstallProgress, self).fork()
+        if pid == 0:
+            # No need to clean up after ourselves, since the child
+            # process will die after dpkg has been run.
+            self.old_excepthook = sys.excepthook
+            sys.excepthook = self._prevent_dpkg_apport_error
+        return pid
+
+    def _prevent_dpkg_apport_error(self, exc_type, exc_obj, exc_tb):
+        """Prevent dpkg errors from generating Apport crash reports.
+
+        When dpkg reports an error, a SystemError is raised and cleaned
+        up in C code. However, it seems like the Apport except hook is
+        called before the C code clears the error, generating crash
+        reports even though nothing crashed.
+
+        This exception hook doesn't call the Apport hook for
+        SystemErrors, but it calls it for all other errors.
+        """
+        if exc_type is SystemError:
+            sys.__excepthook__(exc_type, exc_obj, exc_tb)
+            return
+        self.old_excepthook(exc_type, exc_obj, exc_tb)
+
 
 class AptFacade(object):
     """Wrapper for tasks using Apt.

=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2012-05-08 23:21:07 +0000
+++ landscape/package/tests/test_facade.py	2012-11-20 13:45:23 +0000
@@ -1,4 +1,5 @@
 import os
+import sys
 import textwrap
 import tempfile
 
@@ -9,7 +10,8 @@
 from landscape.constants import UBUNTU_PATH
 from landscape.lib.fs import read_file, create_file
 from landscape.package.facade import (
-    TransactionError, DependencyError, ChannelError, AptFacade)
+    TransactionError, DependencyError, ChannelError, AptFacade,
+    LandscapeInstallProgress)
 
 from landscape.tests.mocker import ANY
 from landscape.tests.helpers import LandscapeTest, EnvironSaverHelper
@@ -1045,12 +1047,64 @@
 
         This test executes dpkg for real, which should fail, complaining
         that superuser privileges are needed.
-        """
+
+        The error from the dpkg sub process is included.
+        """
+        self._add_system_package("foo")
+        self.facade.reload_channels()
+        foo = self.facade.get_packages_by_name("foo")[0]
+        self.facade.mark_remove(foo)
+        exception = self.assertRaises(
+            TransactionError, self.facade.perform_changes)
+        self.assertIn("Error in function: \nSystemError:", exception.args[0])
+
+    def test_perform_changes_dpkg_error_retains_excepthook(self):
+        """
+        We install a special excepthook when preforming package
+        operations, to prevent Apport from generating crash reports when
+        dpkg returns a failure. It's only installed when doing the
+        actual package operation, so the original excepthook is there
+        after the perform_changes() method returns.
+        """
+        old_excepthook = sys.excepthook
         self._add_system_package("foo")
         self.facade.reload_channels()
         foo = self.facade.get_packages_by_name("foo")[0]
         self.facade.mark_remove(foo)
         self.assertRaises(TransactionError, self.facade.perform_changes)
+        self.assertIs(old_excepthook, sys.excepthook)
+
+    def test_prevent_dpkg_apport_error_system_error(self):
+        """
+        C{_prevent_dpkg_apport_error} prevents the Apport excepthook
+        from being called when a SystemError happens, since SystemErrors
+        are expected to happen and will be caught in the Apt C binding..
+        """
+        hook_calls = []
+
+        progress = LandscapeInstallProgress()
+        progress.old_excepthook = (
+            lambda exc_type, exc_value, exc_tb: hook_calls.append(
+                (exc_type, exc_value, exc_tb)))
+        progress._prevent_dpkg_apport_error(
+            SystemError, SystemError("error"), object())
+        self.assertEqual([], hook_calls)
+
+    def test_prevent_dpkg_apport_error_non_system_error(self):
+        """
+        If C{_prevent_dpkg_apport_error} gets an exception that isn't a
+        SystemError, the old Apport hook is being called.
+        """
+        hook_calls = []
+
+        progress = LandscapeInstallProgress()
+        progress.old_excepthook = (
+            lambda exc_type, exc_value, exc_tb: hook_calls.append(
+                (exc_type, exc_value, exc_tb)))
+        error = object()
+        traceback = object()
+        progress._prevent_dpkg_apport_error(Exception, error, traceback)
+        self.assertEqual([(Exception, error, traceback)], hook_calls)
 
     def test_perform_changes_dpkg_exit_dirty(self):
         """