Page MenuHomeFreeBSD

read builtin: Empty variables on timeout
ClosedPublic

Authored by bdrewery on Tue, Sep 7, 7:05 PM.

Details

Summary

This matches how a non-timeout error is handled.

Test Plan

read10.0 passed before, read11.0 failed before. Both pass.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

freebsd_quinteiro.org added inline comments.
bin/sh/miscbltin.c
233–234

What about a status = (128 +..; goto out; here?

346

And an out: label here? I know gotos aren't always preferred. I saw other uses of them in this code, especially in eval.c.

This is more consistent, and probably more useful (while read loops tend not to treat EOF and read errors differently).

The whole error behaviour does not appear to match what most non-ash shells such as bash, ksh93, mksh and yash do (an immediate read error such as caused by <&- or 0>/dev/null leaves the variables unchanged), but POSIX seems to permit both (Consequences of errors in the read page "Default" pointing to XCU 1.4 Utility Description Defaults).

bin/sh/miscbltin.c
346

It is possible, but I'm not sure it is better.

Note that some ++ would have to be rearranged as well.

bin/sh/tests/builtins/read11.0
4

Existing tests use mktemp -d, placing the fifo into a temporary directory, to avoid the discouraged -u option.

7

Hmm, how portable is this? It is probably faster than forking off a child process to open the fifo for writing, which the standard requires and other tests do.

11

Some other tests do stuff like [ "$r" -gt 128 ] && [ "$(kill -l "$r")" = TERM ] to avoid depending on signal numbers.

bin/sh/miscbltin.c
346

Yep, you're right. This version doesn't set the current argument to the empty string, and the other one does. You would have to ap++ before coming here. Never mind.

bdrewery marked 2 inline comments as done.
  • test feedback
  • avoid races in read11.0 test
This revision is now accepted and ready to land.Fri, Sep 17, 3:27 PM
This revision was automatically updated to reflect the committed changes.