It doesn't look like you're handling partial reads or other exceptional conditions in your call to sysread(). Something like this might be a start.

use POSIX qw(EINTR EWOULDBLOCK EAGAIN);
...
# Try to read 4096 bytes from $fh using sysread().  Icky.

my $data;
my $offset = 0;
my $length = 4096;

while($length > 0) # Handle partial reads
{
  my $read = sysread($fh, $data, $length, $offset);

  unless(defined $read)
  {
    next  if($! == EINTR || $! == EWOULDBLOCK || !$ == EAGAIN);
    # Handle other errors here
    ...
  }

  last  if($read == 0); # EOF

  $offset += $read;
  $length -= $read;
}
[download]

In practice, the details are somewhat reliant on the exact nature of the read(2) system call on your OS. The code above almost certainly does NOT handle all potential errors, but it's a start.

The big point is that you can't expect sysread() to actually read as much data as you ask it to read. At the very least, you have to handle partial reads and then either loop until you've read what you want (as shown above) or make some other sort of decision.

What you definitely don't want to do is treat something like an undef return caused by EINTR as a fatal error. That type of thing tends to crop up at the worst times (e.g., on a busy machine but never on your idle development box).

sysread() has similar annoyances associated with it. If it's at all possible to use the blocking versions (read() and write()) it will make your life a lot easier.

It doesn't look like you're handling partial reads

He does. Relevant code:

while(my @ready = $select->can_read) {
...
      my $data;
      my $length = sysread $fh, $data, 4096;

...
           $processeddata .= $data;
...
}
[download]

All the data ends up in processeddata, no matter how much is read by a given call to sysread.

It doesn't look like you're handling ... other exceptional conditions

He does this too. Relevant code:

      if( ! defined $length || $length == 0 ) {
        $err .= "Error from child: $!\n" unless defined $length;
        $select->remove($fh);
      }
[download]

The filehandle is removed from the list of handles monitored by select. I'm guessing can_read will return false when it no longer monitors any handles.

Something like this might be a start.

That's bad! You removed select, which is cruicial here.

All the data ends up in processeddata, no matter how much is read by a given call to sysread.

Oops, my bad. Actually, hm, I'm not sure what happens without the offset adjustments that I have in my code...

It doesn't look like you're handling ... other exceptional conditions

He does this too. Relevant code:

      if( ! defined $length || $length == 0 ) {
        $err .= "Error from child: $!\n" unless defined $length;
        $select->remove($fh);
      }
[download]

That treats things like EINTR as "fatal" errors (stop listening on that $fh), which is probably not what he wants.

You removed select, which is cruicial here.

I was just commenting on the vagaries of sysread().

1) The docs say "open3() does not wait for and reap the child process after it exits." You need to call waitpid on children that have completed.

2) Your return value are not the best. You return true when when open3 fails. You also return true on success.

3) I find it odd that you return undef when can_read returns a file handle you didn't give it. It shouldn't happen, so I would just ignore the error, since it shouldn't happen anyway. The safest way to handle it would be to do something like:

$err .= "Error from select: can_ready returned bad handle\n";
$select->remove($fh);
[download]

Re #3 - I'm not sure I follow. If it shouldn't happen, it shouldn't matter how it's handled.

Besides, I'd rather be woken up in the middle of the night by something that shouldn't happen but did, rather than mask the error by ignoring it. My house shouldn't catch fire, for example... ;-)

Besides, I'd rather be woken up in the middle of the night by something that shouldn't happen but did, rather than mask the error by ignoring it.

The dieing would be a better alternative then returning undef, which simply displaces the error to some other indeterminate location.

The function only returns undef in that instance. I hate having some generic function die on me unexpectedly, especially when my boss calls me up shouting "You're stoopid program doesn't work!" I'd rather leave the choice to the calling program:

callexternalpgm( parms ) or die "$!";
[download]

or, more likely,

callexternalpgm( parms ) or LogIt( "Error calling external program: $!
+" );
[download]

Harley J Pig

Thank you. That was some very informative reading.
Harley J Pig