only clear the stream error state in readline() for a good nextargv()

[tonycoz]

This would previously clear the stream error state in any case
where sv_gets() failed and the error state was set.

Since this now leaves the error state on the stream, the close()
calls in the test updated by this commit would fail, changing its
output. Since the result of those closes didn't seem related
to the purpose of the test, I changed it not throw an error on
either close() failing.

Fixes #20060

[bram-perl]

The code change looks good but what I'm missing:

• a perldelta entry (I'm not sure when these are normally added): perl's own test suite was "impacted" by the bug fix so it's possible(/likely?) other code out there will be too; so I think it certainly should be mentioned in perldelta
• a test for the new behavior (i.e. that the error doesn't get cleared when it shouldn't)
• a test for the old behavior (i.e. that the error gets cleared when it should)

For that last one:
What I tried was to completely remove Perl_clearerr() call in Perl_do_readline and then tried to come up with a test case where it would behave differently.
So far I've not been successful.. All seem to work as expected...
Digging a bit deeper: it appears that nextargv() will call a do_open-variant and these all appear to call S_openn_cleanup which appears to call PerlIO_clearerr() when the open succeeded..
In other words: I don't know when the Perl_clearerr() in Perl_do_readline is actually needed.. Can anyone come up with a test case?

[tonycoz]

• a perldelta entry (I'm not sure when these are normally added): perl's own test suite was "impacted" by the bug fix so it's possible(/likely?) other code out there will be too; so I think it certainly should be mentioned in perldelta

I never include perldelta updates in PRs - they're almost guaranteed to conflict with other updates to perldelta. But I could talk about it here, maybe something like:

(under incompatible changes)
=head2 readline() no longer clears the stream error flag

C<readline()>, also spelled C<< <> >>, would clear the handle's error flag after an error occurred on the stream.

The error flag is now only cleared when processing starts on a new file when reading the C<ARGV> stream.

Since the error flag is no longer cleared calling `close()` on the stream may fail and if the stream was not explicitly
closed, the implicit close of the stream may produce a warning.

• a test for the new behavior (i.e. that the error doesn't get cleared when it shouldn't)

This is reasonable, I'll look at it.

• a test for the old behavior (i.e. that the error gets cleared when it should)

I don't see a way to do that, since the error is only cleared for nextargv() we'd need to populate @ARGV with a file that can be opened successfully for reading, but then produces an error (not eof) when read.

[bram-perl]

• a test for the old behavior (i.e. that the error gets cleared when it should)

I don't see a way to do that, since the error is only cleared for nextargv() we'd need to populate @ARGV with a file that can be opened successfully for reading, but then produces an error (not eof) when read.

Doing some more testing:

On my system reading from /dev/ttyS3: open succeeds, read fails:

    $ strace perl -ne '' /dev/ttyS3
    ...
    open("/dev/ttyS3", O_RDONLY)            = 3
    ioctl(3, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, 0x7ffce37139c0) = -1 EIO (Input/output error)
    lseek(3, 0, SEEK_CUR)                   = -1 ESPIPE (Illegal seek)
    fstat(3, {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 67), ...}) = 0
    fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
    read(3, 0x633640, 8192)                 = -1 EIO (Input/output error)
    close(3)                                = 0

So open succeeds, read fails so that is a good test case.

Testing with my own fh:

old perl:

    $ perl  -MIO::Handle -wle 'open my $fh, "<", "/dev/ttyS3" or die "open failed: $!";  <$fh> or warn "Readline failed: $!";  print $fh->error();'
    Readline failed: Input/output error at -e line 1.
    0

your PR:

    $ ./perl -Ilib -MIO::Handle -wle 'open my $fh, "<", "/dev/ttyS3" or die "open failed: $!";  <$fh> or warn "Readline failed: $!";  print $fh->error();'
    Readline failed: Input/output error at -e line 1.
    1

blead without PerlIO_clearerr() in Perl_do_readline:

    $ ./perl -Ilib -MIO::Handle -wle 'open my $fh, "<", "/dev/ttyS3" or die "open failed: $!";  <$fh> or warn "Readline failed: $!";  print $fh->error();'
    Readline failed: Input/output error at -e line 1.
    1

So that looks good.

Testing with ARGV:

old perl:

    $ perl -MIO::Handle -wle '<ARGV> or warn "Readline failed: $!";  print ARGV->error();' /dev/ttyS3
    Readline failed: Input/output error at -e line 1.
    -1

your PR:

    $ ./perl -Ilib -MIO::Handle -wle '<ARGV> or warn "Readline failed: $!";  print ARGV->error();' /dev/ttyS3
    Readline failed: Input/output error at -e line 1.
    -1

blead without PerlIO_clearerr() in Perl_do_readline:

    $ ./perl -Ilib -MIO::Handle -wle '<ARGV> or warn "Readline failed: $!";  print ARGV->error();' /dev/ttyS3
    Readline failed: Input/output error at -e line 1.
    -1

No change in behaviour.

ARGV->error() returns -1 because ARGV is closed (Perl_do_readline closes it on error`).

Which really means: as far as I can tell you can't check if reading from ARGV failed by using ARGV->error() since that will either return 0 (no error) or -1 (closed/invalid handle) never 1.

So again I'm at the state where I don't see when PerlIO_clearerr() in Perl_do_readline does something useful/something observable :/

TLDR:

I (still) question if/when doing a PerlIO_clearerr() inside if (fp) { is needed.

[bram-perl]

After writing the previous message I started wondering about something else: why does close return an error?

Old message

Again testing with /dev/ttyS3 (where open succeeds but read fails with EIO):

    $ cat tt.pl
    open my $fh, "<", "/dev/ttyS3" or die "open failed: $!";
    <$fh> or warn "Readline failed: $!";
    close $fh or warn "Close failed: $!";

    $ ./perl tt.pl
    Readline failed: Input/output error at tt.pl line 4.
    Close failed: Input/output error at tt.pl line 5.

So according to that I would expect that close returned an error;
So let's check it using strace:

    $ strace ./perl tt.pl
    ...
    open("/dev/ttyS3", O_RDONLY|O_CLOEXEC)  = 3
    ioctl(3, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, 0x7ffd96ae9050) = -1 EIO (Input/output error)
    lseek(3, 0, SEEK_CUR)                   = -1 ESPIPE (Illegal seek)
    fstat(3, {st_mode=S_IFCHR|0660, st_rdev=makedev(4, 67), ...}) = 0
    read(3, 0x986520, 8192)                 = -1 EIO (Input/output error)
    write(2, "Readline failed: Input/output er"..., 53) = 53
    close(3)                                = 0

The close system call returned 0, i.e. no error.
So why are we reporting an error on close?

From perldoc -f close:

    Closes the file or pipe associated with the filehandle, flushes
    the IO buffers, and closes the system file descriptor. Returns
    true if those operations succeed and if no error was reported by
    any PerlIO layer.

=>

• 'flushes the IO buffers': it was a read fh so there are no buffers to flush
• 'closes the system file descriptor': closing the file descriptor succeeded (as shown by strace)
• 'no error was reported by any PerlIO layer': that could mean anything I guess..

The question: would one expect that a failed read results in a failed close?
Especially when the actual close syscall succeeded?

Update doing some more testing shows the same behavior when writing to a file.
That is: when the write syscall returns an error then Perl's close will report that error even when the actual close syscall succeeded.

So applying the same behavior to read looks OK.

Write example

Using a tmpfs of 1M:

$ mkdir /tmp/bla
$ mount -o size=1M  -t tmpfs tmpfs /tmp/bla
$ perl -wle 'open my $fh, ">", "/tmp/bla/foo"; print $fh "a" x (2 ** 19) or warn "print 1 failed"; print $fh "a" x (2 ** 19) or warn "print 2 failed"; close $fh or warn "close failed: $!"'
close failed: No space left on device at -e line 1.

$ strace perl -wle 'open my $fh, ">", "/tmp/bla/foo"; print $fh "a" x (2 ** 19) or warn "print 1 failed"; print $fh "a" x (2 ** 19) or warn "print 2 failed"; close $fh or warn "close failed: $!"'
...
write(3, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 8192) = 8192
write(3, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 8192) = 8192
write(3, "a\n", 2)                      = -1 ENOSPC (No space left on device)
close(3)                                = 0

Note:

• at the system level: errors reported on the last write, no error on close;
• at the 'Perl'-level no errors were reported on print, only on close;

[bram-perl]

(I still wonder if/when PerlIO_clearerr is needed but that shouldn't hold back this PR)

[tonycoz]

Looking at the flow through that code, do_open6() and nextargv() it looks like the clearerr() is only needed for globs:

• nextargv() doesn't call do_close(), it just calls do_open6(), which calls openn_setup() which PerlIO_close()s the input stream instead of calling do_close() or io_close(). In nextargv() new PerlIO objects are created, presumably with the error flag cleared, so the clearerr() I moved has no effect.
• the glob closing code does call do_close(), but always treats such a failure as a child process exit failure, so it probably should keep on doing clearerr()
• the remaining cases are for user opened files/sockets/pipes, in which case we should preserve the error.

Note that the implicit close warning is only ever produced for an IO object going out of scope, not for a file reopened:

open FH, "<", "foo";
# do something to put $fh into error
open FH, "<", "bar"; # stream was in error state, but no warning

I've re-worked it to only clear the error for glob(), which I think is correct(-ish), but should only have an effect for miniperl.

[demerphq]

On Thu, 18 Aug 2022 at 02:39, Tony Cook ***@***.***> wrote: - a perldelta entry (I'm not sure when these are normally added): perl's own test suite was "impacted" by the bug fix so it's possible(/likely?) other code out there will be too; so I think it certainly should be mentioned in perldelta I never include perldelta updates in PRs - they're almost guaranteed to conflict with other updates to perldelta.

IMO it would be nice to have a solution to that. If perldeltas changes were added as new filelets and assembled later it might make people more inclined to add new perldeltas. That would avoid any conflicts and make it easier to see what changes came from what PR's. Eg, instead of changing perldelta you create perldelta_20103.pod, it contains the delta changes from PR #20103, this one. When we do the release process we construct perldelta from the pieces. cheers, Yves

…

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[bram-perl]

I've re-worked it to only clear the error for glob(), which I think is correct(-ish), but should only have an effect for miniperl.

Do you have an example of when it's needed?
(I did try several things - with miniperl - but so far I've not yet found a case where it appears to make a difference, so I'm wondering what I'm missing..)

[tonycoz]

Do you have an example of when it's needed?

I don't see a specific case - it would have to be read() failing before sv_gets() has read anything, and the cases where read() fails where we don't otherwise handle it (EINTR can happen and is handled).

On POSIX systems I don't think it can happen unless something else is badly wrong, none of EAGAIN, EFAULT, EBADF, EIO, EISDIR should happen.

It might be different on non-POSIX systems.