Package: init-system-helpers
Version: 1.22
Severity: important
Tags: patch
Dear Maintainers,
deb-systemd-helper doesn't handle drop-ins at all (only the main
service file is parsed) and it doesn't treat lists properly: when
creating links for Alias=, it doesn't properly treat multiple entries
in the same line ($1 instead of $_ used), when removing links for
Also=, it assumes that there's only ever one entry per line, and it
doesn't support systemd's syntax to reset a list to empty (which native
systemd supports for the [Install] section entries).
Thus, a lot of possibilities for the administrator to customize unit
files are lost on Debian. Also, if there are units in Debian that
contain Alias=a.service b.service or Also=a.service b.service in one
line (instead of two separate entries), those also won't work properly.
(I haven't checked.)
I've attached a patch that fixes these issues: a new function to parse
the unit file + all drop-ins is added (parse_install_sections), which
handles list resets and multiple entries properly. This function is
then used everywhere where this information is needed.
Note that the patch follows systemd's logic completely by also looking
at the output directory of generators. This obviously will produce
different results if things in those directories contain these types of
settings and you compare executing this on a true system vs. a chroot
vs. the same systemd w/ sysvinit running, so it may be debatable as to
whether you'd want to include this or not. But upstream systemd
enable/disable will read that and will have the same issue. (i.e.
behaving differently in chroots if a drop-in in /run has an [Install]
section.) I don't mind much either way - if you don't like looking at
/run, remove the lines from @unit_paths.
Also, while I've tested this to some extent, I haven't done extensive
stress testing of these changes. I'm fairly certain it's correct the
way it is, but I have to admit I'm not a Perl guy, so you should
double-check this. I HAVE diverted deb-systemd-helper on my system
now and replaced it with this, so I'll hopefully notice mistakes.
Christian
-- System Information:
Debian Release: 8.0
APT prefers testing
APT policy: (500, 'testing'), (100, 'experimental'), (100,
'unstable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386
Kernel: Linux 3.16.0-4-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash
Init: systemd (via /run/systemd/system)
Versions of packages init-system-helpers depends on:
ii perl-base 5.20.2-2
init-system-helpers recommends no packages.
init-system-helpers suggests no packages.
-- no debconf information
diff -Nru init-system-helpers-1.22/script/deb-systemd-helper
init-system-helpers-1.22+bugfix1/script/deb-systemd-helper
--- init-system-helpers-1.22/script/deb-systemd-helper 2014-11-17
22:59:23.000000000 +0100
+++ init-system-helpers-1.22+bugfix1/script/deb-systemd-helper 2015-03-15
15:17:11.000000000 +0100
@@ -95,6 +95,15 @@
my $enabled_state_dir = '/var/lib/systemd/deb-systemd-helper-enabled';
my $masked_state_dir = '/var/lib/systemd/deb-systemd-helper-masked';
+# in order of proper precedence where to find unit files
+my @unit_paths = (
+ "/run/systemd/generator.late",
+ "/lib/systemd/system",
+ "/run/systemd/generator",
+ "/etc/systemd/system",
+ "/run/systemd/generator.early"
+);
+
# Globals are bad, but in this specific case, it really makes things much
# easier to write and understand.
my $changed_sth;
@@ -118,12 +127,63 @@
my ($scriptname) = @_;
my $service_path = $scriptname;
- if (-f "/etc/systemd/system/$scriptname") {
- $service_path = "/etc/systemd/system/$scriptname";
- } elsif (-f "/lib/systemd/system/$scriptname") {
- $service_path = "/lib/systemd/system/$scriptname";
+
+ foreach (reverse @unit_paths) {
+ if (-f "$_/$service_path") {
+ return "$_/$service_path";
+ }
+ }
+
+ error("unable to find unit file $service_path");
+}
+
+sub find_dropins {
+ my ($scriptname) = @_;
+
+ my %dropins;
+
+ foreach (@unit_paths) {
+ foreach (<$_/$scriptname.d/*.conf>) {
+ $dropins{basename($_)} = $_;
+ }
+ }
+
+ return sort values %dropins;
+}
+
+sub parse_install_sections {
+ my ($scriptname) = @_;
+
+ my $service_path = find_unit($scriptname);
+ my @dropins = find_dropins($scriptname);
+
+ my $result = {
+ 'WantedBy' => [],
+ 'RequiredBy' => [],
+ 'Also' => [],
+ 'Alias' => []
+ };
+
+ foreach ($service_path, @dropins) {
+ open my $fh, '<', $_ or error("unable to read $_");
+ while (my $line = <$fh>) {
+ chomp($line);
+ if ($line =~ /^\s*(WantedBy|RequiredBy|Also|Alias)=(.*)$/i) {
+ my @values = shellwords($2);
+
+ # systemd will reset a list to empty if it encounters an
+ # empty value
+ if (scalar (@values) == 0) {
+ $result->{$1} = [];
+ } else {
+ push(@{$result->{$1}}, @values);
+ }
+ }
+ }
+ close($fh);
}
- return $service_path;
+
+ return $result;
}
sub dsh_state_path {
@@ -181,33 +241,20 @@
my @links;
- open my $fh, '<', $service_path or error("unable to read $service_path");
- while (my $line = <$fh>) {
- chomp($line);
- my $service_link;
-
- if ($line =~ /^\s*(WantedBy|RequiredBy)=(.+)$/i) {
- for my $value (shellwords($2)) {
- my $wants_dir = "/etc/systemd/system/$value";
- $wants_dir .= '.wants' if $1 eq 'WantedBy';
- $wants_dir .= '.requires' if $1 eq 'RequiredBy';
- push @links, { dest => $service_path, src =>
"$wants_dir/$scriptname" };
- }
- }
+ my $unit_config = parse_install_sections($scriptname);
- if ($line =~ /^\s*Also=(.+)$/i) {
- for my $value (shellwords($1)) {
- push @links, get_link_closure($value, find_unit($value));
- }
- }
-
- if ($line =~ /^\s*Alias=(.+)$/i) {
- for my $value (shellwords($1)) {
- push @links, { dest => $service_path, src =>
"/etc/systemd/system/$1" };
- }
- }
+ for my $value (@{$unit_config->{'WantedBy'}}) {
+ push @links, { dest => $service_path, src =>
"/etc/systemd/system/$value.wants/$scriptname" };
+ }
+ for my $value (@{$unit_config->{'RequiredBy'}}) {
+ push @links, { dest => $service_path, src =>
"/etc/systemd/system/$value.requires/$scriptname" };
+ }
+ for my $value (@{$unit_config->{'Also'}}) {
+ push @links, get_link_closure($value, find_unit($value));
+ }
+ for my $value (@{$unit_config->{'Alias'}}) {
+ push @links, { dest => $service_path, src =>
"/etc/systemd/system/$value" };
}
- close($fh);
return @links;
}
@@ -342,16 +389,10 @@
# disabling actually worked or not â the case is handled by
# dh_installsystemd generating an appropriate disable
# command by parsing the service file at debhelper-time.
- open(my $fh, '<', $service_path) or return;
- while (my $line = <$fh>) {
- chomp($line);
- my $service_link;
-
- if ($line =~ /^\s*Also=(.+)$/i) {
- remove_links(find_unit($1));
- }
+ my $unit_config = parse_install_sections(basename($service_path));
+ for my $value (@{$unit_config->{'Also'}}) {
+ remove_links(find_unit($value));
}
- close($fh);
}
# Recursively deletes a directory structure, if all (!) components are empty,
