Dmitrijs Ledkovs has proposed merging lp:~xnox/upstart/overrides into lp:upstart.
Requested reviews: Upstart Reviewers (upstart-reviewers) For more details, see: https://code.launchpad.net/~xnox/upstart/overrides/+merge/141914 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 -- https://code.launchpad.net/~xnox/upstart/overrides/+merge/141914 Your team Upstart Reviewers is requested to review the proposed merge of lp:~xnox/upstart/overrides into lp:upstart.
=== modified file 'ChangeLog' --- ChangeLog 2012-12-18 09:02:24 +0000 +++ ChangeLog 2013-01-04 13:40:29 +0000 @@ -1,3 +1,11 @@ +2013-01-04 Dmitrijs Ledkovs <[email protected]> + + * 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 <[email protected]> * 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-04 13:40:29 +0000 @@ -2,7 +2,7 @@ * * conf.c - configuration management * - * Copyright © 2009,2010,2011 Canonical Ltd. + * Copyright © 2009,2010,2011,2012 Canonical Ltd. * Author: Scott James Remnant <[email protected]>. * * 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,97 @@ } /** + * conf_to_job_name: + * @source_path: path to ConfigSource + * @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 then 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 & finds the + * first applicable override file. It will not look for override file + * beyond the @conf_file's ConfigSource. + * + * 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_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. * @@ -670,6 +775,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; + } + if (override_path) + 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); + } +} + + +/** * conf_create_modify_handler: * @source: configuration source, * @watch: NihWatch for source, @@ -693,85 +858,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_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 +948,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 overide (%s)", + path); + conf_create_modify_handler (source, watch, path, NULL); } /** @@ -867,64 +983,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 +1027,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 +1083,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 +1442,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-04 13:40:29 +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-04 13:40:29 +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; }
-- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
