Comment on

This code can be trimmed down a lot.

$stdin, $stdout and $stderr are never used, so they should be deleted.

Flushing does nothing for input handles, so $dout->autoflush(1);, $derr->autoflush(1);, $dout->flush(); and $derr->flush(); do nothing.

$din->flush(); is useless for two reason: 1) The program on the other side of the pipe has ended, and 2) the close does a flush.

$din->autoflush(1); is not needed since we don't write to $din. Even if we did, open3 makes it autoflush for us.

Since we don't do anything with $din, $dout and $derr anymore, we can let open3 create them for us.

Filehandles are automatically closed when they go out of scope.

The $pid is for a dead process, so why return it to the caller?

It's more common to use 0/1 for boolean instead of "false"/"true". Deviations make it less readable, and strings make it more complicated. Using a regular expression to check the equality of two strings is overkill. eq is the code for that.

Reformatting $out and $err has nothing to do with OSexecute and should be done by the calling function if it so desires.

There are many small problems.

$stdin->add("\*STDIN"); should be $stdin->add(\*STDIN);.

$val = waitpid(-1, 0); should be $val = waitpid($pid, 0);. $val isn't used, so the assignment can be ommited as well.

"@execute" will cram all the arguments into one word.

Why change a stdout and stderr to 1 if it's empty, when it's just as easy to check for the empty string ($str eq '' and length($str)

$stdout is changed from to 1 if it's '0'. Same for $stderr.

Returning a hash is... odd. If the code did return ($stdout, $stderr);, the caller could very simply do my ($stdout, $stderr) = OSexecute(...);

Cleaned Up Code

# RETURNS: ( $stdout, $stderr )  Both are refs to an array of lines.
sub OSexecute {
    my @execute = @_;

    local $, = ' ';  # for print "...@execute..."

    my $debug = 0;
    #$debug = 1;
    print("OSexecute(@execute)\n") if ($debug);

    # Here is the actual execution.
    my $pid = eval { open3($din, $dout, $derr, @execute) };
    die "OSexecute(@execute): $@" if ($@);

    # Wait for process to complete.
    waitpid($pid, 0);

    # Gather the results
    my @stdout = <$dout>;
    my @stderr = <$derr>;

    # We should check the return code of the child.
    # Gotta trap SIGPIPE for that.

    return ( \@stdout, \@stderr );
}
[download]

There's a big problem

{
   my $file_name;

   foreach $file_name ('c:\\tinyfile.txt', 'c:\\biggfile.txt') {
      my ($stdout, $stderr);

      print("$file_name\n", ("=" x length($file_name))."\n", "\n");
      ($stdout, $stderr)
         = OSexecute('cmd.exe', '/c', 'type', "\"$file_name\"");
      print("stdout\n", "------\n", @$stdout);
      print("Nothing was sent to STDOUT.\n") unless (@$stdout);
      print("\n");
      print("stderr\n", "------\n", @$stderr);
      print("Nothing was sent to STDERR.\n") unless (@$stderr);
      print("\n", "\n");
   }
}
[download]

outputs

c:\tinyfile.txt
===============

stdout
------
foo
bar
bla

stderr
------
Nothing was sent to STDERR.


c:\biggfile.txt
===============
*** HANGS ***
[download]

The problem is that file handles (including $dout and $derr) have a buffer that's limited in size. biggfile.txt is less than 2KB, which is really quite small, so this needs to be fixed.

Fix

# RETURNS: ( $stdout, $stderr )  Both are refs to an array of lines.
sub OSexecute {
    my @execute = @_;

    my @stdout;
    my @stderr;

    local $, = ' ';  # for print "...@execute..."

    my $debug = 0;
    #$debug = 1;
    print("OSexecute(@execute)\n") if ($debug);

    # Here is the actual execution.
    my $pid = eval { open3($din, $dout, $derr, @execute) };
    die "OSexecute(@execute): $@" if ($@);

    my $select = IO::Select->new();
    $select->add($dout);
    $select->add($derr);

    my @ready;
    my $fh;

    # Gather the results
    while (@ready = $select->can_read()) {
       foreach $fh (@ready) {
          push(@stdout, <$fh>) if ($fh == $dout);
          push(@stderr, <$fh>) if ($fh == $derr);
       }
    }

    # Wait for process to complete and reap it.
    waitpid($pid, 0);

    # We should check the return code of the child.
    # Gotta trap SIGPIPE for that.

    return ( \@stdout, \@stderr );
}
[download]

Fixed?

Oops! select() (and IO::Select) doesn't work in Windows. That sucks. It means we're gonna have to use threads!!! I have no experience with threads, so maybe another day.

In reply to Re^3: Open3 and bad gut feeling : Finally by ikegami
in thread Open3 and bad gut feeling by coreolyn

Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!

Read Where should I post X? if you're not absolutely sure you're posting in the right place.

Please read these before you post! —

Posts may use any of the Perl Monks Approved HTML tags:

a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr

Outside of code tags, you may need to use entities for some characters:

	For:		Use:
	&		`&`
	<		`<`
	>		`>`
	[		`[`
	]		`]`

Link using PerlMonks shortcuts! What shortcuts can I use for linking?

See Writeup Formatting Tips and other pages linked from there for more info.


Pathologically Eclectic Rubbish Lister
	PerlMonks

Comment on

The best material for plates (tableware) is: