Merge lp:~xnox/upstart/overrides into lp:upstart

[Dimitri John Ledkov]

This branch implements override files from any directory as specified here [1].

Instead of checking for a known path to .override file, a helper function `conf_get_best_override' is used that iterates across conf_sources and looks for the first .override file.

Next, I refactored another helper function conf_load_path_with_override that: loads a conf file, finds best override file for it & if found, applies it.

For the handlers we now have the following logic:
- if a .conf file is created/modified: simply call conf_load_path_with_override.
- if a .override file is created/modified/deleted: iterate across all conf_sources, check for "matching" .conf file and reload it with conf_load_path_with_override.
- if a .conf file is deleted: the behaviour is the same, unref the file from the ConfSource.

For system init, there shouldn't be much performance impact since there is only one directory to do lookups in.

For session init, most jobs will be in the lowest priority config_source. Thus resulting in lstat call in each config_source for each job to check for an .override file. But in practice, those higher priority config_sources may not even exist and lstat calls on non-existing directories is cheap. Potentially this can be micro-optimised by storing hashes of all found .override files while walking the config_sources and then instead of lstat check do a hash lookup...

[1] https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#Configuration_Files_for_User_Jobs

[James Hunt (jamesodhunt)]

Hi Dmitrijs,

* Changelog: Need extra indent for 'priority' and 'correctly'.
* init/conf.c:
  - Might just as well add ',2013' to copyright now :)
  - conf_to_job_name():
    - Typo: 'ConfigSource' should be 'ConfSource'.
    - Typo: 'from then' should be 'from the'.
    - You could drop the braces for the final if/else test, but they
      were there originally I guess :)
  - conf_get_best_override():
    - Typo: '& finds the' should be '& find the'.
    - Typo: 'for override file' should be 'for override files'.
    - Typo: 'ConfigSource' should be 'ConfSource'.
    - @name and @last_source are not asserted.
    - Using lstat(2) seems correct since we don't support symlinks on
      purpose, but conf_source_reload_file() is still using stat(2) so
      there is a discrepancy.
  - conf_load_path_with_override():
    - Need space around '=' in initial assignments.
    - 'if (override_path)' at line 818 technically redundant since we already know it
      cannot be NULL.
  - conf_create_modify_handler():
    - Missing check on return from nih_sprintf().
  - conf_delete_handler():
    - Typo: 'overide' rather than 'override'.

Regarding your comments on the lstat issue, we need to keep an eye on this. As you say, the impact should not be great, but it would still be worth performing some tests on a slow system to see if the new code makes any appreciable difference to performance.

review: Needs Fixing

[lp:~xnox/upstart/overrides]

1424. By Dimitri John Ledkov on 2013-01-08

Review comments

[Dimitri John Ledkov (xnox)]

Addressed all comments.
Checking return status of nih_sprintf with NIH_MUST.
Converted conf_source_reload_file to use lstat as well.

[lp:~xnox/upstart/overrides]

1425. By Dimitri John Ledkov on 2013-01-08

;

[James Hunt (jamesodhunt)]

Thanks. LGTM - merged!

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2012-12-18 09:02:24 +0000
+++ ChangeLog	2013-01-08 16:18:21 +0000
@@ -1,3 +1,11 @@
[phone]  Dmitrijs Ledkovs  <[e-mail]>
+
+	* init/conf.c: add ability to apply override files from higher
+	  priority configuration sources.
+	* init/tests/test_conf.c: test that multiple override files are
+	  correctly applied and removed.
+	* init/tests/test_conf_static.c: test override file detection.
+
 2012-12-17  James Hunt  <[e-mail]>
 
 	* init/man/init.5: Document that User Jobs are not supported
 
=== modified file 'init/conf.c'
--- init/conf.c	2012-12-19 12:46:46 +0000
+++ init/conf.c	2013-01-08 16:18:21 +0000
@@ -2,7 +2,7 @@
  *
  * conf.c - configuration management
  *
- * Copyright © 2009,2010,2011 Canonical Ltd.
+ * Copyright © 2009,2010,2011,2012,2013 Canonical Ltd.
  * Author: Scott James Remnant <[e-mail]>.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -86,6 +86,14 @@
 static inline char *toggle_conf_name   (const void *parent, const char *path)
 	__attribute__ ((warn_unused_result, malloc));
 
+static inline char * conf_to_job_name  (const char *source_path,
+                                        const char *conf_path)
+	__attribute__ ((malloc, warn_unused_result));
+
+static char * conf_get_best_override   (const char *name,
+                                        const ConfSource *last_source)
+	__attribute__ ((malloc, warn_unused_result));
+
 /**
  * conf_sources:
  *
@@ -110,6 +118,8 @@
 static inline int
 is_conf_file_std (const char *path)
 {
+	nih_assert (path != NULL);
+
 	char *ptr = strrchr (path, '.');
 
 	if (ptr && IS_CONF_EXT_STD (ptr))
@@ -132,6 +142,8 @@
 static inline int
 is_conf_file_override (const char *path)
 {
+	nih_assert (path != NULL);
+
 	char *ptr = strrchr (path, '.');
 
 	if (ptr && IS_CONF_EXT_OVERRIDE (ptr))
@@ -154,6 +166,8 @@
 static inline int
 is_conf_file (const char *path)
 {
+	nih_assert (path != NULL);
+
 	char *ptr = strrchr (path, '.');
 
 	if (ptr && (ptr > path) && (ptr[-1] != '/') && IS_CONF_EXT (ptr))
@@ -163,6 +177,99 @@
 }
 
 /**
+ * conf_to_job_name:
+ * @source_path: path to ConfSource
+ * @conf_path: path to configuration file
+ *
+ * Constructs the job name for a given @conf_path. Removes
+ * @source_path directory name from the front of @conf_path and
+ * extension from the end.
+ *
+ * Returns: newly-allocated name.
+ *
+ **/
+static inline char *
+conf_to_job_name (const char * source_path, const char * conf_path)
+{
+	const char *start, *end;
+	char       *name = NULL;
+	int        source_len;
+
+	nih_assert (source_path != NULL);
+	nih_assert (conf_path != NULL);
+
+	start = conf_path;
+	source_len = strlen (source_path);
+
+	if (! strncmp (start, source_path, source_len))
+		start += source_len;
+
+	while (*start == '/')
+		start++;
+
+	end = strrchr (start, '.');
+	if (end && IS_CONF_EXT (end))
+		name = NIH_MUST (nih_strndup (NULL, start, end - start));
+	else
+		name = NIH_MUST (nih_strdup (NULL, start));
+
+	return name;
+}
+
+
+/**
+ * conf_get_best_override:
+ * @conf_file: conf_file object
+ *
+ * Given a @conf_file iterate over all config sources & find the
+ * first applicable override file. It will not look for override files
+ * beyond the @conf_file's ConfSource.
+ *
+ * Returns: newly allocated path to override file or NULL.
+ **/
+static char *
+conf_get_best_override (const char *name, const ConfSource *last_source)
+{
+	char         *try_path=NULL;
+	struct stat   statbuf;
+
+	nih_assert (name != NULL);
+	nih_assert (last_source != NULL);
+
+	NIH_LIST_FOREACH (conf_sources, iter) {
+
+		ConfSource *source = (ConfSource *)iter;
+
+		/* Look at directories only */
+		if (source->type == CONF_FILE)
+			continue;
+
+		/* Reclaim memory */
+		if (try_path)
+			nih_free (try_path);
+
+		/* construct path */
+		try_path = NIH_MUST (nih_sprintf (NULL, "%s/%s%s", source->path, name, CONF_EXT_OVERRIDE));
+
+		/* Found it! */
+		if (lstat (try_path, &statbuf) == 0 && S_ISREG (statbuf.st_mode))
+			return try_path;
+
+		/* Warning, you have reached the end of the conveyor!
+		 * Ignore overrides beyond .conf itself.
+		 */
+		if (source == last_source)
+			break;
+	}
+
+	if (try_path)
+		nih_free (try_path);
+
+	return NULL;
+}
+
+
+/**
  * Convert a configuration file name to an override file name and vice
  * versa.
  *
@@ -511,7 +618,7 @@
 
 	override_path = toggle_conf_name (NULL, source->path);
 
-	if (stat (override_path, &statbuf) != 0)
+	if (lstat (override_path, &statbuf) != 0)
 		return 0;
 
 	nih_debug ("Updating configuration for %s from %s",
@@ -670,6 +777,66 @@
 }
 
 /**
+ * conf_load_path_with_override:
+ * @source: configuration source
+ * @conf_path: path to config file
+ *
+ * Loads given @conf_path as a config file in a given @source. Then it
+ * finds an override file. If an override file is found it applies it
+ * as well.
+ **/
+static void
+conf_load_path_with_override (ConfSource *source,
+			      const char *conf_path)
+{
+	int                     ret = 0;
+	const char   *error_path = NULL;
+	char      *override_path = NULL;
+	nih_local char *job_name = NULL;
+
+	nih_assert (source != NULL);
+	nih_assert (conf_path != NULL);
+
+	/* reload conf file */
+	nih_debug ("Loading configuration file %s", conf_path);
+	ret = conf_reload_path (source, conf_path, NULL);
+	if (ret < 0) {
+		error_path = conf_path;
+		goto error;
+	}
+
+	job_name = conf_to_job_name (source->path, conf_path);
+	override_path = conf_get_best_override (job_name, source);
+	if (! override_path)
+		return;
+
+	/* overlay override settings */
+	nih_debug ("Loading override file %s for %s", conf_path, override_path);
+	ret = conf_reload_path (source, conf_path, override_path);
+	if (ret < 0) {
+		error_path = override_path;
+		goto error;
+	}
+	nih_free (override_path);
+	return;
+
+error:
+	{
+		NihError *err;
+
+		err = nih_error_get ();
+		nih_error ("%s: %s: %s", error_path,
+			   _("Error while loading configuration file"),
+			   err->message);
+		nih_free (err);
+		if (override_path)
+			nih_free (override_path);
+		return;
+	}
+}
+
+
+/**
  * conf_create_modify_handler:
  * @source: configuration source,
  * @watch: NihWatch for source,
@@ -693,85 +860,45 @@
 			    struct stat *statbuf)
 {
 	ConfFile *file = NULL;
-	const char *error_path = path;
-	nih_local char *new_path = NULL;
-	int ret;
+	char *config_path = NULL;
+	nih_local char *job_name = NULL;
 
 	nih_assert (source != NULL);
 	nih_assert (watch != NULL);
 	nih_assert (path != NULL);
-	nih_assert (statbuf != NULL);
 
 	/* note that symbolic links are ignored */
-	if (! S_ISREG (statbuf->st_mode))
-		return;
-
-	new_path = toggle_conf_name (NULL, path);
-	file = (ConfFile *)nih_hash_lookup (source->files, new_path);
-
-	if (is_conf_file_override (path)) {
-		if (! file) {
-			/* override file has no corresponding conf file */
-			nih_debug ("Ignoring orphan override file %s", path);
-			return;
-		}
-
-		/* reload conf file */
-		nih_debug ("Loading configuration file %s", new_path);
-		ret = conf_reload_path (source, new_path, NULL);
-		if (ret < 0) {
-			error_path = new_path;
-			goto error;
-		}
-
-		/* overlay override settings */
-		nih_debug ("Loading override file %s for %s", path, new_path);
-		ret = conf_reload_path (source, new_path, path);
-		if (ret < 0) {
-			error_path = path;
-			goto error;
-		}
-	} else {
-		nih_debug ("Loading configuration and override files for %s", path);
-
-		/* load conf file */
-		nih_debug ("Loading configuration file %s", path);
-		ret = conf_reload_path (source, path, NULL);
-		if (ret < 0) {
-			error_path = path;
-			goto error;
-		}
-
-		/* ensure we ignore directory changes (which won't have overrides. */
-		if (is_conf_file_std (path)) {
-			struct stat st;
-			if (stat (new_path, &st) == 0) {
-				/* overlay override settings */
-				nih_debug ("Loading override file %s for %s", new_path, path);
-				ret = conf_reload_path (source, path, new_path);
-				if (ret < 0) {
-					error_path = new_path;
-					goto error;
-				}
-			}
-
-		}
+	if (statbuf && ! S_ISREG (statbuf->st_mode))
+		return;
+
+	/* ignore non-config file changes */
+	if (! is_conf_file (path))
+		return;
+
+        /* For config file, load it and it's override file */
+	if (is_conf_file_std (path)) {
+		conf_load_path_with_override (source, path);
+		return;
+	}
+
+	/* For override files, reload all matching conf+override combos */
+	job_name = conf_to_job_name (source->path, path);
+	NIH_LIST_FOREACH (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+
+		if (source->type == CONF_FILE)
+			continue;
+
+		config_path = NIH_MUST (nih_sprintf (NULL, "%s/%s%s", source->path, job_name, CONF_EXT_STD));
+		file = (ConfFile *)nih_hash_lookup (source->files, config_path);
+		if (file) {
+			/* Find its override file and reload both */
+			conf_load_path_with_override (source, config_path);
+		}
+		nih_free (config_path);
 	}
 
 	return;
-
-error:
-	{
-		NihError *err;
-
-		err = nih_error_get ();
-		nih_error ("%s: %s: %s", error_path,
-				_("Error while loading configuration file"),
-				err->message);
-		nih_free (err);
-		if (file)
-			nih_unref (file, source);
-	}
 }
 
 /**
@@ -823,28 +950,19 @@
 
 	/* non-override files (and directories) are the simple case, so handle
 	 * them and leave.
-	 */ 
+	 */
 	if (! is_conf_file_override (path)) {
 		nih_unref (file, source);
 		return;
 	}
 
-	/* if an override file is deleted for which there is a corresponding
-	 * conf file, reload the conf file to remove any modifications
-	 * introduced by the override file.
+	/* Deleting override file is about the same as changing one.
+	 * We need to iterate across all matching jobs and reload them
+	 * with new "best" override file, if any.
 	 */
-	new_path = toggle_conf_name (NULL, path);
-	file = (ConfFile *)nih_hash_lookup (source->files, new_path);
-
-	if (file) {
-		nih_debug ("Reloading configuration for %s on deletion of overide (%s)",
-				new_path, path);
-
-		if ( conf_reload_path (source, new_path, NULL) < 0 ) {
-			nih_warn ("%s: %s", new_path,
-					_("Unable to reload configuration after override deletion"));
-		}
-	}
+	nih_debug ("Reloading configuration for matching configs on deletion of override (%s)",
+		   path);
+	conf_create_modify_handler (source, watch, path, NULL);
 }
 
 /**
@@ -867,64 +985,18 @@
 		   const char  *path,
 		   struct stat *statbuf)
 {
-	ConfFile *file = NULL;
-	nih_local char *new_path = NULL;
-
 	nih_assert (source != NULL);
 	nih_assert (dirname != NULL);
 	nih_assert (path != NULL);
 	nih_assert (statbuf != NULL);
 
-	/* We assume that CONF_EXT_STD files are visited before
-	 * CONF_EXT_OVERRIDE files. Happily, this assumption is currently
-	 * valid since CONF_EXT_STD comes before CONF_EXT_OVERRIDE if ordered
-	 * alphabetically.
-	 *
-	 * If this were ever to change (for example if we decided to
-	 * rename the CONF_EXT_OVERRIDE files to end in ".abc", say), the logic
-	 * in this function would be erroneous since it would never be possible when
-	 * visiting an override file (before a conf file) to lookup a conf file
-	 * in the hash, since the conf file would not yet have been seen and thus would
-	 * not exist in the hash (yet).
-	 */
-	nih_assert (CONF_EXT_STD[1] < CONF_EXT_OVERRIDE[1]);
-
 	if (! S_ISREG (statbuf->st_mode))
 		return 0;
 
-	if (is_conf_file_std (path)) {
-		if (conf_reload_path (source, path, NULL) < 0) {
-			NihError *err;
-
-			err = nih_error_get ();
-			nih_error ("%s: %s: %s", path,
-					_("Error while loading configuration file"),
-					err->message);
-			nih_free (err);
-		}
-		return 0;
-	}
-
-	new_path = toggle_conf_name (NULL, path);
-	file = (ConfFile *)nih_hash_lookup (source->files, new_path);
-
-	if (file) {
-		/* we're visiting an override file with an associated conf file that
-		 * has already been loaded, so just overlay the override file. If
-		 * there is no corresponding conf file, we ignore the override file.
-		 */
-		if (conf_reload_path (source, new_path, path) < 0) {
-			NihError *err;
-
-			err = nih_error_get ();
-			nih_error ("%s: %s: %s", new_path,
-					_("Error while reloading configuration file"),
-					err->message);
-			nih_free (err);
-		}
-	}
-
-	return 0;
+	if (is_conf_file_std (path))
+		conf_load_path_with_override (source, path);
+	
+	return 0;      
 }
 
 
@@ -957,7 +1029,6 @@
 {
 	ConfFile       *file = NULL;
 	nih_local char *buf = NULL;
-	const char     *start, *end;
 	nih_local char *name = NULL;
 	size_t          len, pos, lineno;
 	NihError       *err = NULL;
@@ -1014,23 +1085,8 @@
 
 		break;
 	case CONF_JOB_DIR:
-		/* Construct the job name by taking the path and removing
-		 * the directory name from the front and the extension
-		 * from the end.
-		 */
-		start = path;
-		if (! strncmp (start, source->path, strlen (source->path)))
-			start += strlen (source->path);
-
-		while (*start == '/')
-			start++;
-
-		end = strrchr (start, '.');
-		if (end && IS_CONF_EXT (end)) {
-			name = NIH_MUST (nih_strndup (NULL, start, end - start));
-		} else {
-			name = NIH_MUST (nih_strdup (NULL, start));
-		}
+
+		name = conf_to_job_name (source->path, path);
 
 		/* Create a new job item and parse the buffer to produce
 		 * the job definition.
@@ -1388,4 +1444,3 @@
 }
 
 #endif /* DEBUG */
-
 
=== modified file 'init/tests/test_conf.c'
--- init/tests/test_conf.c	2012-12-19 12:46:46 +0000
+++ init/tests/test_conf.c	2013-01-08 16:18:21 +0000
@@ -2424,7 +2424,8 @@
 	FILE       *f;
 	int         ret, fd[4096], i = 0;
 	char        dirname[PATH_MAX];
-	char        filename[PATH_MAX], override[PATH_MAX];
+	char        filename[PATH_MAX], override[PATH_MAX], override2[PATH_MAX];
+	char       *dir;
 	JobClass   *job;
 	NihError   *err;
 
@@ -3424,6 +3425,123 @@
 	unlink (filename);
 	TEST_EQ (rmdir (dirname), 0);
 
+	TEST_FEATURE ("create two watches, two overrides, conf, then delete override files one by one");
+	TEST_ENSURE_CLEAN_ENV ();
+	TEST_FILENAME (dirname);
+	TEST_EQ (mkdir (dirname, 0755), 0);
+
+	strcpy (override, dirname);
+	strcat (override, "/peter/foo.override");
+	strcpy (override2, dirname);
+	strcat (override2, "/paul/foo.override");
+	strcpy (filename, dirname);
+	strcat (filename, "/paul/foo.conf");
+
+	const char *sources[] = {"peter", "paul", NULL};
+
+	for (const char **src = sources; *src; src++) {
+		dir = nih_sprintf (NULL, "%s/%s", dirname, *src);
+		TEST_EQ (mkdir (dir, 0755), 0);
+		source = conf_source_new (NULL, dir, CONF_JOB_DIR);
+		TEST_NE_P (source, NULL);
+		ret = conf_source_reload (source);
+		TEST_EQ (ret, 0);
+		nih_free (dir);
+	}
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* create override */
+	f = fopen (override, "w");
+	TEST_NE_P (f, NULL);
+	fprintf (f, "manual\n");
+	fprintf (f, "author \"peter\"\n");
+	fclose (f);
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* create override */
+	f = fopen (override2, "w");
+	TEST_NE_P (f, NULL);
+	fprintf (f, "manual\n");
+	fprintf (f, "author \"paul\"\n");
+	fprintf (f, "env wibble=wobble\n");
+	fclose (f);
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* create conf */
+	f = fopen (filename, "w");
+	TEST_NE_P (f, NULL);
+	fprintf (f, "start on started\n");
+	fprintf (f, "emits hello\n");
+	fprintf (f, "author \"mary\"\n");
+	fclose (f);
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* ensure conf loaded */
+	file = (ConfFile *)nih_hash_lookup (source->files, filename);
+	TEST_NE_P (file, NULL);
+	job = (JobClass *)nih_hash_lookup (job_classes, "foo");
+	TEST_NE_P (job, NULL);
+	TEST_EQ_P (file->job, job);
+	TEST_EQ_STR ((job->emits)[0], "hello");
+
+	/* should pick up the top-priority override, *NOT* conf */
+	TEST_EQ_P (job->start_on, NULL);
+	TEST_EQ_STR (job->author, "peter");
+
+	/* delete override */
+	unlink (override);
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* ensure conf reloaded and updated with the second override */
+	file = (ConfFile *)nih_hash_lookup (source->files, filename);
+	TEST_NE_P (file, NULL);
+	job = (JobClass *)nih_hash_lookup (job_classes, "foo");
+	TEST_NE_P (job, NULL);
+	TEST_EQ_P (file->job, job);
+	TEST_EQ_STR ((job->emits)[0], "hello");
+
+	/* should pick up the second override, *NOT* conf */
+	TEST_EQ_P (job->start_on, NULL);
+	TEST_EQ_STR (job->author, "paul");
+	TEST_EQ_STR ((job->env)[0], "wibble=wobble");
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* delete override */
+	unlink (override2);
+
+	TEST_FORCE_WATCH_UPDATE();
+
+	/* ensure conf loaded */
+	file = (ConfFile *)nih_hash_lookup (source->files, filename);
+	TEST_NE_P (file, NULL);
+	job = (JobClass *)nih_hash_lookup (job_classes, "foo");
+	TEST_NE_P (job, NULL);
+	TEST_EQ_P (file->job, job);
+	TEST_EQ_STR (job->author, "mary");
+	TEST_EQ_STR ((job->emits)[0], "hello");
+	TEST_NE_P (job->start_on, NULL);
+	TEST_EQ_P (job->env, NULL);
+
+	unlink (filename);
+
+	for (const char **src = sources; *src; src++) {
+		dir = nih_sprintf (NULL, "%s/%s", dirname, *src);
+		TEST_EQ (rmdir (dir), 0);
+		nih_free (dir);
+	}
+
+	TEST_EQ (rmdir (dirname), 0);
+
+	NIH_LIST_FOREACH_SAFE (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		nih_free (source);
+	}
 
 	TEST_FEATURE ("create conf, watch, then create invalid override, delete override");
 	TEST_ENSURE_CLEAN_ENV ();
 
=== modified file 'init/tests/test_conf_static.c'
--- init/tests/test_conf_static.c	2012-12-19 12:46:46 +0000
+++ init/tests/test_conf_static.c	2013-01-08 16:18:21 +0000
@@ -34,8 +34,8 @@
 	char *f;
 	char *p;
 
-	TEST_FUNCTION_FEATURE ("toggle_conf_name",
-			"changing conf to override");
+	TEST_FUNCTION ("toggle_conf_name");
+	TEST_FEATURE ("changing conf to override");
 
 	TEST_FILENAME (dirname);
 	strcpy (filename, dirname);
@@ -71,11 +71,154 @@
 	nih_free (job);
 }
 
+void
+test_conf_to_job_name (void)
+{
+	char dirname[PATH_MAX];
+	char         *filename;
+	char             *name;
+
+	TEST_FUNCTION ("conf_to_job_name");
+	TEST_FEATURE ("with .conf file");
+	TEST_FILENAME (dirname);
+	filename = nih_sprintf (NULL, "%s/foo.conf", dirname);
+	name = conf_to_job_name (dirname, filename);
+	TEST_EQ_STR (name, "foo");
+	nih_free (filename);
+	nih_free (name);
+	
+	TEST_FEATURE ("with .override file");
+	filename = nih_sprintf (NULL, "%s/foo.override", dirname);
+	name = conf_to_job_name (dirname, filename);
+	TEST_EQ_STR (name, "foo");
+	nih_free (filename);
+	nih_free (name);
+
+	TEST_FEATURE ("with .conf in a sub-directory");
+	filename = nih_sprintf (NULL, "%s/foo/bar.conf", dirname);
+	name = conf_to_job_name (dirname, filename);
+	TEST_EQ_STR (name, "foo/bar");
+	nih_free (filename);
+	nih_free (name);
+	
+	TEST_FEATURE ("without extension");
+	filename = nih_sprintf (NULL, "%s/foo", dirname);
+	name = conf_to_job_name (dirname, filename);
+	TEST_EQ_STR (name, "foo");
+	nih_free (filename);
+	nih_free (name);
+
+}
+
+void
+test_conf_get_best_override (void)
+{
+	const char *sources[] = {"peter", "paul", "mary", NULL};
+	FILE                *f;
+	char dirname[PATH_MAX];
+	char              *dir;
+	char         *expected;
+	char             *path;
+
+	TEST_FUNCTION ("conf_get_best_override");
+
+	TEST_FILENAME (dirname);
+	mkdir (dirname, 0755);
+	
+	for (const char **src = sources; *src; src++) {
+		dir = nih_sprintf (NULL, "%s/%s", dirname, *src);
+		TEST_EQ (mkdir (dir, 0755), 0);
+		NIH_MUST (conf_source_new (NULL, dir, CONF_JOB_DIR));
+		nih_free (dir);
+	}
+
+	TEST_FEATURE ("with no overrides");
+	NIH_LIST_FOREACH (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		path = conf_get_best_override ("foo", source);
+		TEST_EQ_P (path, NULL);
+	}
+
+	TEST_FEATURE ("with single highest priority override");
+	expected = nih_sprintf (NULL, "%s/%s/foo.override", dirname, sources[0]);
+	f = fopen (expected, "w");
+	TEST_NE_P (f, NULL);
+	TEST_EQ (fclose (f), 0);
+
+	NIH_LIST_FOREACH (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		path = conf_get_best_override ("foo", source);
+		TEST_EQ_STR (path, expected);
+		nih_free (path);
+	}	
+	TEST_EQ (unlink (expected), 0);
+
+	TEST_FEATURE ("with single middle priority override");
+	expected = nih_sprintf (NULL, "%s/%s/foo.override", dirname, sources[1]);
+	f = fopen (expected, "w");
+	TEST_NE_P (f, NULL);
+	TEST_EQ (fclose (f), 0);
+
+	NIH_LIST_FOREACH (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		path = conf_get_best_override ("foo", source);
+
+		/* Poor-man's basename(1) */
+		dir = conf_to_job_name (dirname, source->path);
+		if (strcmp (dir, sources[0]) == 0) {
+			TEST_EQ_P (path, NULL);
+		} else {
+			TEST_EQ_STR (path, expected);
+			nih_free (path);
+		}
+		nih_free (dir);
+	}	
+	TEST_EQ (unlink (expected), 0);
+
+	TEST_FEATURE ("with single lowest priority override");
+	expected = nih_sprintf (NULL, "%s/%s/foo.override", dirname, sources[2]);
+	f = fopen (expected, "w");
+	TEST_NE_P (f, NULL);
+	TEST_EQ (fclose (f), 0);
+
+	NIH_LIST_FOREACH (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		path = conf_get_best_override ("foo", source);
+
+		/* Poor-man's basename(1) */
+		dir = conf_to_job_name (dirname, source->path);
+		if (strcmp (dir, sources[2]) == 0) {
+			TEST_EQ_STR (path, expected);
+			nih_free (path);
+		} else {
+			TEST_EQ_P (path, NULL);
+		}
+		nih_free (dir);
+	}	
+	TEST_EQ (unlink (expected), 0);
+
+	/* Clean up */
+	for (const char **src = sources; *src; src++) {
+		dir = nih_sprintf (NULL, "%s/%s", dirname, *src);
+		TEST_EQ (rmdir (dir), 0);
+		nih_free (dir);
+	}
+
+	TEST_EQ (rmdir (dirname), 0);
+
+	NIH_LIST_FOREACH_SAFE (conf_sources, iter) {
+		ConfSource *source = (ConfSource *)iter;
+		nih_free (source);
+	}
+}
+
 int
 main (int   argc,
       char *argv[])
 {
 	test_toggle_conf_name ();
+	test_conf_to_job_name ();
+	test_conf_get_best_override ();
 
 	return 0;
 }