Merge lp:~jamesodhunt/upstart/bug-1221466+1222702 into lp:upstart

Proposed by James Hunt on 2013-09-10
Status: Merged
Merged at revision: 1549
Proposed branch: lp:~jamesodhunt/upstart/bug-1221466+1222702
Merge into: lp:upstart
Diff against target: 327 lines (+191/-43)
3 files modified
ChangeLog (+13/-0)
extra/upstart-file-bridge.c (+32/-8)
scripts/tests/test_pyupstart_session_init.py (+146/-35)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1221466+1222702
Reviewer Review Type Date Requested Status
Dimitri John Ledkov 2013-09-10 Approve on 2013-11-03
Review via email: mp+184829@code.launchpad.net

Description of the change

* extra/upstart-file-bridge.c:
  - create_handler(): Use original_file() for directories
    (LP: #1221466, #1222702).
  - watched_dir_new(): Additional assert.
  - watched_file_new():
    - Additional assert.
    - Store original directory path in file object to ensure reliable
      matching for directories.
* scripts/tests/test_pyupstart_session_init.py: Added file bridge tests
  for directory creation, modification and deletion.

As expected, new tests in this branch fail with current upstart-file-bridge, but pass when using the updated version in this branch.

To post a comment you must log in.
Dimitri John Ledkov (xnox) wrote :

lgtm.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2013-08-23 14:49:09 +0000
3+++ ChangeLog 2013-09-10 17:00:41 +0000
4@@ -1,3 +1,16 @@
5+2013-09-10 James Hunt <james.hunt@ubuntu.com>
6+
7+ * extra/upstart-file-bridge.c:
8+ - create_handler(): Use original_file() for directories
9+ (LP: #1221466, #1222702).
10+ - watched_dir_new(): Additional assert.
11+ - watched_file_new():
12+ - Additional assert.
13+ - Store original directory path in file object to ensure reliable
14+ matching for directories.
15+ * scripts/tests/test_pyupstart_session_init.py: Added file bridge tests
16+ for directory creation, modification and deletion.
17+
18 2013-08-23 James Hunt <james.hunt@ubuntu.com>
19
20 * NEWS: Release 1.10
21
22=== modified file 'extra/upstart-file-bridge.c'
23--- extra/upstart-file-bridge.c 2013-07-03 08:26:23 +0000
24+++ extra/upstart-file-bridge.c 2013-09-10 17:00:41 +0000
25@@ -177,8 +177,9 @@
26 *
27 * @file: WatchedFile:
28 *
29- * Obtain the appropriate WatchedFile path: either the original if the
30- * path underwent expansion, else the initial unexpanded path.
31+ * Obtain the appropriate WatchedFile path: the original if the
32+ * path underwent expansion or is a directory, else the initial
33+ * unexpanded path.
34 *
35 * Required for emitting events since jobs need the unexpanded path to
36 * allow their start/stop condition to match even if the path has
37@@ -261,6 +262,9 @@
38 * @parent: parent who is watching over us.
39 *
40 * Details of the file being watched.
41+ *
42+ * For directories, @original contains the full path specified by the
43+ * job and @path will contain @original, less any trailing slashes.
44 **/
45 typedef struct watched_file {
46 NihList entry;
47@@ -855,7 +859,7 @@
48 *
49 * Although technically fraudulent (the file might not have _just
50 * been created_ - it may have existed forever), it is necessary
51- * since otherwise jobs will hang around wating for the file to
52+ * since otherwise jobs will hang around waiting for the file to
53 * be 'freshly-created'. However, although nih_watch_new() has
54 * been told to run the create handler for pre-existing files
55 * that doesn't help as we don't watch the files, we watch
56@@ -986,10 +990,10 @@
57 * was modified.
58 */
59 if (file->events & IN_MODIFY)
60- handle_event (handled, file->path, IN_MODIFY, path);
61+ handle_event (handled, original_path (file), IN_MODIFY, path);
62 } else if (! strcmp (file->path, path)) {
63 /* Directory has been created */
64- handle_event (handled, file->path, IN_CREATE, NULL);
65+ handle_event (handled, original_path (file), IN_CREATE, NULL);
66 add_dir = TRUE;
67 nih_list_add (&entries, &file->entry);
68 }
69@@ -1464,6 +1468,10 @@
70
71 len = strlen (watched_path);
72
73+ /* Sanity-check */
74+ if (len <= 1)
75+ return NULL;
76+
77 if (len > 1 && watched_path[len-1] == '/') {
78 /* Better to remove a trailing slash before handing to
79 * inotify since although all works as expected, the
80@@ -1570,18 +1578,34 @@
81
82 len = strlen (path);
83
84+ /* Sanity-check */
85+ if (len <= 1)
86+ return NULL;
87+
88+ /* Determine if the file is a directory */
89 file->dir = (path[len-1] == '/');
90
91- /* optionally one or the other, but not both */
92+ /* Optionally one or the other, but not both */
93 if (file->dir || file->glob)
94 nih_assert (file->dir || file->glob);
95
96- file->path = nih_strdup (file, path);
97+ /* Don't store the trailing slash for directories since that
98+ * confuses the logic and is redundant (due to file->dir).
99+ */
100+ file->path = nih_strndup (file, path, file->dir ? len-1 : len);
101 if (! file->path)
102 goto error;
103
104 file->original = NULL;
105- if (original) {
106+
107+ if (file->dir) {
108+ /* Retain the original directory path to simplify event
109+ * matching.
110+ */
111+ file->original = nih_strndup (file, path, len);
112+ if (! file->original)
113+ goto error;
114+ } else if (original) {
115 file->original = nih_strdup (file, original);
116 if (! file->original)
117 goto error;
118
119=== modified file 'scripts/tests/test_pyupstart_session_init.py'
120--- scripts/tests/test_pyupstart_session_init.py 2013-08-06 16:54:28 +0000
121+++ scripts/tests/test_pyupstart_session_init.py 2013-09-10 17:00:41 +0000
122@@ -158,59 +158,170 @@
123 self.assertIsInstance(pid, int)
124 os.kill(pid, 0)
125
126- # Create a job that makes use of the file event to watch to a
127+ target_dir = tempfile.mkdtemp()
128+ file = target_dir + os.sep + 'foo'
129+ dir = target_dir + os.sep + 'bar'
130+
131+ # Create a job that makes use of the file event to watch a
132 # file in a newly-created directory.
133- dir = tempfile.mkdtemp()
134- file = dir + os.sep + 'foo'
135-
136- msg = 'got file %s' % file
137+ file_msg = 'got file %s' % file
138 lines = []
139 lines.append('start on file FILE=%s EVENT=create' % file)
140- lines.append('exec echo %s' % msg)
141- create_job = self.upstart.job_create('wait-for-file-creation', lines)
142- self.assertTrue(create_job)
143+ lines.append('exec echo %s' % file_msg)
144+ create_file_job = self.upstart.job_create('wait-for-file-creation', lines)
145+ self.assertTrue(create_file_job)
146
147- # Create empty file
148- open(file, 'w').close()
149+ # Create job that waits for a file modification
150+ lines = []
151+ lines.append('start on file FILE=%s EVENT=modify' % file)
152+ lines.append('exec echo %s' % file_msg)
153+ modify_file_job = self.upstart.job_create('wait-for-file-modify', lines)
154+ self.assertTrue(modify_file_job)
155
156 # Create another job that triggers when the same file is deleted
157 lines = []
158 lines.append('start on file FILE=%s EVENT=delete' % file)
159- lines.append('exec echo %s' % msg)
160- delete_job = self.upstart.job_create('wait-for-file-deletion', lines)
161- self.assertTrue(delete_job)
162+ lines.append('exec echo %s' % file_msg)
163+ delete_file_job = self.upstart.job_create('wait-for-file-deletion', lines)
164+ self.assertTrue(delete_file_job)
165+
166+ # Create job that triggers on directory creation
167+ dir_msg = 'got directory %s' % dir
168+ lines = []
169+ # XXX: note the trailing slash to force a directory watch
170+ lines.append('start on file FILE=%s/ EVENT=create' % dir)
171+ lines.append('exec echo %s' % dir_msg)
172+ create_dir_job = self.upstart.job_create('wait-for-dir-creation', lines)
173+ self.assertTrue(create_dir_job)
174+
175+ # Create job that triggers on directory modification
176+ lines = []
177+ # XXX: note the trailing slash to force a directory watch
178+ lines.append('start on file FILE=%s/ EVENT=modify' % dir)
179+ lines.append('exec echo %s' % dir_msg)
180+ modify_dir_job = self.upstart.job_create('wait-for-dir-modify', lines)
181+ self.assertTrue(modify_dir_job)
182+
183+ # Create job that triggers on directory deletion
184+ lines = []
185+ # XXX: note the trailing slash to force a directory watch
186+ lines.append('start on file FILE=%s/ EVENT=delete' % dir)
187+ lines.append('exec echo %s' % dir_msg)
188+ delete_dir_job = self.upstart.job_create('wait-for-dir-delete', lines)
189+ self.assertTrue(delete_dir_job)
190+
191+ # Create empty file
192+ open(file, 'w').close()
193+
194+ # Create directory
195+ os.mkdir(dir)
196
197 # No need to start the jobs of course as the file-bridge handles that!
198
199 # Identify full path to job logfiles
200- create_job_logfile = create_job.logfile_name(dbus_encode(''))
201- self.assertTrue(create_job_logfile)
202-
203- delete_job_logfile = delete_job.logfile_name(dbus_encode(''))
204- self.assertTrue(delete_job_logfile)
205-
206- # Wait for the create job to run and produce output
207- self.assertTrue(wait_for_file(create_job_logfile))
208-
209- # Check the output
210- with open(create_job_logfile, 'r', encoding='utf-8') as f:
211- lines = f.readlines()
212- self.assertTrue(len(lines) == 1)
213- self.assertEqual(msg, lines[0].rstrip())
214-
215- shutil.rmtree(dir)
216+ create_file_job_logfile = create_file_job.logfile_name(dbus_encode(''))
217+ self.assertTrue(create_file_job_logfile)
218+
219+ modify_file_job_logfile = modify_file_job.logfile_name(dbus_encode(''))
220+ self.assertTrue(modify_file_job_logfile)
221+
222+ delete_file_job_logfile = delete_file_job.logfile_name(dbus_encode(''))
223+ self.assertTrue(delete_file_job_logfile)
224+
225+ create_dir_job_logfile = create_dir_job.logfile_name(dbus_encode(''))
226+ self.assertTrue(create_dir_job_logfile)
227+
228+ modify_dir_job_logfile = modify_dir_job.logfile_name(dbus_encode(''))
229+ self.assertTrue(modify_dir_job_logfile)
230+
231+ delete_dir_job_logfile = delete_dir_job.logfile_name(dbus_encode(''))
232+ self.assertTrue(delete_dir_job_logfile)
233+
234+ #--------------------
235+
236+ # Wait for the create file job to run and produce output
237+ self.assertTrue(wait_for_file(create_file_job_logfile))
238+
239+ # Check the output
240+ with open(create_file_job_logfile, 'r', encoding='utf-8') as f:
241+ lines = f.readlines()
242+ self.assertTrue(len(lines) == 1)
243+ self.assertEqual(file_msg, lines[0].rstrip())
244+
245+ #--------------------
246+
247+ # Wait for the create directory job to run and produce output
248+ self.assertTrue(wait_for_file(create_dir_job_logfile))
249+
250+ # Check the output
251+ with open(create_dir_job_logfile, 'r', encoding='utf-8') as f:
252+ lines = f.readlines()
253+ self.assertTrue(len(lines) == 1)
254+ self.assertEqual(dir_msg, lines[0].rstrip())
255+
256+ #--------------------
257+
258+ # Modify the file
259+ open(file, 'w').close()
260+
261+ # Wait for the create file job to run and produce output
262+ self.assertTrue(wait_for_file(modify_file_job_logfile))
263+
264+ # Check the output
265+ with open(modify_file_job_logfile, 'r', encoding='utf-8') as f:
266+ lines = f.readlines()
267+ self.assertTrue(len(lines) == 1)
268+ self.assertEqual(file_msg, lines[0].rstrip())
269+
270+ #--------------------
271+ # Modify the directory by creating a new file in it.
272+
273+ dir_file = dir + os.sep + 'baz'
274+ open(dir_file, 'w').close()
275+
276+ # Wait for the modify directory job to run and produce output
277+ self.assertTrue(wait_for_file(modify_dir_job_logfile))
278+
279+ # Check the output
280+ with open(modify_dir_job_logfile, 'r', encoding='utf-8') as f:
281+ lines = f.readlines()
282+ self.assertTrue(len(lines) == 1)
283+ self.assertEqual(dir_msg, lines[0].rstrip())
284+
285+ #--------------------
286+
287+ os.remove(dir_file)
288+ os.rmdir(dir)
289+
290+ # Wait for the delete directory job to run and produce output
291+ self.assertTrue(wait_for_file(delete_dir_job_logfile))
292+
293+ # Check the output
294+ with open(delete_dir_job_logfile, 'r', encoding='utf-8') as f:
295+ lines = f.readlines()
296+ self.assertTrue(len(lines) == 1)
297+ self.assertEqual(dir_msg, lines[0].rstrip())
298+ #--------------------
299+
300+ shutil.rmtree(target_dir)
301
302 # Wait for the delete job to run and produce output
303- self.assertTrue(wait_for_file(delete_job_logfile))
304+ self.assertTrue(wait_for_file(delete_file_job_logfile))
305
306 # Check the output
307- with open(delete_job_logfile, 'r', encoding='utf-8') as f:
308+ with open(delete_file_job_logfile, 'r', encoding='utf-8') as f:
309 lines = f.readlines()
310 self.assertTrue(len(lines) == 1)
311- self.assertEqual(msg, lines[0].rstrip())
312-
313- os.remove(create_job_logfile)
314- os.remove(delete_job_logfile)
315+ self.assertEqual(file_msg, lines[0].rstrip())
316+
317+ #--------------------
318+
319+ os.remove(create_file_job_logfile)
320+ os.remove(modify_file_job_logfile)
321+ os.remove(delete_file_job_logfile)
322+ os.remove(create_dir_job_logfile)
323+ os.remove(modify_dir_job_logfile)
324+ os.remove(delete_dir_job_logfile)
325
326 file_bridge.stop()
327 self.stop_session_init()

Subscribers

People subscribed via source and target branches