5

I have a script and I am trying to elimate bad practices using perlcritic.

One line I have is as follows:

open(my($FREESPCHK), $cmdline ) || &zdie($MSG_PASSTHRU,"Error checking free space of file system.");

This gives this error: Two-argument "open" used at line xxx, column x. See page 207 of PBP. (Severity: 5)

Any ideas on how to fix it?

toolic
  • 57,801
  • 17
  • 75
  • 117
En-Motion
  • 899
  • 6
  • 21
  • 34

2 Answers2

25

If you use the --verbose 11 flag, you'll get a far more detailed explanation of the error. In this case, the error you get is looks like this:

Two-argument "open" used at line 6, near 'open FILE, 'somefile';'.
InputOutput::ProhibitTwoArgOpen (Severity: 5)

The three-argument form of `open' (introduced in Perl 5.6) prevents subtle bugs that occur when the filename starts with funny characters like '>' or '<'. The IO::File module provides a nice object-oriented interface to filehandles, which I think is more elegant anyway.

 open( $fh, '>output.txt' );          # not ok
 open( $fh, q{>}, 'output.txt' );     # ok

 use IO::File;
 my $fh = IO::File->new( 'output.txt', q{>} ); # even better!

It's also more explicitly clear to define the input mode of the file, as in the difference between these two:

  open( $fh, 'foo.txt' );       # BAD: Reader must think what default mode is
  open( $fh, '<', 'foo.txt' );  # GOOD: Reader can see open mode

This policy will not complain if the file explicitly states that it is compatible with a version of perl prior to 5.6 via an include statement, e.g. by having `require 5.005' in it.

I found this by reading the perlcritic documentation.

Dave Cross
  • 68,119
  • 3
  • 51
  • 97
  • 1
    I like to use a custom `--verbose FORMAT` which includes `%P`. This shows me the entire policy name, which allows me to easily get the complete explanation of the error using `perldoc`, via a quick copy'n'paste on my command line: `perldoc Perl::Critic::Policy::InputOutput::ProhibitTwoArgOpen` – toolic Dec 19 '11 at 15:52
1

To make Perl Critic shut up, but do no real good at all, just modify the code to:

open(my $PIPE_FROM_FREESPCHK, "-|", $cmdline)
    || zdie($MSG_PASSTHRU, "Error checking free space of file system.");

Note however that this is absolutely no better in any regard whatsoever from the far more obvious:

open(my $PIPE_FROM_FREESPCHK, "$cmdline |")
    || zdie($MSG_PASSTHRU, "Error checking free space of file system.");

Because you are not separating out your tokens for calling exec directly. That would look more like this:

open(my $PIPE_FROM_FREESPCHK, "-|", $cmd_name, @cmd_args)
    || zdie($MSG_PASSTHRU, "Error checking free space of file system.");

The question is whether you are running a shell command or just exec’ing something. If your free check is something like df . 2>/dev/null | awk ...., then you need the full shell. If it is just df, then you don’t.

tchrist
  • 78,834
  • 30
  • 123
  • 180
Tudor Constantin
  • 26,330
  • 7
  • 49
  • 72
  • The answer is essentially correct, but in the case of opening a command the `mode` parameter should be `'-|'` for a readable filehandle (you want to read the stdout of a command) or `'|-'` a writable filehandle (you want to write to stdin of a command). – vstm Dec 19 '11 at 12:03
  • 3
    Not any better ...except when it is. Your so called equivalent doesn't work when the command starts with ">" (which I use rather commonly). That's the whole point of the critique, so talk about missing the point. – ikegami Dec 19 '11 at 21:55