Bug#704197: Please review: systemd checks
On 2013-03-29 11:11, Michael Stapelberg wrote:
> Package: lintian
> Version: 2.5.10.4
> Severity: wishlist
>
> Attached you can find my first stab at systemd-related checks for
> lintian. While some details in parsing the service files are not
> implemented (see the TODOs in the code), I’d like you to have a look at
> the checks in general. Is there anything that needs to be improved
> before these can be shipped with lintian?
>
> Thanks!
>
Hi,
Thanks for working on Lintian checks, it is much appreciated it. :)
I have annotated some comments with "[style]", which are minor stylistic
guidelines. I know Lintian's code style is a mess in general, so it
describes the style I hope we will eventually reach[1]. :)
Note that I will try to (remember to) not repeat style hints, even if
the same "mistake" is made multiple times.
~Niels
[1] When in doubt, I tend to use:
http://www.eyrie.org/~eagle/notes/style/perl.html
>
> systemd.desc
>
>
> Check-Script: systemd
> Author: Michael Stapelberg <stapelberg@debian.org>
> Abbrev: systemd
Abrrev is optional and is only useful if the name is shorter than the
name in Check-Script (it is an abbrevation of the name, like "manpages"
is aliased "man").
> Type: binary
> Info: Checks various systemd policy things
> Needs-Info: scripts, index, unpacked, file-info
>
> [... some tags ...]
I noticed that there appear to be no use of references (Ref: URL, #bug,
policy X.Y ...). I would recommend finding such so people can quickly
find more information. Links to systemd documentation, specification or
even just a Debian wiki page.
>
> systemd
>
> [...]
>
> use File::Basename;
[style] I like to group Lintian modules separately (and after) external
modules. That basically gives 3-4 groups; pragmas[, constants],
external modules and then Lintian modules.
> use Lintian::Collect::Binary ();
This import is not needed (in general, Perl do not require you to load
modules just because you use instances of classes from that module).
> use Lintian::Relation qw(:constants);
Does not appear to be used?
> use Data::Dumper;
Left-over from debugging?
>
> sub run {
> my ($pkg, $type, $info) = @_;
>
> if ($type eq 'binary') {
This condition will always be true - the "Type:"-field determines what
values $type can have. In this particular case, it is set to "binary".
If you do not need $pkg or $type, then you can replace them with undef. E.g.
my (undef, undef, $info) = @_;
That has the advantage that we know that argument is unused. (As far as
I can tell, $pkg is passed around to various subs but never actually used).
> [...]
>
> for my $file ($info->sorted_index) {
> if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
> tag('systemd-tmpfiles.d-outside-usr-lib', $file);
[style] The tag sub is a special case in regards to style; it tends to
be treated like a "perl built-in" or "die-like sub" (i.e. no parentheses
unless needed for understanding or semantic reasons). So:
tag 'systemd-tmpfiles.d-outside-usr-lib', $file;
Note if you must use parentheses around tag, a built-in or a "die"-like
sub, please use the same style as for regular subs (see next comment).
> [...]
> if ($file =~ m,/systemd/system/.*\.service$,) {
> check_systemd_service_file($pkg, $info, $file);
[style] For "non-built" sub, we tend to have space between the sub name
and the argument parentheses. For subs that takes no arguments, the
parentheses are omitted where possible. So:
check_systemd_service_file ($pkg, $info, $file);
> [...]
> my @init_scripts = grep(m,^etc/init\.d/.+,, $info->sorted_index);
>
Erh, I think "," might have been a poor choice of regex delimiter here
(as it is also the argument delimiter). For this you could use ":"
(among others) or alternatively call grep with a block:
my @init_scripts = grep {m,^etc/init\.d/.+,} $info->sorted_index;
> [...]
> if ($ships_systemd_file) {
> for my $init_script (@init_scripts) {
> if (grep(/\Q$init_script\E\.service/, @systemd_targets) == 0) {
> tag('systemd-no-service-for-init-script', $init_script);
> }
We sometimes use the "reversed" form of if/unless to reduce scope level.
E.g. something like:
tag 'systemd-no-service-for-init-script', $init_script
unless grep /\Q$init_script\E\.service/, @systemd_targets;
There is no hard rule on went; just an FYI that you are free to use it.
> [...]
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
$pkg argument does not appear to be used?
>
> my $lsb_source_seen;
> open(my $fh, '<', $info->unpacked($file));
No error handling if open fails! Something as simple as:
or fail "open $file: $!";
is fine.
Secondly, there is no check for file type. If someone (deliberately)
creates $file as a fifo-pipe or a symlink it will DoS or (possibly) read
"host system" files (respectively). Usually, a
$info->index ($file)->is_regular_file
should do (if symlinks/hardlinks can be ignored). Alternatively, (for
symlinks) please check that the symlink can be safely resolved before
opening the file (e.g. via the link_resolved method). For more
information, please see the Lintian::Path module's API.
> [...]
>
> sub check_systemd_service_file {
> my ($pkg, $info, $file) = @_;
>
$pkg not used here either?
> my $filename = $info->unpacked ($file);
> open(my $fh, '<', $filename);
Missing error handling.
> [...]
> }
>
> sub split_quoted {
> [...]
> }
>
Is this something that could be done by Text::ParseWords?
> sub extract_service_file_names {
> my ($pkg, $info, $file) = @_;
>
$pkg is not used?
> my @aliases;
> my $section;
> open(my $fh, '<', $info->unpacked($file));
No error handling here
> while (<$fh>) {
> [...]
>
> return (basename($file), @aliases);
> }
>
> sub check_maintainer_scripts {
> my ($info) = @_;
>
> # TODO: use lab_data_path before submitting
Indeed!
> open my $fd, '<', $info->base_dir . '/control-scripts'
> or fail "cannot open lintian control-scripts file: $!";
>
> [...]
Error handling! \o/
~Niels
Reply to: