Beefy Boxes and Bandwidth Generously Provided by pair Networks
Do you know where your variables are?
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??

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 ); }

    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"); } }

    outputs

    c:\tinyfile.txt =============== stdout ------ foo bar bla stderr ------ Nothing was sent to STDERR. c:\biggfile.txt =============== *** HANGS ***

    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 ); }

    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

    Title:
    Use:  <p> text here (a paragraph) </p>
    and:  <code> code here </code>
    to format your post; it's "PerlMonks-approved HTML":



    • Are you posting in the right place? Check out Where do I post X? to know for sure.
    • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
      <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
    • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
    • Want more info? How to link or How to display code and escape characters are good places to start.
    Log In?
    Username:
    Password:

    What's my password?
    Create A New User
    Domain Nodelet?
    Chatterbox?
    and the web crawler heard nothing...

    How do I use this?Last hourOther CB clients
    Other Users?
    Others exploiting the Monastery: (4)
    As of 2024-03-29 12:39 GMT
    Sections?
    Information?
    Find Nodes?
    Leftovers?
      Voting Booth?

      No recent polls found