Thanks Mark. With the time crunch I elected to increase the precision of the statement to answer one question, even though it doesn't answer all of them, as you said. I'm definitely interested in improving this more as SME availability increases!

Planning to commit the minimal fix at D53767, is that OK for you?

"efi" vs. "eficom"? The limited contexts where comconsole should be used there days (does it presume x86/i368?)?

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Update per review, add missing change

Edited my comment with a more cleaned up version.

This works for me. It could be cleaned up more. Confirmed it prevents the original issue reported.

• check status == 1.
• handle -t 0 special, which avoids clock_gettime and doesn't sleep forever.

I like what you did here of calling it a module continuously.

improve timeout argument parser

# echo yes | /usr/bin/time ./sh -c 'read -t 5 n; echo $n'
yes
        5.00 real         0.92 user         4.05 sys

should be 0. We may need another test case using timeout(1).

add test case

This passes all tests. However, I noticed that we don't have full coverage for the read -t 0 case. The read11 test case verifies that read -t 0 returns 128 + SIGALRM if no input is available, but there is no test case that verifies that it succeeds if input is available.

This passes the sh tests but times out on poudriere. Looking into why.

fix -t 0 case

fix

Visual inspection: looks functionally equivalent, all options match their intend.
Test run: OK with 31 service jails (at least net_basic works ok).

My intuition is we'll need a SIGALRM handler which is going to get tricky to not interfere with user traps. I haven't thought on it deeply.

Pressing request changes because I gave a whole talk about how I want to get this out of the tree and have spent several months working on that.

Poudriere's test suite has more testing of this timeout and SIGALRM. It checks for timeout adjusting on [EINTR] and ensuring timeout runs within the period specified. If you want to try that you'll need to modify external/sh/miscbltin.c with the patch and then use make checkquick. Or I can check on the next patch.

We have a lot of different styles in use, and I think this one is a violation of the consistency patterns in the manual as a whole. I gave a talk about it at BSDCan '25. Here's a link youtu.be/RthIOXpwwsM

Yeah I read the if as a while and forgot that FreeBSD does not update tv when select() is interrupted. I'll work on this some more.

Looks like I forgot to put a Differential Revision: tag on that commit.

kaizen I guess

@aly_aaronly.me please don't re-open a review to report an issue, since it won't have the desired effect and just adds confusion. It looks like the text was changed here and the GitHub pull request has been merged and I think there's nothing more to do.

Hmmm, I wonder if what might be better is to rework the existing apm.4 manpage to say it is an interface (instead of a driver), but not merge all its content over into acpi.4. It's more that this is a way to describe an API than a device driver. There would be no real SYNOPSIS in this case in the classic sense (we don't really have a model for a SYNOPSIS for something that is an abstract interface as opposed to a foo.ko you can load). That is, you would take the existing apm.4, and make the following changes:

I would love this. This change should probably also revert the commit I just made to strip "device iwx" from iwx.4 synopsis so it's atomic.

Hadn't noticed this new revision (Phabricator mail sending seems to be delayed), so reposting the relevant part of the comment I added on D47878 in the meantime: