Your message dated Sun, 25 Dec 2016 12:41:02 +0100
with message-id <e6de7333-8f77-0942-7ef1-fd8a94920...@iwakd.de>
and subject line Re: init-system-helpers: deb-systemd-helper doesn't handle
drop-ins and is inconsistent w/ systemd w.r.t. lists
has caused the Debian Bug report #780522,
regarding init-system-helpers: deb-systemd-helper doesn't handle drop-ins and
is inconsistent w/ systemd w.r.t. lists
to be marked as done.
This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.
(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact ow...@bugs.debian.org
immediately.)
--
780522: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=780522
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems
--- Begin Message ---
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,
--- End Message ---
--- Begin Message ---
Version: 1.34
Control: tags -1 - moreinfo
On 12/21/2016 01:53 PM, Felipe Sateler wrote:
> Control: tags -1 moreinfo
> On Sun, 15 Mar 2015 15:23:38 +0100 Christian Seiler <christ...@iwakd.de>
> wrote:
>> Package: init-system-helpers
>> Version: 1.22
>> Severity: important
>> Tags: patch
>>
>> 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).
>
> So, lets recap. Upstream considers ignoring dropins in the [Install]
> section a feature. I think we should follow upstream here (if they
> change we should also change of course). Given that, there is only
> minor benefit to handling empty lists (it's unlikely a single file
> will reset a list defined the previous line). However, handling
> multiple words per line is important. However, since the fix in
> #820359 landed, I think we should be OK here.
>
> So, should this be marked as done, or am I missing something more your
> patch does?
Yes, you're right. I was hoping to be able to work on a patch for
upstream to support drop-ins in this case, but I didn't get to
that.
Since I agree that this should follow upstream, I'm closing this
bug report for now. Should I be able to create a patch for upstream
that does this and get upstream to accept this, I'll reopen this
bug report.
Regards,
Christian
--- End Message ---
_______________________________________________
Pkg-systemd-maintainers mailing list
Pkg-systemd-maintainers@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-systemd-maintainers
