Debian Bug report logs - #704197
Please review: systemd checks

version graph

Package: lintian; Maintainer for lintian is Debian Lintian Maintainers <lintian-maint@debian.org>; Source for lintian is src:lintian.

Reported by: Michael Stapelberg <stapelberg@debian.org>

Date: Fri, 29 Mar 2013 10:15:01 UTC

Severity: wishlist

Found in version lintian/2.5.10.4

Fixed in version lintian/2.5.15

Done: Niels Thykier <niels@thykier.net>

Bug is archived. No further changes may be made.

Toggle useless messages

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to debian-bugs-dist@lists.debian.org, pkg-systemd-maintainers@lists.alioth.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Fri, 29 Mar 2013 10:15:05 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
New Bug report received and forwarded. Copy sent to pkg-systemd-maintainers@lists.alioth.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>. (Fri, 29 Mar 2013 10:15:05 GMT) Full text and rfc822 format available.

Message #5 received at submit@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Debian Bug Tracking System <submit@bugs.debian.org>
Subject: Please review: systemd checks
Date: Fri, 29 Mar 2013 11:11:21 +0100
[Message part 1 (text/plain, inline)]
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!
[systemd.desc (text/plain, attachment)]
[systemd (text/plain, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Fri, 29 Mar 2013 11:21:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Fri, 29 Mar 2013 11:21:04 GMT) Full text and rfc822 format available.

Message #10 received at 704197@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: Michael Stapelberg <stapelberg@debian.org>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Fri, 29 Mar 2013 12:16:14 +0100
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





Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Fri, 29 Mar 2013 14:27:15 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Fri, 29 Mar 2013 14:27:15 GMT) Full text and rfc822 format available.

Message #15 received at 704197@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Niels Thykier <niels@thykier.net>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Fri, 29 Mar 2013 15:24:28 +0100
[Message part 1 (text/plain, inline)]
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:

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.

> 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.

> 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.

> That has the advantage that we know that argument is unused.
I don’t understand what the advantage of knowing that is :-).

> 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?

>> 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.

-- 
Best regards,
Michael
[systemd (application/octet-stream, attachment)]
[systemd.desc (application/octet-stream, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Fri, 29 Mar 2013 17:21:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Russ Allbery <rra@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Fri, 29 Mar 2013 17:21:04 GMT) Full text and rfc822 format available.

Message #20 received at 704197@bugs.debian.org (full text, mbox):

From: Russ Allbery <rra@debian.org>
To: Michael Stapelberg <stapelberg@debian.org>
Cc: 704197@bugs.debian.org, Niels Thykier <niels@thykier.net>
Subject: Re: Bug#704197: Please review: systemd checks
Date: Fri, 29 Mar 2013 10:19:49 -0700
Michael Stapelberg <stapelberg@debian.org> writes:
> 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.

I desperately need to update the page that Niels is looking at to
incorporate the results of a very comprehensive style review we did at
Stanford with reference to Perl Best Practices, as it's changed a lot.  We
have a new house style that's similar but incorporates a ton of advice
from Perl Best Practices, and I have both perlcritic and perltidy
configurations that correspond to it (as closely as one can, at least --
perltidy is sadly not very flexible).

Some of those changes are, alas, annoyingly intrusive compared to all of
my previous Perl code (which is a chunk of the code in Lintian); for
example, I was convinced to drop the space before the parens on function
calls because it seems to really confuse people coming from just about any
other language except GNU-style C.

I definitely recommend adopting perlcritic and perltidy for this sort of
thing, though, and now use them as part of an automated test suite for my
Perl code.

For a copy of the standard tests that I'm now using, as well as the
perlcriticrc and perltidyrc files (until I can get everything else
updated), clone:

    git://git.eyrie.org/devel/rra-c-util.git

and look at tests/data/perl{critic,tidy}rc and the tests in perl/t.
tests/tap/perl/Test/RRA.pm and tests/tap/perl/test/Config.pm may also be
of interest (particularly the former; the latter is used by my tests to
load configuration so that I can reuse the same test scripts across
multiple packages with different requirements).

I'll send the raw Markdown of our new internal style to the Lintian list.

-- 
Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Mon, 01 Apr 2013 22:15:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Mon, 01 Apr 2013 22:15:04 GMT) Full text and rfc822 format available.

Message #25 received at 704197@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: Michael Stapelberg <stapelberg@debian.org>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Tue, 02 Apr 2013 00:11:01 +0200
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





Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Sat, 06 Apr 2013 16:36:05 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Sat, 06 Apr 2013 16:36:05 GMT) Full text and rfc822 format available.

Message #30 received at 704197@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Niels Thykier <niels@thykier.net>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Sat, 06 Apr 2013 18:33:38 +0200
[Message part 1 (text/plain, inline)]
Hi Niels,

Niels Thykier <niels@thykier.net> writes:
>> 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].
Okay, so how about this?

sub check_init_script {
    my ($pkg, $info, $file) = @_;

    my $lsb_source_seen;
    my $path = $info->index ($file);
    unless ($path->is_regular_file ||
            ($path->is_symlink && defined($path->link_resolved))) {
        tag 'init-script-is-not-a-file', $file;
    }
    open(my $fh, '<', $info->unpacked($file))
        or fail "cannot open $file: $!";
    # …
}

> 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).
I switched to shellwords. We can always rever to the code we’ve had
before, but in my tests, shellwords works fine.

Find the new files attached.

-- 
Best regards,
Michael
[systemd (application/octet-stream, attachment)]
[systemd.desc (application/octet-stream, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Sat, 06 Apr 2013 18:21:09 GMT) Full text and rfc822 format available.

Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Sat, 06 Apr 2013 18:21:09 GMT) Full text and rfc822 format available.

Message #35 received at 704197@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: Michael Stapelberg <stapelberg@debian.org>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Sat, 06 Apr 2013 20:08:42 +0200
On 2013-04-06 18:33, Michael Stapelberg wrote:
> Hi Niels,
> 
> Niels Thykier <niels@thykier.net> writes:
>> [...]
>> 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].
> Okay, so how about this?
> 
> sub check_init_script {
>     my ($pkg, $info, $file) = @_;
> 
>     my $lsb_source_seen;
>     my $path = $info->index ($file);
>     unless ($path->is_regular_file ||
>             ($path->is_symlink && defined($path->link_resolved))) {
>         tag 'init-script-is-not-a-file', $file;

I think you are missing a return here?

>     }
>     open(my $fh, '<', $info->unpacked($file))
>         or fail "cannot open $file: $!";
>     # …
> }
> 
>> 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).
> I switched to shellwords. We can always rever to the code we’ve had
> before, but in my tests, shellwords works fine.
> 

Appreciated.

> Find the new files attached.
> 


~Niels






Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Sat, 06 Apr 2013 18:45:18 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Sat, 06 Apr 2013 18:45:18 GMT) Full text and rfc822 format available.

Message #40 received at 704197@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Niels Thykier <niels@thykier.net>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Sat, 06 Apr 2013 20:44:13 +0200
[Message part 1 (text/plain, inline)]
Hi Niels,

Niels Thykier <niels@thykier.net> writes:
> I think you are missing a return here?
Indeed, thanks.

New files are attached, here is the list of things that I know need to
be fixed:

1) We don’t have any documentation references in the .desc file yet.
2) I need to switch to lab_data_path in check_maintainer_scripts().

Could you please also say a few words on how the usual inclusion process
works? I.e., what are the next steps after there are no more things left
to fix? Also, do I need to mark the checks as experimental because they
are new?

-- 
Best regards,
Michael
[systemd (application/octet-stream, attachment)]
[systemd.desc (application/octet-stream, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Mon, 08 Apr 2013 10:03:10 GMT) Full text and rfc822 format available.

Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Mon, 08 Apr 2013 10:03:10 GMT) Full text and rfc822 format available.

Message #45 received at 704197@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: Michael Stapelberg <stapelberg@debian.org>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Sat, 06 Apr 2013 21:57:40 +0200
On 2013-04-06 20:44, Michael Stapelberg wrote:
> Hi Niels,
> 
> Niels Thykier <niels@thykier.net> writes:
>> I think you are missing a return here?
> Indeed, thanks.
> 
> New files are attached, here is the list of things that I know need to
> be fixed:
> 
> 1) We don’t have any documentation references in the .desc file yet.
> 2) I need to switch to lab_data_path in check_maintainer_scripts().
> 
> Could you please also say a few words on how the usual inclusion process
> works? I.e., what are the next steps after there are no more things left
> to fix?

Tests!  Once there are tests for all the new tags (and none of the
existing tests breaks) we are usually ready to accept the checks.

On test writing:  We don't have a tutorial for writing tests atm.  There
is a bit of help (although a bit outdated) in t/tests/README.

To give you a rough idea of what you are working with, a test case in
t/tests basically follows this pattern:

 mkdir -p rundir/test/
 rsync t/templates/tests/skel/ rundir/test/
 # note t/tests/<testcase>/debian/ is the root of the source package
 # thus, t/tests/<testcase>/debian/debian/ is the "debian" dir.
 rsync t/tests/<testcase>/debian/ rundir/test/

 for file in changelog control ; do
	subst rundir/test/debian/${file}.in > rundir/test/debian/${file}
 done

 cd rundir/test && dpkg-buildpackage -us -uc
 lintian -IE rundir/*.changes | sort > actual
 diff -u t/tests/<testcase>/tags actual

The test should be named after the check (i.e. systemd-<name> in this
case) and have a sequence of 6000 (in t/tests/<testcase>/desc).  A
template for the desc:

  Testname: <name of test>
  Sequence: 6000
  Description: <1-line description>
  Test-For:
   tag1
   tag2

Note: Testname must match the (base)name of the directory[1].  The
description will always be used as synopsis for the package (so it
should follow the same rules).  There are more fields you can use (see
t/tests/README).
  If you want to test for false-positives, replace Test-For with
Test-Against[2].

The full test suite takes over 30 minutes (user time).  Have a look at
Lintian::Tutorial::TestSuite to run only the tests you need while you
work on the tests/check[3].  When you are done, please do a full run of
all tests.  Some of the other tests do some funny checks that may
trigger bugs in your check[4].
  If the full test suite is too heavy for your machine (my laptop
doesn't like it either), just let me know and I will do the run for you.


> Also, do I need to mark the checks as experimental because they are new?
> 

Depends on your level of confidence in your tags and how "well-defined"
the "problem" is[5].  Most tags never had the experimental marker on them.

~Niels

[1] Not checked, but can cause "hard to debug test failures"...
especially if it matches the name of another test (copy-waste ftl).

[2] Test-For requires the test to emit the tag for it to pass.
Test-Against causes it to fail unconditionally if the tag was emitted.
Both fields may be used at the same time.

[3] To test your check for syntax errors (etc.), you may want to run:

  debian/rules onlyrun=check-load

After that:

  debian/rules onlyrun=<testcase>
  debian/rules onlyrun=<check>
  debian/rules onlyrun=suite:scripts

When they pass, you might be ready for the full test suite.  If you got
cores to spare, throw a DEB_BUILD_OPTIONS=parallel=n.

[4] e.g. suite:debs builds some really interesting debs. The legacy test
"filenames" also got some fun filenames in it.

[5] If there are a lot of corner cases (etc.), the check may need
several iterations to mature.





Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Sat, 13 Apr 2013 21:21:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Sat, 13 Apr 2013 21:21:04 GMT) Full text and rfc822 format available.

Message #50 received at 704197@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Niels Thykier <niels@thykier.net>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Sat, 13 Apr 2013 23:17:46 +0200
[Message part 1 (text/plain, inline)]
Hi Niels,

Thanks for your prompt reply.

Niels Thykier <niels@thykier.net> writes:
> Tests!  Once there are tests for all the new tags (and none of the
> existing tests breaks) we are usually ready to accept the checks.
Cool.

Find attached two git format-patch files. The first adds the latest
version of my systemd checks, the second one adds tests.

Could you please have a look at the tests? Please note that I did not
run the whole testsuite because it fails on my machine (see my IRC
query). It’d be great if you could run it for me.

-- 
Best regards,
Michael
[0001-add-systemd-checks.patch (text/x-diff, attachment)]
[0002-add-systemd-tests.patch (text/x-diff, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Tue, 16 Apr 2013 16:57:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Tue, 16 Apr 2013 16:57:04 GMT) Full text and rfc822 format available.

Message #55 received at 704197@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: Michael Stapelberg <stapelberg@debian.org>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Tue, 16 Apr 2013 18:56:20 +0200
On 2013-04-06 18:33, Michael Stapelberg wrote:
> Hi Niels,
> 
> [...]
> Okay, so how about this?
> 
> sub check_init_script {
>     my ($pkg, $info, $file) = @_;
> 
>     my $lsb_source_seen;
>     my $path = $info->index ($file);
>     unless ($path->is_regular_file ||
>             ($path->is_symlink && defined($path->link_resolved))) {
>         tag 'init-script-is-not-a-file', $file;
>     }
>     open(my $fh, '<', $info->unpacked($file))
>         or fail "cannot open $file: $!";
>     # …
> }
> 
> [...]



I thought this was safe, but it does have an issue as well.  Consider
symlink chaining:

  safe-symlink -> unsafe-symlink
  unsafe-symlink -> ../../../../etc/passwd

$path->link_resolved will approve "safe-symlink" because it can be
resolved safely.  However, it does not check that the target is also a
safe symlink - so a loop/recursion is needed.  That said, using the new
"is_ancestor_of" (from L::Util) is probably a lot easier to use
correctly.  Basically:

  use Lintian::Util qw(is_ancestor_of);

  [...]

  my $unpacked_file = $info->unpacked($file);
  if ( -f $unpacked_file &&
       is_ancestor_of($info->unpacked, $unpacked_file)) {
     # exists, is a file and within the package root.
     open(my $fd, '<', $unpacked_file) or fail "..."
     [...]
  } else {
     # unsafe, is not a file or does not exist
     [...]
  }

~Niels





Information forwarded to debian-bugs-dist@lists.debian.org, Debian Lintian Maintainers <lintian-maint@debian.org>:
Bug#704197; Package lintian. (Wed, 17 Apr 2013 20:54:04 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Stapelberg <stapelberg@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Lintian Maintainers <lintian-maint@debian.org>. (Wed, 17 Apr 2013 20:54:04 GMT) Full text and rfc822 format available.

Message #60 received at 704197@bugs.debian.org (full text, mbox):

From: Michael Stapelberg <stapelberg@debian.org>
To: Niels Thykier <niels@thykier.net>, 704197@bugs.debian.org
Subject: Re: Bug#704197: Please review: systemd checks
Date: Wed, 17 Apr 2013 22:51:37 +0200
[Message part 1 (text/plain, inline)]
Hi Niels,

Niels Thykier <niels@thykier.net> writes:
> I thought this was safe, but it does have an issue as well.  Consider
> symlink chaining:
>
>   safe-symlink -> unsafe-symlink
>   unsafe-symlink -> ../../../../etc/passwd
>
> $path->link_resolved will approve "safe-symlink" because it can be
> resolved safely.  However, it does not check that the target is also a
> safe symlink - so a loop/recursion is needed.  That said, using the new
> "is_ancestor_of" (from L::Util) is probably a lot easier to use
> correctly.  Basically:
Thanks for the explanation and the example. I have updated my code and
the tests still work. Find the updated patches attached (rebased against
current master).

-- 
Best regards,
Michael
[0001-add-systemd-checks.patch (text/x-diff, attachment)]
[0002-add-systemd-tests.patch (text/x-diff, attachment)]
[0003-systemd-use-is_ancestor_of-instead-of-just-link_reso.patch (text/x-diff, attachment)]

Added tag(s) pending. Request was from Niels Thykier <niels@thykier.net> to control@bugs.debian.org. (Sun, 30 Jun 2013 20:57:11 GMT) Full text and rfc822 format available.

Reply sent to Niels Thykier <niels@thykier.net>:
You have taken responsibility. (Mon, 22 Jul 2013 21:36:13 GMT) Full text and rfc822 format available.

Notification sent to Michael Stapelberg <stapelberg@debian.org>:
Bug acknowledged by developer. (Mon, 22 Jul 2013 21:36:13 GMT) Full text and rfc822 format available.

Message #67 received at 704197-close@bugs.debian.org (full text, mbox):

From: Niels Thykier <niels@thykier.net>
To: 704197-close@bugs.debian.org
Subject: Bug#704197: fixed in lintian 2.5.15
Date: Mon, 22 Jul 2013 21:33:40 +0000
Source: lintian
Source-Version: 2.5.15

We believe that the bug you reported is fixed in the latest version of
lintian, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 704197@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Niels Thykier <niels@thykier.net> (supplier of updated lintian package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Format: 1.8
Date: Mon, 22 Jul 2013 22:53:25 +0200
Source: lintian
Binary: lintian
Architecture: source all
Version: 2.5.15
Distribution: unstable
Urgency: low
Maintainer: Debian Lintian Maintainers <lintian-maint@debian.org>
Changed-By: Niels Thykier <niels@thykier.net>
Description: 
 lintian    - Debian package checker
Closes: 700502 701177 704197 707534 707906 708367 708551 710919 712607 712641 712932 713884 714191 714437
Changes: 
 lintian (2.5.15) unstable; urgency=low
 .
   "use less qw(memory);"
 .
   * Summary of tag changes:
     + Added:
       - composer-package-without-pkg-php-tools-builddep
       - init-script-is-not-a-file
       - init.d-script-does-not-source-init-functions
       - maintainer-script-calls-systemctl
       - manpage-named-after-build-path
       - missing-pkg-php-tools-addon
       - missing-pkg-php-tools-buildsystem
       - pear-channel-without-pkg-php-tools-builddep
       - pear-package-but-missing-dependency
       - pear-package-feature-requires-newer-pkg-php-tools
       - pear-package-not-using-substvar
       - pear-package-without-pkg-php-tools-builddep
       - pecl-package-requires-build-dependency
       - service-file-is-not-a-file
       - systemd-no-service-for-init-script
       - systemd-service-file-outside-lib
       - systemd-service-file-refers-to-obsolete-target
       - systemd-tmpfiles.d-outside-usr-lib
 .
   * checks/*.pm:
     + [NT] Add final return to all subs in checks and ensure
       that the "run" sub complies with Lintian's own
       recommendation.
   * checks/binary.pm:
     + [NT] Apply patch from Bastien Roucariès to fix false-
       negatives for debug files in usr/lib/debug/.build-id.
       (Closes: #714191)
     + [NT] Apply patch from Bastien Roucariès to fix false-
       positive debug-file-with-no-debug-symbols for files
       using compressed debug sections.
   * checks/fields.pm:
     + [NT] Apply patch from Niko Tyni to fix false-positive
       package-superseded-by-perl for packages with epochs.
       (Closes: #710919)
   * checks/files.pm:
     + [NT] Fix some false-negative extra-license-file.
       Thanks to Helmut Grohne for the report and the advices.
       (Closes: #701177)
   * checks/manpages.{desc,pm}:
     + [NT] Apply patch from Bastien Roucariès to test for
       manpages named after their build path.
       (Closes: #713884)
     + [NT] Skip some checks on empty manpages.
       (Closes: #700502)
   * checks/phppear.{desc,pm}:
     + [NT] New check based on patches from Mathieu Parent.
       (Closes: #708551)
   * checks/source-copyright.pm:
     + [NT] Some tags now refer to the line number of the field
       with an issue instead of the line number of the paragraph.
   * checks/systemd.{desc,pm}:
     + [NT] New check for systemd related files.  Thanks to
       Michael Stapelberg for providing the check and the
       tests.  (Closes: #704197)
 .
   * collection/copyright-file:
     + [NT] Avoid creating an empty copyright file when it is
       not needed.
   * collection/unpacked:
     + [NT] Skip signature checking of source packages.
       (Closes: #707534)
 .
   * data/binary/embedded-libs:
     + [NT] Rename libgd2 to libgd.  (Closes: #708367)
   * data/fields/virtual-packages:
     + [NT] Refresh.  Thanks to Laurent Bigonville for the
       reminder.  (Closes: #712641)
   * data/files/{fonts,locale-codes}:
     + [NT] Refresh.
   * data/menu-format/add-categories:
     + [NT] Apply patch from Bastien Roucariès to include newer
       categories.  Thanks to Yves-Alexis Perez for the report.
       (Closes: #712932)
   * data/output/manual-references:
     + [NT] Refresh.
   * data/scripts/interpreters:
     + [NT] Apply patch from Bastien Roucariès to include gjs.
       Thanks to Andreas Henriksson for the report.
       (Closes: #712607)
 .
   * debian/control:
     + [NT] Add Build-Depends on pkg-php-tools for a new test.
   * debian/dirs:
     + [NT] Remove /var/lib/lintian, we no longer use it.
   * debian/docs:
     + [NT] Add auto-generated API documentation.
   * debian/lintian.examples:
     + [NT] New file to install examples.  (Closes: #707906)
   * debian/rules:
     + [NT] Generate API documentation during build.
     + [NT] Call dh_installexamples.
 .
   * doc/examples/*:
     + [NT] New example files.
   * doc/lintian.xml:
     + [NT] Add small example vendor profile to the user
       manual.
 .
   * frontend/lintian:
     + [NT] Add information about memory usage with -ddd if
       Devel::Size is available.  A more detailed breakdown
       of the memory usage with -dddd.
 .
   * lib/Lintian/Collect/Package.pm:
     + [NT] Share some string values in the file_info and in
       the (X_)index methods.  This reduces memory usage a bit.
   * lib/Lintian/Collect/Source.pm:
     + [NT] "binaries" and "binary_field" now only exposes data
       about entries in d/control with a valid package name.
     + [NT] Document that "binaries" return an unorderd list.
     + [NT] relation and relation_noarch now recognises
       "Build-Depends-Arch".
   * lib/Lintian/Path.pm:
     + [NT] Fix bug in the overloaded qr// operator.
   * lib/Lintian/ProcessablePool.pm:
     + [NT] Fix a bug that could cause .changes files to be
       silently skipped.  This only occured if a related package
       was passed on the command line before the .changes file.
       Thanks to Salvo Tomaselli for reporting the bug.
       (Closes: #714437)
   * lib/Lintian/Relation.pm:
     + [NT] Apply some memory optimisations to some common cases.
   * lib/Lintian/Tags.pm:
     + [NT] Use croak instead of die when a check emits an
       unknown tag.  This gives the check writer a better chance
       of finding where the problem occured.
   * lib/Lintian/Util.pm:
     + [NT] Have parse_dpkg_control and visit_dpkg_control
       give a more detailed line number information about
       paragraphs.
 .
   * reporting/harness:
     + [NT] Clear some variables before running Lintian in the
       hope it will reduce the memory pressure on "long runs".
     + [NT] Remove support for "$LINTIAN_GPG_CHECK" config
       variable.  Lintian no longer checks any signatures.
Checksums-Sha1: 
 e287a6c3f71ffb151170b925d831aa360713a02a 2573 lintian_2.5.15.dsc
 a1cced0033a98ffa348904ad0167b5134a22d7ab 1244089 lintian_2.5.15.tar.gz
 b4481d88cde6dc77eb1622451606a6fb9284577b 853838 lintian_2.5.15_all.deb
Checksums-Sha256: 
 dbd26cb1a540189585bb7e8099bdb992059a634427755ff034a1d67703d9179b 2573 lintian_2.5.15.dsc
 56718df28df44e74f255457d6bcc26133e669de7e74d2ab582884bd03f6c3159 1244089 lintian_2.5.15.tar.gz
 6e7a2c6fcb6d562f24f065de257462ad96db658033ab7d8285153cf550ed2477 853838 lintian_2.5.15_all.deb
Files: 
 d55eff6b4298cda9788a04da4c11e59e 2573 devel optional lintian_2.5.15.dsc
 fa5189c93322387403d555237e952a1d 1244089 devel optional lintian_2.5.15.tar.gz
 0c968de4cbf2d910ad5987d6e7a58fc3 853838 devel optional lintian_2.5.15_all.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJR7aQGAAoJEAVLu599gGRC4yMP/i2du8G9LgWuETMt15GDaBjg
B6fy1h4flZ9UIufshA8VHLVpimn+Kv2TQ2IKHDL3uoAu/cOCCwu+bXWeICzT4elU
ZLT7y0r54MPAzY8WluKdPfka1cgXdRlqUmfvYJFesiF4crTbmW3lJhVg+/pYKPal
fYpFygp3HHfVCNDdqAcuDVChXrxwYVCatXED5EL0p1+uFOLHK8cCzdnLYX6hVZeN
raXRGv4g4U+HsJzQ62/0jPGvnrnexQom/tdkHkcUfMnly4QjqbAoqoKTtOt3DJf8
WJ7Q9tQyGH3Fb9AO4Y6D1tCnmeDt7D9lVEhz3evBsztPtPbbDCG7Xvvgg5BIH3l5
8cqlrULGtd2Cc04QNF3rMf3/axmI58J+qryJKmuGwcYZ3vdjd1puetl6qOCnHgc3
1Avs5BObXuk86bI7EKOD+r9LyYhTAkAgku4v0fHbSIjkpJB5otNeke5MIv/hK1JD
SggpROPJld4vVCKsxKe3UeKXjondapMutLLxXVQvQojGpg/uZqai7dHre23DSGy2
1rt5OJ5ROwRLnoFkk+CvwMz1jF6a8JBR3bm4VEVkg2gmc3lgK5yM+6OLO4A/9nvq
hPS0w4cpJd/AxDMD8Pgvp0vnwIxCDXp11S17OgkVK9dt7xDZ71PWpoPMltuwa0aM
IJk2GL9xWL0Zz00dD9Hu
=RqOv
-----END PGP SIGNATURE-----




Bug archived. Request was from Debbugs Internal Request <owner@bugs.debian.org> to internal_control@bugs.debian.org. (Tue, 20 Aug 2013 07:32:15 GMT) Full text and rfc822 format available.

Send a report that this bug log contains spam.


Debian bug tracking system administrator <owner@bugs.debian.org>. Last modified: Fri Apr 18 04:15:05 2014; Machine Name: beach.debian.org

Debian Bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.