On 29 June 2015 at 15:39, Felipe Sateler <fsate...@debian.org> wrote:
> Hi Niels,
>
> On 28 June 2015 at 08:50, Niels Thykier <ni...@thykier.net> wrote:
>> On 2015-06-28 03:09, Felipe Sateler wrote:
>>> Package: lintian
>>> Version: 2.5.32
>>> Severity: wishlist
>>> Tags: patch
>>>
>>> Hi,
>>>
>>> Please find attached a patch that does $subject. I have taken the
>>> liberty to refactor the code a bit in order to stop tagging multiple
>>> times for the same error.
>>>
>>> Patches 1-3 are the refactoring, patch 4 is the new check. There is a
>>> test for the new check.
>>>
>>
>> Hi,
>>
>> Thanks for working on this. I have a couple of remarks to some of the
>> patches (interleaved into the patches below).
>>
>>> I'm wondering if tag systemd-no-service-for-init-script should be
>>> lowered in severity but added inconditionally... but that is a separate
>>> issue. If/when this patch is merged, I can provide a patch for changing
>>> that so we can discuss that.
>>>
>>
>> Ok. :)
>>
>>> [...]
>>>
>>> 0001-Reorder-systemd-checks.patch
>>>
>>>
>>>>From 1c4ad47fead2a6d32b5fdc6888ba7b5333804bcb Mon Sep 17 00:00:00 2001
>>> From: Felipe Sateler <fsate...@debian.org>
>>> Date: Sat, 27 Jun 2015 16:32:42 -0300
>>> Subject: [PATCH 1/4] Reorder systemd checks
>>>
>>> This reorder groups most checks inside the corresponding check_*
>>> ---
>>> checks/systemd.pm | 140
>>> ++++++++++++++++++++++++++++++------------------------
>>> 1 file changed, 79 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/checks/systemd.pm b/checks/systemd.pm
>>> index d36cf65..4a45b49 100644
>>> --- a/checks/systemd.pm
>>> +++ b/checks/systemd.pm
>>> [...]
>>> +sub get_systemd_service_files {
>>> + my $info = shift @_;
>>
>> Please use the "my ($info) = @_;" notation instead.
>>
>> The use of "shift" is generally discouraged throughout the lintian code
>> base (I think we
>
> ... the suspense is killing me ;). Removed all references to shift.
But not all in the first commit :/. New version fixes that.
>
>>
>>> +
>>> + return grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
>>> +}
>>> +
>>> +sub get_systemd_service_names {
>>> + my ($info) = @_;
>>> + my %services;
>>> +
>>> + [...]
>>> + return %services;
>>> +}
>>
>> Please consider returning this as a ref. Since it is passed around, we
>> end up copying it several times. Admittedly, I suspect it is a small
>> hash, I am mostly in it for being consistent with similar usage for
>> larger hashes.
>>
>> Quick cheat-sheet
>>
>> return \%services;
>> $services{foo} => $services->{foo}
>> if (%foo) => if (%{$foo})
>
> Thanks, I got quite dizzy trying to understand references. Fixed this
>
>>
>>> +
>>> sub check_systemd_service_file {
>>> my ($info, $file) = @_;
>>>
>>> + tag 'systemd-service-file-outside-lib', $file if ($file =~
>>> m,^etc/systemd/system/,);
>>> + tag 'systemd-service-file-outside-lib', $file if ($file =~
>>> m,^usr/lib/systemd/system/,);
>>
>> Note this will match non-files (including the etc/systemd/system
>> directory). If the systemd package ships that as an empty dir (or
>> containing a README).
>>
>> Though presumably something already filters this out before we get that far?
>
> Yes, this sub is only called for files returned by get_systemd_service_files.
>
>>
>>> +
>>
>> An "empty" whitespace line - please apply "perltidy -it=4 -b
>> checks/systemd.pm" (it will possibly reformat other things as well).
I forgot this part. Attached a new patchset applying perltidy on each commit.
--
Saludos,
Felipe Sateler
From bcea42d3db13d7f5f24405a2fd37e5d0d2579fd9 Mon Sep 17 00:00:00 2001
From: Felipe Sateler <fsate...@debian.org>
Date: Sat, 27 Jun 2015 16:32:42 -0300
Subject: [PATCH 1/4] Reorder systemd checks
This reorder groups most checks inside the corresponding check_*
---
checks/systemd.pm | 138 ++++++++++++++++++++++++++++++------------------------
1 file changed, 78 insertions(+), 60 deletions(-)
diff --git a/checks/systemd.pm b/checks/systemd.pm
index d36cf65..5815827 100644
--- a/checks/systemd.pm
+++ b/checks/systemd.pm
@@ -37,81 +37,64 @@ use Lintian::Util qw(fail lstrip rstrip);
sub run {
my (undef, undef, $info) = @_;
- # Figure out whether the maintainer of this package did any effort to
- # make the package work with systemd. If not, we will not warn in case
- # of an init script that has no systemd equivalent, for example.
- my $ships_systemd_file = any { m,/systemd/, } $info->sorted_index;
-
- # An array of names which are provided by the service files.
- # This includes Alias= directives, so after parsing
- # NetworkManager.service, it will contain NetworkManager and
- # network-manager.
- my @systemd_targets;
-
+ # non-service checks
for my $file ($info->sorted_index) {
if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
tag 'systemd-tmpfiles.d-outside-usr-lib', $file;
}
- if ($file =~ m,^etc/systemd/system/.*\.service$,) {
- tag 'systemd-service-file-outside-lib', $file;
- }
- if ($file =~ m,^usr/lib/systemd/system/.*\.service$,) {
- tag 'systemd-service-file-outside-lib', $file;
- }
- if ($file =~ m,/systemd/system/.*\.service$,) {
- check_systemd_service_file($info, $file);
- for my $name (extract_service_file_names($info, $file)) {
- push @systemd_targets, $name;
- }
- }
}
- my @init_scripts = grep { m,^etc/init\.d/.+, } $info->sorted_index;
+ my @init_scripts = get_init_scripts($info);
+ my @service_files = get_systemd_service_files($info);
- # Verify that each init script includes /lib/lsb/init-functions,
- # because that is where the systemd diversion happens.
- for my $init_script (@init_scripts) {
- check_init_script($info, $init_script);
- }
+ # A hash of names reference which are provided by the service files.
+ # This includes Alias= directives, so after parsing
+ # NetworkManager.service, it will contain NetworkManager and
+ # network-manager.
+ my $services = get_systemd_service_names($info);
- @init_scripts = map { basename($_) } @init_scripts;
+ for my $script (@init_scripts) {
+ check_init_script($info, $script, $services);
+ }
- if ($ships_systemd_file) {
- for my $init_script (@init_scripts) {
- tag 'systemd-no-service-for-init-script', $init_script
- unless any { m/\Q$init_script\E\.service/ } @systemd_targets;
- }
+ for my $service (@service_files) {
+ check_systemd_service_file($info, $service);
}
check_maintainer_scripts($info);
return;
}
+sub get_init_scripts {
+ my ($info) = @_;
+ my @ignore = ('README','skeleton','rc','rcS',);
+ my @scripts;
+ if (my $initd_path = $info->index_resolved_path('etc/init.d/')) {
+ for my $init_script ($initd_path->children) {
+ next if any { $_ eq $init_script->basename } @ignore;
+ next
+ if $init_script->is_symlink
+ && $init_script->link eq '/lib/init/upstart-job';
+
+ push(@scripts, $init_script);
+ }
+ }
+ return @scripts;
+}
+
+# Verify that each init script includes /lib/lsb/init-functions,
+# because that is where the systemd diversion happens.
sub check_init_script {
- my ($info, $file) = @_;
+ my ($info, $file, $services) = @_;
my $basename = $file->basename;
my $lsb_source_seen;
- # Couple of special cases we don't care about...
- return
- if $basename eq 'README'
- or $basename eq 'skeleton'
- or $basename eq 'rc'
- or $basename eq 'rcS';
-
- if ($file->is_symlink) {
- # We cannot test upstart-jobs
- return if $file->link eq '/lib/init/upstart-job';
- }
-
if (!$file->is_regular_file) {
unless ($file->is_open_ok) {
tag 'init-script-is-not-a-file', $file;
return;
}
-
}
-
my $fh = $file->open;
while (<$fh>) {
lstrip;
@@ -127,15 +110,57 @@ sub check_init_script {
}
close($fh);
- if (!$lsb_source_seen) {
- tag 'init.d-script-does-not-source-init-functions', $file;
- }
+ tag 'init.d-script-does-not-source-init-functions', $file
+ unless $lsb_source_seen;
+ # Only tag if the maintainer of this package did any effort to
+ # make the package work with systemd.
+ tag 'systemd-no-service-for-init-script', $basename
+ if (%{$services} and !$services->{$basename});
return;
}
+sub get_systemd_service_files {
+ my ($info) = @_;
+
+ return grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
+}
+
+sub get_systemd_service_names {
+ my ($info) = @_;
+ my %services;
+
+ my $safe_add_service = sub {
+ my ($name, $file) = @_;
+ if (exists $services{$name}) {
+ # should add a tag here
+ return;
+ }
+ $services{$name} = 1;
+ };
+
+ for my $file (get_systemd_service_files($info)) {
+ my $name = $file->basename;
+ $name =~ s/\.service$//;
+ $safe_add_service->($name, $file);
+
+ my @aliases
+ = extract_service_file_values($info, $file, 'Install', 'Alias');
+
+ for my $alias (@aliases) {
+ $safe_add_service->($alias, $file);
+ }
+ }
+ return \%services;
+}
+
sub check_systemd_service_file {
my ($info, $file) = @_;
+ tag 'systemd-service-file-outside-lib', $file
+ if ($file =~ m,^etc/systemd/system/,);
+ tag 'systemd-service-file-outside-lib', $file
+ if ($file =~ m,^usr/lib/systemd/system/,);
+
my @values = extract_service_file_values($info, $file, 'Unit', 'After');
my @obsolete = grep { /^(?:syslog|dbus)\.target$/ } @values;
tag 'systemd-service-file-refers-to-obsolete-target', $file, $_
@@ -236,13 +261,6 @@ sub extract_service_file_values {
return @values;
}
-sub extract_service_file_names {
- my ($info, $file) = @_;
-
- my @aliases= extract_service_file_values($info, $file, 'Install', 'Alias');
- return (basename($file), @aliases);
-}
-
sub check_maintainer_scripts {
my ($info) = @_;
--
2.1.4
From 8e2490138ff72a74d84365c7e90ca4c2bf6f2040 Mon Sep 17 00:00:00 2001
From: Felipe Sateler <fsate...@debian.org>
Date: Sat, 27 Jun 2015 21:56:11 -0300
Subject: [PATCH 2/4] Check files as we detect them, and discard invalid files
prevents duplicate service-file-is-not-a-file
---
checks/systemd.pm | 30 ++++++++++++++++--------------
t/tests/systemd-general/tags | 1 -
2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/checks/systemd.pm b/checks/systemd.pm
index 5815827..99a286f 100644
--- a/checks/systemd.pm
+++ b/checks/systemd.pm
@@ -51,16 +51,12 @@ sub run {
# This includes Alias= directives, so after parsing
# NetworkManager.service, it will contain NetworkManager and
# network-manager.
- my $services = get_systemd_service_names($info);
+ my $services = get_systemd_service_names($info, \@service_files);
for my $script (@init_scripts) {
check_init_script($info, $script, $services);
}
- for my $service (@service_files) {
- check_systemd_service_file($info, $service);
- }
-
check_maintainer_scripts($info);
return;
}
@@ -121,12 +117,18 @@ sub check_init_script {
sub get_systemd_service_files {
my ($info) = @_;
+ my @res;
+ my @potential
+ = grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
- return grep { m,/systemd/system/.*\.service$, } $info->sorted_index;
+ for my $file (@potential) {
+ push(@res, $file) if check_systemd_service_file($info, $file);
+ }
+ return @res;
}
sub get_systemd_service_names {
- my ($info) = @_;
+ my ($info,$files_ref) = @_;
my %services;
my $safe_add_service = sub {
@@ -138,7 +140,7 @@ sub get_systemd_service_names {
$services{$name} = 1;
};
- for my $file (get_systemd_service_files($info)) {
+ for my $file (@{$files_ref}) {
my $name = $file->basename;
$name =~ s/\.service$//;
$safe_add_service->($name, $file);
@@ -161,11 +163,16 @@ sub check_systemd_service_file {
tag 'systemd-service-file-outside-lib', $file
if ($file =~ m,^usr/lib/systemd/system/,);
+ unless ($file->is_open_ok
+ || ($file->is_symlink && $file->link eq '/dev/null')) {
+ tag 'service-file-is-not-a-file', $file;
+ return 0;
+ }
my @values = extract_service_file_values($info, $file, 'Unit', 'After');
my @obsolete = grep { /^(?:syslog|dbus)\.target$/ } @values;
tag 'systemd-service-file-refers-to-obsolete-target', $file, $_
for @obsolete;
- return;
+ return 1;
}
sub service_file_lines {
@@ -207,11 +214,6 @@ sub extract_service_file_values {
my (@values, $section);
- unless ($file->is_open_ok
- || ($file->is_symlink && $file->link eq '/dev/null')) {
- tag 'service-file-is-not-a-file', $file;
- return;
- }
my @lines = service_file_lines($file);
my $key_ws = first_index { /^[[:alnum:]]+(\s*=\s|\s+=)/ } @lines;
if ($key_ws > -1) {
diff --git a/t/tests/systemd-general/tags b/t/tests/systemd-general/tags
index 47fe757..3223f6a 100644
--- a/t/tests/systemd-general/tags
+++ b/t/tests/systemd-general/tags
@@ -1,6 +1,5 @@
E: systemd-general: init-script-is-not-a-file etc/init.d/fifo-pipe-as-init
E: systemd-general: service-file-is-not-a-file etc/systemd/system/fifo-pipe-as-init.service
-E: systemd-general: service-file-is-not-a-file etc/systemd/system/fifo-pipe-as-init.service
E: systemd-general: service-key-has-whitespace etc/systemd/system/test.service at line 3
E: systemd-general: service-key-has-whitespace etc/systemd/system/test.service at line 3
E: systemd-general: service-key-has-whitespace usr/lib/systemd/system/test.service at line 3
--
2.1.4
From 34f9fd2c9a405933282734fa7704aa5c41fda8bd Mon Sep 17 00:00:00 2001
From: Felipe Sateler <fsate...@debian.org>
Date: Sat, 27 Jun 2015 22:01:19 -0300
Subject: [PATCH 3/4] Add parameter to prevent tagging when parsing values
Enables us to prevent multiple service-key-has-whitespace
---
checks/systemd.pm | 7 ++++---
t/tests/systemd-complex-service-file/tags | 1 -
t/tests/systemd-general/tags | 2 --
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/checks/systemd.pm b/checks/systemd.pm
index 99a286f..c413585 100644
--- a/checks/systemd.pm
+++ b/checks/systemd.pm
@@ -146,7 +146,7 @@ sub get_systemd_service_names {
$safe_add_service->($name, $file);
my @aliases
- = extract_service_file_values($info, $file, 'Install', 'Alias');
+ = extract_service_file_values($info, $file, 'Install', 'Alias', 1);
for my $alias (@aliases) {
$safe_add_service->($alias, $file);
@@ -210,14 +210,15 @@ sub service_file_lines {
# Extracts the values of a specific Key from a .service file
sub extract_service_file_values {
- my ($info, $file, $extract_section, $extract_key) = @_;
+ my ($info, $file, $extract_section, $extract_key, $skip_tag) = @_;
my (@values, $section);
my @lines = service_file_lines($file);
my $key_ws = first_index { /^[[:alnum:]]+(\s*=\s|\s+=)/ } @lines;
if ($key_ws > -1) {
- tag 'service-key-has-whitespace', $file, 'at line', $key_ws;
+ tag 'service-key-has-whitespace', $file, 'at line', $key_ws
+ unless $skip_tag;
}
if (any { /^\.include / } @lines) {
my $parent_dir = $file->parent_dir;
diff --git a/t/tests/systemd-complex-service-file/tags b/t/tests/systemd-complex-service-file/tags
index 1ffee42..61a9669 100644
--- a/t/tests/systemd-complex-service-file/tags
+++ b/t/tests/systemd-complex-service-file/tags
@@ -1,4 +1,3 @@
E: systemd-complex-service-file: service-key-has-whitespace lib/systemd/system/test3.service at line 3
-E: systemd-complex-service-file: service-key-has-whitespace lib/systemd/system/test3.service at line 3
W: systemd-complex-service-file: systemd-service-file-refers-to-obsolete-target lib/systemd/system/test.service dbus.target
W: systemd-complex-service-file: systemd-service-file-refers-to-obsolete-target lib/systemd/system/test2.service syslog.target
diff --git a/t/tests/systemd-general/tags b/t/tests/systemd-general/tags
index 3223f6a..6f693e3 100644
--- a/t/tests/systemd-general/tags
+++ b/t/tests/systemd-general/tags
@@ -1,8 +1,6 @@
E: systemd-general: init-script-is-not-a-file etc/init.d/fifo-pipe-as-init
E: systemd-general: service-file-is-not-a-file etc/systemd/system/fifo-pipe-as-init.service
E: systemd-general: service-key-has-whitespace etc/systemd/system/test.service at line 3
-E: systemd-general: service-key-has-whitespace etc/systemd/system/test.service at line 3
-E: systemd-general: service-key-has-whitespace usr/lib/systemd/system/test.service at line 3
E: systemd-general: service-key-has-whitespace usr/lib/systemd/system/test.service at line 3
E: systemd-general: special-file etc/init.d/fifo-pipe-as-init 0644
E: systemd-general: special-file etc/systemd/system/fifo-pipe-as-init.service 0644
--
2.1.4
From 929d634804df47c595e2b9ede2da299d50fcabd9 Mon Sep 17 00:00:00 2001
From: Felipe Sateler <fsate...@debian.org>
Date: Sat, 27 Jun 2015 12:20:19 -0300
Subject: [PATCH 4/4] systemd.{desc,pm}: add check for rcS.d init scripts
without native systemd unit
---
checks/systemd.desc | 14 ++++++++++++++
checks/systemd.pm | 12 ++++++++++--
t/tests/systemd-general/debian/debian/init | 2 +-
t/tests/systemd-general/desc | 1 +
t/tests/systemd-general/tags | 1 +
5 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/checks/systemd.desc b/checks/systemd.desc
index f84835f..6554f58 100644
--- a/checks/systemd.desc
+++ b/checks/systemd.desc
@@ -47,6 +47,20 @@ Info: The listed init.d script has no systemd equivalent.
Your package ships a service file, but for the listed init.d script, there is
no corresponding systemd service file.
+Tag: systemd-no-service-for-init-rcS-script
+Severity: serious
+Certainty: certain
+Ref: https://wiki.debian.org/Teams/pkg-systemd/rcSMigration
+Info: The rcS init.d script has no systemd equivalent.
+ .
+ Systemd has a SysV init.d script compatibility mode. It provides access to
+ each SysV init.d script as long as there is no native service file with the
+ same name (e.g. <tt>/lib/systemd/system/rsyslog.service</tt> corresponds to
+ <tt>/etc/init.d/rsyslog</tt>).
+ .
+ Services in rcS.d are particularly problematic, because they often cause
+ dependency loops, as they are ordered very early in the boot sequence.
+
Tag: init.d-script-does-not-source-init-functions
Severity: normal
Certainty: certain
diff --git a/checks/systemd.pm b/checks/systemd.pm
index c413585..1332659 100644
--- a/checks/systemd.pm
+++ b/checks/systemd.pm
@@ -84,6 +84,7 @@ sub check_init_script {
my ($info, $file, $services) = @_;
my $basename = $file->basename;
my $lsb_source_seen;
+ my $is_rcs_script = 0;
if (!$file->is_regular_file) {
unless ($file->is_open_ok) {
@@ -96,12 +97,14 @@ sub check_init_script {
lstrip;
if ($. == 1 and m{\A [#]! \s*/lib/init/init-d-script}xsm) {
$lsb_source_seen = 1;
- last;
}
+ if (m,#.*Default-Start:.*S,) {
+ $is_rcs_script = 1;
+ }
+
next if /^#/;
if (m,(?:\.|source)\s+/lib/(?:lsb/init-functions|init/init-d-script),){
$lsb_source_seen = 1;
- last;
}
}
close($fh);
@@ -112,6 +115,11 @@ sub check_init_script {
# make the package work with systemd.
tag 'systemd-no-service-for-init-script', $basename
if (%{$services} and !$services->{$basename});
+
+ # rcS scripts are particularly bad, warn even if there is
+ # no systemd integration
+ tag 'systemd-no-service-for-init-rcS-script', $basename
+ if (!$services->{$basename} and $is_rcs_script);
return;
}
diff --git a/t/tests/systemd-general/debian/debian/init b/t/tests/systemd-general/debian/debian/init
index afffa18..42cb175 100644
--- a/t/tests/systemd-general/debian/debian/init
+++ b/t/tests/systemd-general/debian/debian/init
@@ -3,7 +3,7 @@
# Provides: systemd-general
# Required-Start: $remote_fs $syslog
# Required-Stop: $remote_fs $syslog
-# Default-Start: 2 3 4 5
+# Default-Start: S 2 3 4 5
# Default-Stop: 0 1 6
# Short-Description: Example initscript
# Description: This file should be used to construct scripts to be
diff --git a/t/tests/systemd-general/desc b/t/tests/systemd-general/desc
index 0bffbeb..f865251 100644
--- a/t/tests/systemd-general/desc
+++ b/t/tests/systemd-general/desc
@@ -12,3 +12,4 @@ Test-For:
systemd-tmpfiles.d-outside-usr-lib
systemd-service-file-refers-to-obsolete-target
systemd-no-service-for-init-script
+ systemd-no-service-for-init-rcS-script
diff --git a/t/tests/systemd-general/tags b/t/tests/systemd-general/tags
index 6f693e3..3dc3b91 100644
--- a/t/tests/systemd-general/tags
+++ b/t/tests/systemd-general/tags
@@ -4,6 +4,7 @@ E: systemd-general: service-key-has-whitespace etc/systemd/system/test.service a
E: systemd-general: service-key-has-whitespace usr/lib/systemd/system/test.service at line 3
E: systemd-general: special-file etc/init.d/fifo-pipe-as-init 0644
E: systemd-general: special-file etc/systemd/system/fifo-pipe-as-init.service 0644
+E: systemd-general: systemd-no-service-for-init-rcS-script systemd-general
E: systemd-general: systemd-no-service-for-init-script systemd-general
E: systemd-general: systemd-service-file-outside-lib etc/systemd/system/fifo-pipe-as-init.service
E: systemd-general: systemd-service-file-outside-lib etc/systemd/system/test.service
--
2.1.4
