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

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3006
Proposed branch: lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333
Merge into: lp:software-center
Diff against target: 189 lines (+77/-34)
4 files modified
softwarecenter/config.py (+20/-5)
softwarecenter/log.py (+2/-6)
softwarecenter/utils.py (+37/-22)
test/test_utils.py (+18/-1)
To merge this branch: bzr merge lp:~gary-lasker/software-center/fix-shutdown-crash-lp996333
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email: mp+105027@code.launchpad.net

Description of the change

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!

To post a comment you must log in.
Michael Vogt (mvo) wrote :

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) wrote :

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

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) wrote :

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) wrote :

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.

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) wrote :

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) wrote :

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
3006. By Gary Lasker on 2012-05-11

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

Gary Lasker (gary-lasker) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/config.py'
2--- softwarecenter/config.py 2012-04-02 18:23:20 +0000
3+++ softwarecenter/config.py 2012-05-11 22:58:19 +0000
4@@ -23,9 +23,12 @@
5 except ImportError:
6 from ConfigParser import SafeConfigParser
7 import os
8+import logging
9
10 from paths import SOFTWARE_CENTER_CONFIG_FILE
11
12+LOG = logging.getLogger(__name__)
13+
14
15 class SoftwareCenterConfig(SafeConfigParser):
16
17@@ -39,16 +42,28 @@
18 self.configfile = config
19 try:
20 self.read(self.configfile)
21- except:
22+ except Exception as e:
23 # don't crash on a corrupted config file
24+ LOG.warn("Could not read the config file '%s': %s",
25+ self.configfile, e)
26 pass
27
28 def write(self):
29 tmpname = self.configfile + ".new"
30- f = open(tmpname, "w")
31- SafeConfigParser.write(self, f)
32- f.close()
33- os.rename(tmpname, self.configfile)
34+ from utils import ensure_file_writable_and_delete_if_not
35+ ensure_file_writable_and_delete_if_not(tmpname)
36+ ensure_file_writable_and_delete_if_not(self.configfile)
37+ try:
38+ f = open(tmpname, "w")
39+ SafeConfigParser.write(self, f)
40+ f.close()
41+ os.rename(tmpname, self.configfile)
42+ except Exception as e:
43+ # don't crash if there's an error when writing to the config file
44+ # (LP: #996333)
45+ LOG.warn("Could not write the config file '%s': %s",
46+ self.configfile, e)
47+ pass
48
49
50 _software_center_config = None
51
52=== modified file 'softwarecenter/log.py'
53--- softwarecenter/log.py 2012-03-15 10:43:13 +0000
54+++ softwarecenter/log.py 2012-05-11 22:58:19 +0000
55@@ -23,6 +23,7 @@
56 import os.path
57
58 from paths import SOFTWARE_CENTER_CACHE_DIR
59+from utils import ensure_file_writable_and_delete_if_not
60
61 """ setup global logging for software-center """
62
63@@ -110,12 +111,7 @@
64 os.makedirs(SOFTWARE_CENTER_CACHE_DIR)
65
66 # according to bug 688682 many people have a non-writeable logfile
67-if os.path.exists(logfile_path) and not os.access(logfile_path, os.W_OK):
68- try:
69- logging.warn("trying to fix non-writeable logfile")
70- os.remove(logfile_path)
71- except:
72- logging.exception("failed to fix non-writeable logfile")
73+ensure_file_writable_and_delete_if_not(logfile_path)
74
75 logfile_handler = logging.handlers.RotatingFileHandler(
76 logfile_path, maxBytes=100 * 1000, backupCount=5)
77
78=== modified file 'softwarecenter/utils.py'
79--- softwarecenter/utils.py 2012-04-25 08:19:42 +0000
80+++ softwarecenter/utils.py 2012-05-11 22:58:19 +0000
81@@ -655,6 +655,43 @@
82 os.close(lock)
83
84
85+def make_string_from_list(base_str, item_list):
86+ """ This function takes a list of items and builds a nice human readable
87+ string with it of the form. Note that the base string needs a "%s".
88+ Example return:
89+ The base string with the list items a,b and c in it.
90+ Note that base_str needs to be a ngettext string already, so the
91+ example usage is:
92+ l = ["foo", "bar"]
93+ base_str = ngettext("This list: %s.", "This list: %s", len(l))
94+ s = make_string_from_list(base_string, l)
95+ """
96+ list_str = item_list[0]
97+ if len(item_list) > 1:
98+ # TRANSLATORS: this is a generic list delimit char, e.g. "foo, bar"
99+ list_str = _(", ").join(item_list[:-1])
100+ # TRANSLATORS: this is the last part of a list, e.g. "foo, bar and baz"
101+ list_str = _("%s and %s") % (list_str,
102+ item_list[-1])
103+ s = base_str % list_str
104+ return s
105+
106+
107+def ensure_file_writable_and_delete_if_not(file_path):
108+ """ This function checks for writeable access to a file and attempts to
109+ correct it if it is found to indeed be set as unwriteable
110+ """
111+ if os.path.exists(file_path) and not os.access(file_path, os.W_OK):
112+ try:
113+ LOG.warn("encountered non-writeable file, attempting to fix "
114+ "by deleting: %s",
115+ file_path)
116+ os.remove(file_path)
117+ except Exception as e:
118+ LOG.exception("failed to fix non-writeable file '%s': %s",
119+ file_path, e)
120+
121+
122 class SimpleFileDownloader(GObject.GObject):
123
124 LOG = logging.getLogger("softwarecenter.simplefiledownloader")
125@@ -782,28 +819,6 @@
126 self.emit('file-download-complete', self.dest_file_path)
127
128
129-def make_string_from_list(base_str, item_list):
130- """ This function takes a list of items and builds a nice human readable
131- string with it of the form. Note that the base string needs a "%s".
132- Example return:
133- The base string with the list items a,b and c in it.
134- Note that base_str needs to be a ngettext string already, so the
135- example usage is:
136- l = ["foo", "bar"]
137- base_str = ngettext("This list: %s.", "This list: %s", len(l))
138- s = make_string_from_list(base_string, l)
139- """
140- list_str = item_list[0]
141- if len(item_list) > 1:
142- # TRANSLATORS: this is a generic list delimit char, e.g. "foo, bar"
143- list_str = _(", ").join(item_list[:-1])
144- # TRANSLATORS: this is the last part of a list, e.g. "foo, bar and baz"
145- list_str = _("%s and %s") % (list_str,
146- item_list[-1])
147- s = base_str % list_str
148- return s
149-
150-
151 # those helpers are packaging system specific
152 from softwarecenter.db.pkginfo import get_pkg_info
153 # do not call here get_pkg_info, since package switch may not have been set
154
155=== modified file 'test/test_utils.py'
156--- test/test_utils.py 2012-04-26 00:27:45 +0000
157+++ test/test_utils.py 2012-05-11 22:58:19 +0000
158@@ -4,6 +4,7 @@
159 import glob
160 import multiprocessing
161 import os
162+import stat
163 import subprocess
164 import tempfile
165 import time
166@@ -158,7 +159,23 @@
167 self.assertEqual(
168 make_string_from_list(base, l),
169 "There was a problem posting this review to twister, factbook, identi.catz and baz (omg!)")
170-
171+
172+ def test_ensure_file_writable_and_delete_if_not(self):
173+ from softwarecenter.utils import ensure_file_writable_and_delete_if_not
174+ from tempfile import NamedTemporaryFile
175+ # first test that a non-writable file is deleted
176+ test_file_not_writeable = NamedTemporaryFile()
177+ os.chmod(test_file_not_writeable.name, stat.S_IRUSR)
178+ self.assertFalse(os.access(test_file_not_writeable.name, os.W_OK))
179+ ensure_file_writable_and_delete_if_not(test_file_not_writeable.name)
180+ self.assertFalse(os.path.exists(test_file_not_writeable.name))
181+ # then test that a writable file is not deleted
182+ test_file_writeable = NamedTemporaryFile()
183+ os.chmod(test_file_writeable.name, stat.S_IRUSR|stat.S_IWUSR)
184+ self.assertTrue(os.access(test_file_writeable.name, os.W_OK))
185+ ensure_file_writable_and_delete_if_not(test_file_writeable.name)
186+ self.assertTrue(os.path.exists(test_file_writeable.name))
187+
188
189 class TestExpungeCache(unittest.TestCase):
190

Subscribers

People subscribed via source and target branches