Hello all, while working on the sysv generator, some more cases came up where the recently introduced "Provides:" symlink handling [1] causes trouble [2]. As soon as you have backup files like /etc/init.d/foo.bak, you'll get a "foo.service -> foo.bak.service" link which prevents the creation of a real foo.service unit. A similar case can also happen if one init.d script Provides: the name of another init.d script (arguably this is at least questionable, but it might happen in practice -- e. g. /etc/init.d/mariad might very well "Provides: mysql" as it's kind of a drop-in replacement).
I wrote some more tests which reproduce these failures, and a proposed patch. It's not exactly nice due to the TOCTOU (which shouldn't cause any practical problem though, it's just a bit unclean), but I can't think of a better solution which covers all corner cases. Details are in the git commit message. Note that this currently adds two log_debug() statements, so that you can better see what's going on (and wrong) in the output for test failures. If you don't want to keep them, I'm fine with dropping them again of course. Opinions? (Especially from Thomas?) Thanks, Martin [1] http://cgit.freedesktop.org/systemd/systemd/commit/?id=b7e7184634d5 [2] https://bugs.debian.org/775404 -- Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
From cb5c1727a62df698fb69bb2ab1fbd634f2283a0b Mon Sep 17 00:00:00 2001 From: Martin Pitt <martin.p...@ubuntu.com> Date: Wed, 21 Jan 2015 10:25:14 +0100 Subject: [PATCH 2/2] sysv-generator: Replace Provides: symlinks with real units Since commit b7e7184 the SysV generator creates symlinks for all "Provides:" in the LSB header. However, this is too greedy; there are cases where the creation of a unit .service file fails because of an already existing symlink with the same name: - Backup files such as /etc/init.d/foo.bak still have "Provides: foo", and thus get a foo.service -> foo.bak.service link. foo.bak would not be enabled in rcN.d/, but we (deliberately) create units for all executables in init.d/ so that a manual "systemctl start" works. If foo.bak is processed before, the symlink already exists. - init.d/bar has "Provides: foo", while there also is a real init.d/foo. The former would create a link foo.service -> bar.service, while the latter would fail to create the real foo.service. 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. 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); + 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', []) if __name__ == '__main__': -- 2.1.4
signature.asc
Description: Digital signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
