Perlcritic - Two argument "open" error

K

2

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?

Kaden answered 19/12, 2011 at 11:58 Comment(2)

What is in $cmdline exactly? Is it a file or something else? – Dhaulagiri 19/12, 2011 at 12:1

## no critic⁠ is all you need to fix this and all the rest of the nattering nabobs of negativity. – Nyhagen 19/12, 2011 at 15:46

A

2

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.

Agreement answered 19/12, 2011 at 12:0 Comment(2)

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). – Toupee 19/12, 2011 at 12: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. – Lodhia 19/12, 2011 at 21:55

B

27

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.

Boris answered 19/12, 2011 at 12:41 Comment(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 – Galatea 19/12, 2011 at 15:52

A

2