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

[James Hunt]

* 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.

[Dimitri John Ledkov (xnox)]

lgtm.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-08-23 14:49:09 +0000
+++ ChangeLog	2013-09-10 17:00:41 +0000
@@ -1,3 +1,16 @@
[phone]  James Hunt  <[e-mail]>
+
+	* 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.
+
 2013-08-23  James Hunt  <[e-mail]>
 
 	* NEWS: Release 1.10
 
=== modified file 'extra/upstart-file-bridge.c'
--- extra/upstart-file-bridge.c	2013-07-03 08:26:23 +0000
+++ extra/upstart-file-bridge.c	2013-09-10 17:00:41 +0000
@@ -177,8 +177,9 @@
  *
  * @file: WatchedFile:
  *
- * Obtain the appropriate WatchedFile path: either the original if the
- * path underwent expansion, else the initial unexpanded path.
+ * Obtain the appropriate WatchedFile path: the original if the
+ * path underwent expansion or is a directory, else the initial
+ * unexpanded path.
  *
  * Required for emitting events since jobs need the unexpanded path to
  * allow their start/stop condition to match even if the path has
@@ -261,6 +262,9 @@
  * @parent: parent who is watching over us.
  *
  * Details of the file being watched.
+ *
+ * For directories, @original contains the full path specified by the
+ * job and @path will contain @original, less any trailing slashes.
  **/
 typedef struct watched_file {
 	NihList      entry;
@@ -855,7 +859,7 @@
 	 *
 	 * Although technically fraudulent (the file might not have _just
 	 * been created_ - it may have existed forever), it is necessary
-	 * since otherwise jobs will hang around wating for the file to
+	 * since otherwise jobs will hang around waiting for the file to
 	 * be 'freshly-created'. However, although nih_watch_new() has
 	 * been told to run the create handler for pre-existing files
 	 * that doesn't help as we don't watch the files, we watch
@@ -986,10 +990,10 @@
 				 * was modified.
 				 */
 				if (file->events & IN_MODIFY)
-					handle_event (handled, file->path, IN_MODIFY, path);
+					handle_event (handled, original_path (file), IN_MODIFY, path);
 			} else if (! strcmp (file->path, path)) {
 				/* Directory has been created */
-				handle_event (handled, file->path, IN_CREATE, NULL);
+				handle_event (handled, original_path (file), IN_CREATE, NULL);
 				add_dir = TRUE;
 				nih_list_add (&entries, &file->entry);
 			}
@@ -1464,6 +1468,10 @@
 
 	len = strlen (watched_path);
 
+	/* Sanity-check */
+	if (len <= 1)
+		return NULL;
+
 	if (len > 1 && watched_path[len-1] == '/') {
 		/* Better to remove a trailing slash before handing to
 		 * inotify since although all works as expected, the
@@ -1570,18 +1578,34 @@
 
 	len = strlen (path);
 
+	/* Sanity-check */
+	if (len <= 1)
+		return NULL;
+
+	/* Determine if the file is a directory */
 	file->dir = (path[len-1] == '/');
 
-	/* optionally one or the other, but not both */
+	/* Optionally one or the other, but not both */
 	if (file->dir || file->glob)
 		nih_assert (file->dir || file->glob);
 
-	file->path = nih_strdup (file, path);
+	/* Don't store the trailing slash for directories since that
+	 * confuses the logic and is redundant (due to file->dir).
+	 */
+	file->path = nih_strndup (file, path, file->dir ? len-1 : len);
 	if (! file->path)
 		goto error;
 
 	file->original = NULL;
-	if (original) {
+
+	if (file->dir) {
+		/* Retain the original directory path to simplify event
+		 * matching.
+		 */
+		file->original = nih_strndup (file, path, len);
+		if (! file->original)
+			goto error;
+	} else if (original) {
 		file->original = nih_strdup (file, original);
 		if (! file->original)
 			goto error;
 
=== modified file 'scripts/tests/test_pyupstart_session_init.py'
--- scripts/tests/test_pyupstart_session_init.py	2013-08-06 16:54:28 +0000
+++ scripts/tests/test_pyupstart_session_init.py	2013-09-10 17:00:41 +0000
@@ -158,59 +158,170 @@
             self.assertIsInstance(pid, int)
             os.kill(pid, 0)
 
-        # Create a job that makes use of the file event to watch to a
+        target_dir = tempfile.mkdtemp()
+        file = target_dir + os.sep + 'foo'
+        dir = target_dir + os.sep + 'bar'
+
+        # Create a job that makes use of the file event to watch a
         # file in a newly-created directory.
-        dir = tempfile.mkdtemp()
-        file = dir + os.sep + 'foo'
-
-        msg = 'got file %s' % file
+        file_msg = 'got file %s' % file
         lines = []
         lines.append('start on file FILE=%s EVENT=create' % file)
-        lines.append('exec echo %s' % msg)
-        create_job = self.upstart.job_create('wait-for-file-creation', lines)
-        self.assertTrue(create_job)
+        lines.append('exec echo %s' % file_msg)
+        create_file_job = self.upstart.job_create('wait-for-file-creation', lines)
+        self.assertTrue(create_file_job)
 
-        # Create empty file
-        open(file, 'w').close()
+        # Create job that waits for a file modification
+        lines = []
+        lines.append('start on file FILE=%s EVENT=modify' % file)
+        lines.append('exec echo %s' % file_msg)
+        modify_file_job = self.upstart.job_create('wait-for-file-modify', lines)
+        self.assertTrue(modify_file_job)
 
         # Create another job that triggers when the same file is deleted
         lines = []
         lines.append('start on file FILE=%s EVENT=delete' % file)
-        lines.append('exec echo %s' % msg)
-        delete_job = self.upstart.job_create('wait-for-file-deletion', lines)
-        self.assertTrue(delete_job)
+        lines.append('exec echo %s' % file_msg)
+        delete_file_job = self.upstart.job_create('wait-for-file-deletion', lines)
+        self.assertTrue(delete_file_job)
+
+        # Create job that triggers on directory creation
+        dir_msg = 'got directory %s' % dir
+        lines = []
+        # XXX: note the trailing slash to force a directory watch
+        lines.append('start on file FILE=%s/ EVENT=create' % dir)
+        lines.append('exec echo %s' % dir_msg)
+        create_dir_job = self.upstart.job_create('wait-for-dir-creation', lines)
+        self.assertTrue(create_dir_job)
+
+        # Create job that triggers on directory modification
+        lines = []
+        # XXX: note the trailing slash to force a directory watch
+        lines.append('start on file FILE=%s/ EVENT=modify' % dir)
+        lines.append('exec echo %s' % dir_msg)
+        modify_dir_job = self.upstart.job_create('wait-for-dir-modify', lines)
+        self.assertTrue(modify_dir_job)
+
+        # Create job that triggers on directory deletion
+        lines = []
+        # XXX: note the trailing slash to force a directory watch
+        lines.append('start on file FILE=%s/ EVENT=delete' % dir)
+        lines.append('exec echo %s' % dir_msg)
+        delete_dir_job = self.upstart.job_create('wait-for-dir-delete', lines)
+        self.assertTrue(delete_dir_job)
+
+        # Create empty file
+        open(file, 'w').close()
+
+        # Create directory
+        os.mkdir(dir)
 
         # No need to start the jobs of course as the file-bridge handles that!
 
         # Identify full path to job logfiles
-        create_job_logfile = create_job.logfile_name(dbus_encode(''))
-        self.assertTrue(create_job_logfile)
-
-        delete_job_logfile = delete_job.logfile_name(dbus_encode(''))
-        self.assertTrue(delete_job_logfile)
-
-        # Wait for the create job to run and produce output
-        self.assertTrue(wait_for_file(create_job_logfile))
-
-        # Check the output
-        with open(create_job_logfile, 'r', encoding='utf-8') as f:
-            lines = f.readlines()
-        self.assertTrue(len(lines) == 1)
-        self.assertEqual(msg, lines[0].rstrip())
-
-        shutil.rmtree(dir)
+        create_file_job_logfile = create_file_job.logfile_name(dbus_encode(''))
+        self.assertTrue(create_file_job_logfile)
+
+        modify_file_job_logfile = modify_file_job.logfile_name(dbus_encode(''))
+        self.assertTrue(modify_file_job_logfile)
+
+        delete_file_job_logfile = delete_file_job.logfile_name(dbus_encode(''))
+        self.assertTrue(delete_file_job_logfile)
+
+        create_dir_job_logfile = create_dir_job.logfile_name(dbus_encode(''))
+        self.assertTrue(create_dir_job_logfile)
+
+        modify_dir_job_logfile = modify_dir_job.logfile_name(dbus_encode(''))
+        self.assertTrue(modify_dir_job_logfile)
+
+        delete_dir_job_logfile = delete_dir_job.logfile_name(dbus_encode(''))
+        self.assertTrue(delete_dir_job_logfile)
+
+        #--------------------
+
+        # Wait for the create file job to run and produce output
+        self.assertTrue(wait_for_file(create_file_job_logfile))
+
+        # Check the output
+        with open(create_file_job_logfile, 'r', encoding='utf-8') as f:
+            lines = f.readlines()
+        self.assertTrue(len(lines) == 1)
+        self.assertEqual(file_msg, lines[0].rstrip())
+
+        #--------------------
+
+        # Wait for the create directory job to run and produce output
+        self.assertTrue(wait_for_file(create_dir_job_logfile))
+
+        # Check the output
+        with open(create_dir_job_logfile, 'r', encoding='utf-8') as f:
+            lines = f.readlines()
+        self.assertTrue(len(lines) == 1)
+        self.assertEqual(dir_msg, lines[0].rstrip())
+
+        #--------------------
+
+        # Modify the file
+        open(file, 'w').close()
+
+        # Wait for the create file job to run and produce output
+        self.assertTrue(wait_for_file(modify_file_job_logfile))
+
+        # Check the output
+        with open(modify_file_job_logfile, 'r', encoding='utf-8') as f:
+            lines = f.readlines()
+        self.assertTrue(len(lines) == 1)
+        self.assertEqual(file_msg, lines[0].rstrip())
+
+        #--------------------
+        # Modify the directory by creating a new file in it.
+
+        dir_file = dir + os.sep + 'baz'
+        open(dir_file, 'w').close()
+
+        # Wait for the modify directory job to run and produce output
+        self.assertTrue(wait_for_file(modify_dir_job_logfile))
+
+        # Check the output
+        with open(modify_dir_job_logfile, 'r', encoding='utf-8') as f:
+            lines = f.readlines()
+        self.assertTrue(len(lines) == 1)
+        self.assertEqual(dir_msg, lines[0].rstrip())
+
+        #--------------------
+
+        os.remove(dir_file)
+        os.rmdir(dir)
+
+        # Wait for the delete directory job to run and produce output
+        self.assertTrue(wait_for_file(delete_dir_job_logfile))
+
+        # Check the output
+        with open(delete_dir_job_logfile, 'r', encoding='utf-8') as f:
+            lines = f.readlines()
+        self.assertTrue(len(lines) == 1)
+        self.assertEqual(dir_msg, lines[0].rstrip())
+        #--------------------
+
+        shutil.rmtree(target_dir)
 
         # Wait for the delete job to run and produce output
-        self.assertTrue(wait_for_file(delete_job_logfile))
+        self.assertTrue(wait_for_file(delete_file_job_logfile))
 
         # Check the output
-        with open(delete_job_logfile, 'r', encoding='utf-8') as f:
+        with open(delete_file_job_logfile, 'r', encoding='utf-8') as f:
             lines = f.readlines()
         self.assertTrue(len(lines) == 1)
-        self.assertEqual(msg, lines[0].rstrip())
-
-        os.remove(create_job_logfile)
-        os.remove(delete_job_logfile)
+        self.assertEqual(file_msg, lines[0].rstrip())
+
+        #--------------------
+
+        os.remove(create_file_job_logfile)
+        os.remove(modify_file_job_logfile)
+        os.remove(delete_file_job_logfile)
+        os.remove(create_dir_job_logfile)
+        os.remove(modify_dir_job_logfile)
+        os.remove(delete_dir_job_logfile)
 
         file_bridge.stop()
         self.stop_session_init()