Bug#704197: Please review: systemd checks
On 2013-03-29 15:24, Michael Stapelberg wrote:
> Hi Niels,
>
> Thanks for the super-fast review. New version is attached, I have fixed
> everything you mentioned, and for the other things I commented inline:
>
You are welcome :) I have not reviewed your revised version yet, but I
will try to look at it soon.
> Niels Thykier <niels@thykier.net> writes:
>> guidelines. I know Lintian's code style is a mess in general, so it
>> describes the style I hope we will eventually reach[1]. :)
> Have you tried using perltidy for Lintian? I loathe manual source code
> formatting after working with gofmt and subsequently perltidy.
>
We did have an "off-by-default" perlcritic test, but not a perltidy one.
I have spent some time with the perlcritic test and it is now on by
default (though with fewer policies than originally).
>> 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.
> Will do once we’ve put up some wiki pages on that.
>
Good. :)
>> If you do not need $pkg or $type, then you can replace them with undef. E.g.
>> my (undef, undef, $info) = @_;
> I prefer to have the variables around, just in case the code needs to be
> changed to use those.
>
I don't feel very strongly about unused arguments[1]. If the perlcritic
policy on unused variables does not trigger on it, they can stay.
Though, I may out of habbit kill the unused variable if I happen to
touch that code.
[1] In many other languages you cannot avoid declaring them anyway :)
>> That has the advantage that we know that argument is unused.
> I don’t understand what the advantage of knowing that is :-).
>
I like it because it sends a clear signal by not having a "name" on the
argument (also, not sure how smart Perl's compile time analysis is so it
may even save a minor amount of memory).
>> 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.
> I came up with this:
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
>
> my $lsb_source_seen;
> my $path = $info->index ($file);
> fail "$file is neither a regular file nor a resolvable symlink"
> unless ($path->is_regular_file || defined($path->link_resolved));
> open(my $fh, '<', $info->unpacked($file))
> or fail "cannot open $file: $!";
>
> # …
> }
>
> Does that seem alright to you?
>
Almost; it definitely plugs the issues I mentioned. That said, I
believe we prefer to emit tags instead of erroring out when we see an
unexpected file type (e.g. see control-file-is-not-a-file).
Secondly, there is a bug in that link_resolved is only applicable to
links. So if it is not a regular file and not a link, the code will
croak in $path->link_resolved[2].
[2] Admittedly a non-issue with the current code where it would
eventually have called "fail" instead. But if the fail part is replaced
with a tag, it becomes an issue.
>>> sub split_quoted {
>>> [...]
>>> }
>>>
>>
>> Is this something that could be done by Text::ParseWords?
> I’m not entirely sure about it. The code I’m using is a 1:1 port of the
> corresponding systemd C code. This obviously has the benefit that there
> are no subtle differences between what we do and what systemd does.
>
It really looks like a implementation of Text::ParseWords's
shellwords[3]. If so, we can get that entire sub as a oneliner (we
already use Text::ParseWords elsewhere).
~Niels
[3] http://perldoc.perl.org/Text/ParseWords.html
Reply to: