Merge lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333 into lp:software-center

[Gary Lasker]

This branch fixes bug 996333, which is a crash that can occur if an error is encountered when writing out the software center config file on shutdown. Note that this crash has the nasty side-effect of blocking shutdown, so software-center never exits.

I've also added logging for errors that occur when both writing and reading the config file.

Many thanks for your review!

[Michael Vogt (mvo)]

Great that you found the root cause of the issue! The fix is fine, just some thoughts below:

I currently see two conditions for the failure:
a) a file that can not be written exists, so we could use os.access(path, os.W_OK) and try unlink the file
b) disk full, nothing we can do

I think it would be good to add a bit of code that tries to auto-correct (a). Code in softwarecenter/log.py
is a good example for this:

# according to bug 688682 many people have a non-writeable logfile
if os.path.exists(logfile_path) and not os.access(logfile_path, os.W_OK):
    try:
        logging.warn("trying to fix non-writeable logfile")
        os.remove(logfile_path)
    except:
        logging.exception("failed to fix non-writeable logfile")

Do you think you could add this ? Maybe adding a "ensure_writable()" helper to utils.py to reuse the
code from log.py ?

[Gary Lasker (gary-lasker)]

Hi mvo! This sound great, I'll work on this and push the changes for you to look at.

[lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333]

3002. By Gary Lasker on 2012-05-09

add a new ensure_file_writeable utility for utils and use it for both the config file and the log file

3003. By Gary Lasker on 2012-05-09

pep8, oops

[Gary Lasker (gary-lasker)]

Hi Michael, I've added a new ensure_file_writeable method to utils.py and wired it into the config and log code. Seems to work well!

Please take a look when you have the time. Thanks again!

[Michael Vogt (mvo)]

This looks good, having a unittest for the ensure_file_writeable() would be ideal, its hopefully
as simple as creating a 0600 file and ensuring that its still there after ensure_writalble and
a 0400 file and ensure if not.

Thinking about it a better name is probably "ensure_file_writable_and_delete_if_not" or "try_remove_if_non_writable()" to ensure that the dangerous nature of it is clearer.

[lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333]

3004. By Gary Lasker on 2012-05-09

change name of method to ensure_file_writable_and_delete_if_not to make it's dangerousness more clear, and also improve for log message for the same reason

3005. By Gary Lasker on 2012-05-09

add unit test for new utils function ensure_file_writable_and_delete_if_not

[Gary Lasker (gary-lasker)]

Hi again mvo, just a note that the unit test is added now and I've renamed the method in utils.

THanks again!

[Michael Vogt (mvo)]

Thanks for the test you added and the rename of the function.

Unfortunately it has some issues, see my comments below:

[..]
167 + # first test that a non-writable file is deleted
168 + test_file_not_writeable = NamedTemporaryFile()
169 + os.chmod(test_file_not_writeable.name, 600)
[..]

To use mode decimal 600 here is very confusing. Normally you would use mode octal 0600 (as I outlined
in my previous comment) which means "user-read", "user-write" and test if the file is not deleted.
Mode numeric 600 is octal 01130 which does not make much sense in the unix permission model.
You probably want to use 0400 here instead or stat.S_IRUSR.

173 + # then test that a writable file is not deleted
174 + test_file_writeable = NamedTemporaryFile()
175 + os.chmod(test_file_writeable.name, 400)
[..]

Same problem as above, using the decimal value here is confusing, its 0420 which is user-read,user-write
and group-write. A uncommon combination. Please use 0600 here instead or stat.S_IRUSR|stat.S_IWUSR

review: Needs Fixing

[lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333]

3006. By Gary Lasker on 2012-05-11

use stat values for setting the chmod values in the unit test

[Gary Lasker (gary-lasker)]

Hi Michael, yes, thanks, decimal values are unuseful indeed, my apologies for the mistake. I updated the test using stat.S_IRUSR and stat.S_IWUSR as these definitely seem more proper (although, hmm, not exactly "readable" with those names, oh well).

Hopefully this will take care of it. Thanks again!

[Michael Vogt (mvo)]

Thanks, that fixes the issue nicely. I agree that the stat values are not that great either, I added a small
comment there with the octal mode (0600, 0400) too (I find the octal stuff easier than the stat stuff, but
that is just personal taste).

review: Approve

=== modified file 'softwarecenter/config.py'
--- softwarecenter/config.py	2012-04-02 18:23:20 +0000
+++ softwarecenter/config.py	2012-05-11 22:58:19 +0000
@@ -23,9 +23,12 @@
 except ImportError:
     from ConfigParser import SafeConfigParser
 import os
+import logging
 
 from paths import SOFTWARE_CENTER_CONFIG_FILE
 
+LOG = logging.getLogger(__name__)
+
 
 class SoftwareCenterConfig(SafeConfigParser):
 
@@ -39,16 +42,28 @@
         self.configfile = config
         try:
             self.read(self.configfile)
-        except:
+        except Exception as e:
             # don't crash on a corrupted config file
+            LOG.warn("Could not read the config file '%s': %s",
+                     self.configfile, e)
             pass
 
     def write(self):
         tmpname = self.configfile + ".new"
-        f = open(tmpname, "w")
-        SafeConfigParser.write(self, f)
-        f.close()
-        os.rename(tmpname, self.configfile)
+        from utils import ensure_file_writable_and_delete_if_not
+        ensure_file_writable_and_delete_if_not(tmpname)
+        ensure_file_writable_and_delete_if_not(self.configfile)
+        try:
+            f = open(tmpname, "w")
+            SafeConfigParser.write(self, f)
+            f.close()
+            os.rename(tmpname, self.configfile)
+        except Exception as e:
+            # don't crash if there's an error when writing to the config file
+            # (LP: #996333)
+            LOG.warn("Could not write the config file '%s': %s",
+                     self.configfile, e)
+            pass
 
 
 _software_center_config = None
 
=== modified file 'softwarecenter/log.py'
--- softwarecenter/log.py	2012-03-15 10:43:13 +0000
+++ softwarecenter/log.py	2012-05-11 22:58:19 +0000
@@ -23,6 +23,7 @@
 import os.path
 
 from paths import SOFTWARE_CENTER_CACHE_DIR
+from utils import ensure_file_writable_and_delete_if_not
 
 """ setup global logging for software-center """
 
@@ -110,12 +111,7 @@
     os.makedirs(SOFTWARE_CENTER_CACHE_DIR)
 
 # according to bug 688682 many people have a non-writeable logfile
-if os.path.exists(logfile_path) and not os.access(logfile_path, os.W_OK):
-    try:
-        logging.warn("trying to fix non-writeable logfile")
-        os.remove(logfile_path)
-    except:
-        logging.exception("failed to fix non-writeable logfile")
+ensure_file_writable_and_delete_if_not(logfile_path)
 
 logfile_handler = logging.handlers.RotatingFileHandler(
     logfile_path, maxBytes=100 * 1000, backupCount=5)
 
=== modified file 'softwarecenter/utils.py'
--- softwarecenter/utils.py	2012-04-25 08:19:42 +0000
+++ softwarecenter/utils.py	2012-05-11 22:58:19 +0000
@@ -655,6 +655,43 @@
     os.close(lock)
 
 
+def make_string_from_list(base_str, item_list):
+    """ This function takes a list of items and builds a nice human readable
+        string with it of the form. Note that the base string needs a "%s".
+        Example return:
+          The base string with the list items a,b and c in it.
+        Note that base_str needs to be a ngettext string already, so the
+        example usage is:
+         l = ["foo", "bar"]
+         base_str = ngettext("This list: %s.", "This list: %s", len(l))
+         s = make_string_from_list(base_string, l)
+    """
+    list_str = item_list[0]
+    if len(item_list) > 1:
+        # TRANSLATORS: this is a generic list delimit char, e.g. "foo, bar"
+        list_str = _(", ").join(item_list[:-1])
+        # TRANSLATORS: this is the last part of a list, e.g. "foo, bar and baz"
+        list_str = _("%s and %s") % (list_str,
+                                     item_list[-1])
+    s = base_str % list_str
+    return s
+
+
+def ensure_file_writable_and_delete_if_not(file_path):
+    """ This function checks for writeable access to a file and attempts to
+        correct it if it is found to indeed be set as unwriteable
+    """
+    if os.path.exists(file_path) and not os.access(file_path, os.W_OK):
+        try:
+            LOG.warn("encountered non-writeable file, attempting to fix "
+                     "by deleting: %s",
+                      file_path)
+            os.remove(file_path)
+        except Exception as e:
+            LOG.exception("failed to fix non-writeable file '%s': %s",
+                          file_path, e)
+
+
 class SimpleFileDownloader(GObject.GObject):
 
     LOG = logging.getLogger("softwarecenter.simplefiledownloader")
@@ -782,28 +819,6 @@
         self.emit('file-download-complete', self.dest_file_path)
 
 
-def make_string_from_list(base_str, item_list):
-    """ This function takes a list of items and builds a nice human readable
-        string with it of the form. Note that the base string needs a "%s".
-        Example return:
-          The base string with the list items a,b and c in it.
-        Note that base_str needs to be a ngettext string already, so the
-        example usage is:
-         l = ["foo", "bar"]
-         base_str = ngettext("This list: %s.", "This list: %s", len(l))
-         s = make_string_from_list(base_string, l)
-    """
-    list_str = item_list[0]
-    if len(item_list) > 1:
-        # TRANSLATORS: this is a generic list delimit char, e.g. "foo, bar"
-        list_str = _(", ").join(item_list[:-1])
-        # TRANSLATORS: this is the last part of a list, e.g. "foo, bar and baz"
-        list_str = _("%s and %s") % (list_str,
-                                     item_list[-1])
-    s = base_str % list_str
-    return s
-
-
 # those helpers are packaging system specific
 from softwarecenter.db.pkginfo import get_pkg_info
 # do not call here get_pkg_info, since package switch may not have been set
 
=== modified file 'test/test_utils.py'
--- test/test_utils.py	2012-04-26 00:27:45 +0000
+++ test/test_utils.py	2012-05-11 22:58:19 +0000
@@ -4,6 +4,7 @@
 import glob
 import multiprocessing
 import os
+import stat
 import subprocess
 import tempfile
 import time
@@ -158,7 +159,23 @@
         self.assertEqual(
             make_string_from_list(base, l),
             "There was a problem posting this review to twister, factbook, identi.catz and baz (omg!)")
-        
+            
+    def test_ensure_file_writable_and_delete_if_not(self):
+        from softwarecenter.utils import ensure_file_writable_and_delete_if_not
+        from tempfile import NamedTemporaryFile
+        # first test that a non-writable file is deleted
+        test_file_not_writeable = NamedTemporaryFile()
+        os.chmod(test_file_not_writeable.name, stat.S_IRUSR)
+        self.assertFalse(os.access(test_file_not_writeable.name, os.W_OK))
+        ensure_file_writable_and_delete_if_not(test_file_not_writeable.name)
+        self.assertFalse(os.path.exists(test_file_not_writeable.name))
+        # then test that a writable file is not deleted
+        test_file_writeable = NamedTemporaryFile()
+        os.chmod(test_file_writeable.name, stat.S_IRUSR|stat.S_IWUSR)
+        self.assertTrue(os.access(test_file_writeable.name, os.W_OK))
+        ensure_file_writable_and_delete_if_not(test_file_writeable.name)
+        self.assertTrue(os.path.exists(test_file_writeable.name))
+
 
 class TestExpungeCache(unittest.TestCase):