'use strict', cleanup everywhere

Julien ÉLIE julien at trigofacile.com
Tue Jul 5 15:56:57 UTC 2011


Hi Florian,

> the other perl scripts that didn't use 'use strict' yet,
> and also cleaned out some perl4-ish features.

That's a nice move.  Thanks!



> procbatch, scanspool and signcontrol don't have manpages yet,
> so if you won't, I might write some pod when I find the time...

Regarding signcontrol, we should define first what its future
is within INN.  Please see:
    http://inn.eyrie.org/trac/ticket/14

“It's not clear that INN should be shipping this program at all.
Only a few people will need to issue PGP-signed control messages,
and those who do can easily get it as part of the pgpverify
distribution (or download one of the many other programs
for this purpose).”

Maybe we could therefore just remove it.



scanspool -> yes, a man page would be interesting to have.
A few comments, already present in its source code, can be
helpful for a start.

procbatch -> yes, also useful.
An interesting start is this memo written by Russ:
    https://groups.google.com/group/news.software.nntp/msg/c41b7d850c50e382
    Message-ID: <ylsnl3aml8.fsf at windlord.stanford.edu>



> --- a/backends/mod-active.in
> +++ b/backends/mod-active.in
> -open(OLDACT, "<  $oldact") || die "$0: open $oldact: $!\n";
> -open(NEWACT, ">  $newact") || die "$0: open $newact: $!\n";
> +open my $OLDACT, '<', $oldact || die "$0: open $oldact: $!\n";
> +open my $NEWACT, '>', $newact || die "$0: open $newact: $!\n";

Shouldn't the parentheses be kept?

open (my $OLDACT, '<', $oldact) || die "$0: open $oldact: $!\n";
open (my $NEWACT, '>', $newact) || die "$0: open $newact: $!\n";

I think that otherwise the die() condition is not executed (owing to
its being linked to “$oldact || die”).  I once detected it on pullnews
and mailpost:
    http://inn.eyrie.org/trac/changeset/7792
    http://inn.eyrie.org/trac/changeset/7796


I have a question about the change of
OLDACT, "< $file"
to
my $OLDACT, '<', $file

I am unsure it works with Perl 5.004_03.  I have not tested.
<http://perldoc.perl.org/perlopentut.html> mentions changes in Perl 5.6.0
about the initialization of indirect filehandles.
Anyway, our documentation states that 5.8.0+ is recommended.  I hope
that people running INN 2.5.0 and above have a recent Perl distribution.



> +while (<$OLDACT>) {
> +  my $group = (split)[0];
> +  next if exists $todelete{$group};
> +  s/ \S+$/ $tochange{$group}/ if exists $tochange{$group};
> +  delete $toadd{$group};
> +  if (!print $NEWACT $_) {
> +    ctlinnd("go $pausemsg");
> +    die "$0: writing $newact failed ($!), aborting\n";
> +  }
> +}

That is a nice function.  Far better than the previous eval()
we had.  Thanks for the rewriting.

I also notice a new behaviour:  restarting INN before dying, which
is indeed nice to do.  We previously did not do that, which could
cause INN stop running in a few cases.



> -close(OLDACT) || warn "$0: close $oldact: $!\n";
> -close(NEWACT) || warn "$0: close $newact: $!\n";
> +close $OLDACT || warn "$0: close $oldact: $!\n";
> +close $NEWACT || warn "$0: close $newact: $!\n";

Same thing here.



> -if (!rename("$oldact", "$oldact.old")) {
> +if (!rename "$oldact", "$oldact.old") {
>     warn "$0: rename $oldact $oldact.old: $!\n";
>   }
> 
> -if (!rename("$newact", "$oldact")) {
> +if (!rename "$newact", "$oldact") {
>     die "$0: rename $newact $oldact: $!\n";
>   }

I added a comment:
      # Do not restart INN here:  we no longer have a valid active file!

I hope nobody will ever experience a blow up of his active file
consecutively to a run of mod-active :-/



> -    print TIMES "$_ $time checkgroups-update\n" || last;
> +    print $TIMES "$_ $time checkgroups-update\n" || last;

I see that we already had a problem here.
I believe we should use:
    print ($TIMES "$_ $time checkgroups-update\n") || last;



> -  close(TIMES) || warn "$0: close $actime: $!\n";
> +  close $TIMES || warn "$0: close $actime: $!\n";

I wonder whether the best unambiguous writing wouldn't be:
    close $TIMES or warn "$0: close $actime: $!\n";

At least, it solves any further problem with the use or the absence
of use of parentheses :)





> --- a/control/signcontrol.in
> +++ b/control/signcontrol.in
> +use Fcntl qw(F_SETFD LOCK_EX);
> -  flock(LOCK, 2);               # block until locked
> +  flock $LOCK, LOCK_EX;         # block until locked

Thanks for the change to LOCK_EX.



> +my ($die, $group, $action, $grouppat, %header, $moderated, $body, @ERROR);
> 
> -$die = '';

If we remove it, we will have a problem of concatenation in an uninitialized
variable afterwards, for instance with:
    $die .= "$0: continuation lines in headers not allowed\n";

I added a comment:

+ # Initialize the $die variable here (we use it to concatenate possible
+ # error messages during the run of the following functions).
+ $die = '';



> -sub
> -readbody
> +sub readbody {
> +  my ($status, $ngline, $fixline, $used, $desc, $mods);
> 
> -{
> -  local($_, $/);
> -  local($status, $ngline, $fixline, $used, $desc, $mods);
> -
> -  undef $/;
> -  $body = $_ = <STDIN>;
> +  local $/ = undef;
> +  $body = <STDIN>;      # slurp the rest of the article

I see that $_ is no longer set to <STDIN>.
Won't it cause a problem just afterwards with these checks using $_?

    $die .= "$0: nonstandard first line in body for $group\n"
      if ! /^\Q$group\E\sis\s$status\snewsgroup\b/;

    $ngline =
      (/^$intro\Q$group\E[ \t]+(.+)\n(\n|\Z(?!\n))/mi)[0];


Incidentally, I do not understand the [0] for $ngline.
Isn't it:

    $die .= "$0: nonstandard first line in body for $group\n"
      if $body !~ /^\Q$group\E\sis\s$status\snewsgroup\b/;

    $ngline =
      ($body =~ /^$intro\Q$group\E[ \t]+(.+)\n(\n|\Z(?!\n))/mi);

without [0]?

-- 
Julien ÉLIE

« Mettez-vous de profil avec les épaules de face et ne bougeons plus
  je vous prie. » (Astérix)



More information about the inn-workers mailing list