Beefy Boxes and Bandwidth Generously Provided by pair Networks Joe
No such thing as a small change
 
PerlMonks  

Comment on

( #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":



  • 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:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.
  • Log In?
    Username:
    Password:

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

    How do I use this? | Other CB clients
    Other Users?
    Others chanting in the Monastery: (9)
    As of 2014-04-19 08:29 GMT
    Sections?
    Information?
    Find Nodes?
    Leftovers?
      Voting Booth?

      April first is:







      Results (478 votes), past polls