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

James Hunt james.hunt at canonical.com
Mon Jan 7 17:02:21 UTC 2013


Review: Needs Fixing

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.
-- 
https://code.launchpad.net/~xnox/upstart/overrides/+merge/141914
Your team Upstart Reviewers is subscribed to branch lp:upstart.



More information about the upstart-devel mailing list