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 <stapelb...@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 -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
