Page MenuHomeFreeBSD

read builtin: Empty variables on timeout
ClosedPublic

Authored by bdrewery on Sep 7 2021, 7:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 10, 11:58 AM
Unknown Object (File)
Mon, Dec 9, 10:50 AM
Unknown Object (File)
Thu, Nov 28, 8:27 PM
Unknown Object (File)
Nov 20 2024, 2:28 PM
Unknown Object (File)
Oct 26 2024, 1:55 PM
Unknown Object (File)
Sep 28 2024, 6:26 AM
Unknown Object (File)
Sep 21 2024, 7:15 AM
Unknown Object (File)
Sep 21 2024, 2:59 AM

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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
5

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

8

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.

12

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.Sep 17 2021, 3:27 PM
This revision was automatically updated to reflect the committed changes.