[systemd-devel] [PATCH] sysv-generator: Replace Provides: symlinks with real units

[Zbigniew Jędrzejewski-Szmek]

On Wed, Jan 21, 2015 at 10:46:03AM +0100, Martin Pitt wrote:
> Keeping track of which alias symlinks we actually want is error prone, and
> restricting the creation of services for enabled init.d scripts would reduce
> the utility of the generator (for manual starting disabled init.d scripts) as
> well as not cover the second case. So if we encounter an existing symlink, just
> remove it before writing a real unit.
Looks fine. Although the code is clearer than the description :)

> Note that two init.d scripts "foo" and "bar" which both provide the same name
> "common" already work. The first processed init script wins and creates the
> "common.service" symlink, and the second just fails to create the symlink
> again. Thus create an additional test case for this to ensure that it keeps
> working sensibly.
> 
> https://bugs.debian.org/775404
> ---
>  src/sysv-generator/sysv-generator.c | 12 ++++++++++++
>  test/sysv-generator-test.py         | 38 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c
> index a47b072..fd3ee20 100644
> --- a/src/sysv-generator/sysv-generator.c
> +++ b/src/sysv-generator/sysv-generator.c
> @@ -147,6 +147,7 @@ static int generate_unit_file(SysvStub *s) {
>          _cleanup_free_ char *wants = NULL;
>          _cleanup_free_ char *conflicts = NULL;
>          int r;
> +        struct stat st;
>  
>          before = strv_join(s->before, " ");
>          if (!before)
> @@ -168,6 +169,14 @@ static int generate_unit_file(SysvStub *s) {
>          if (!unit)
>                  return log_oom();
>  
> +        /* We might already have a symlink with the same name from a Provides:,
> +         * or from backup files like /etc/init.d/foo.bak. Real scripts always win,
> +         * so remove an existing link */
> +        if (lstat(unit, &st) == 0 && S_ISLNK(st.st_mode)) {
> +                log_warning("Overwriting existing symlink %s with real service", unit);
> +                unlink(unit);
> +        }
> +
>          f = fopen(unit, "wxe");
>          if (!f)
>                  return log_error_errno(errno, "Failed to create unit file %s: %m", unit);
> @@ -343,6 +352,8 @@ static int load_sysv(SysvStub *s) {
>          if (!f)
>                  return errno == ENOENT ? 0 : -errno;
>  
> +        log_debug("loading SysV script %s", s->path);
Capital "L"?

> +
>          while (!feof(f)) {
>                  char l[LINE_MAX], *t;
>  
> @@ -492,6 +503,7 @@ static int load_sysv(SysvStub *s) {
>                                                  continue;
>  
>                                          if (unit_name_to_type(m) == UNIT_SERVICE) {
> +                                                log_debug("Adding Provides: alias '%s' for '%s'", m, s->name);
>                                                  r = add_alias(s->name, m);
>                                          } else {
>                                                  /* NB: SysV targets
> diff --git a/test/sysv-generator-test.py b/test/sysv-generator-test.py
> index a3daa9f..63a10ec 100644
> --- a/test/sysv-generator-test.py
> +++ b/test/sysv-generator-test.py
> @@ -271,6 +271,25 @@ class SysvGeneratorTest(unittest.TestCase):
>              self.assertEqual(os.readlink(os.path.join(self.out_dir, f)),
>                               'foo.service')
>  
> +    def test_same_provides_in_multiple_scripts(self):
> +        '''multiple init.d scripts provide the same name'''
> +
> +        self.add_sysv('foo', {'Provides': 'foo common'}, enable=True, prio=1)
> +        self.add_sysv('bar', {'Provides': 'bar common'}, enable=True, prio=2)
> +        err, results = self.run_generator()
> +        self.assertEqual(sorted(results), ['bar.service', 'foo.service'])
> +        # should create symlink for the alternative name for either unit
> +        self.assertIn(os.readlink(os.path.join(self.out_dir, 'common.service')),
> +                      ['foo.service', 'bar.service'])
> +
> +    def test_provide_other_script(self):
> +        '''init.d scripts provides the name of another init.d script'''
> +
> +        self.add_sysv('foo', {'Provides': 'foo bar'}, enable=True)
> +        self.add_sysv('bar', {'Provides': 'bar'}, enable=True)
> +        err, results = self.run_generator()
> +        self.assertEqual(sorted(results), ['bar.service', 'foo.service'])
> +
>      def test_nonexecutable_script(self):
>          '''ignores non-executable init.d script'''
>  
> @@ -313,6 +332,25 @@ class SysvGeneratorTest(unittest.TestCase):
>          self.assertEqual(os.readlink(os.path.join(self.out_dir, 'bar.service')),
>                           'foo.service')
>  
> +    def test_backup_file(self):
> +        '''init.d script with backup file'''
> +
> +        script = self.add_sysv('foo', {}, enable=True)
> +        # backup files (not enabled in rcN.d/)
> +        shutil.copy(script, script + '.bak')
> +        shutil.copy(script, script + '.old')
> +
> +        err, results = self.run_generator()
> +        print(err)
> +        self.assertEqual(sorted(results),
> +                         ['foo.bak.service', 'foo.old.service', 'foo.service'])
> +
> +        # ensure we don't try to create a symlink to itself
> +        self.assertNotIn(err, 'itself')
> +
> +        self.assert_enabled('foo.service', [2, 3, 4, 5])
> +        self.assert_enabled('foo.bak.service', [])
> +        self.assert_enabled('foo.old.service', [])
Looks fine from my POV.

Zbyszek