Whoops. LGTM.

Results here: https://github.com/kevans91/freebsd/actions/runs/11114004259

Simplify; this was previously going to go the route of atf-sh and inject the
shebang, but what we ended up with does not warrant a tmp file at all. We can
also take care of +x + move in one step.

In D46806#1067666, @ngie wrote:

Could you please demo the test program (attach the kyua html report)?

In D46805#1067528, @ngie wrote:

It seems like this should be named "PORCH_TESTS" instead of "PLAIN_TESTS_PORCH" (that seems pretty wordy..).
Or maybe something LUA-prefixed?
PLAIN_TESTS is the variant of test which has a certain expected/documented behavior, whereas, _CXX, _C, _SH are the file types under test. Are all "PORCH" tests expected to exit on 0 if successful and exit non-zero if failed?

In D46748#1066019, @otis wrote:

In D46748#1065822, @kevans wrote:

Can you explain more on why you think you want this? The entire point was to hide the interface and avoid it becoming part of the ABI we need to care about.

To be able to compile anything using be.h (except from bectl).

FWIW, take this simple code:

otis@jb131:~/work/libbe % cat m.c
#include <be.h>

int main(int argc, char *argv[])
{
        libbe_handle_t *lbh;

        lbh = libbe_init(NULL);
        libbe_close(lbh);
        return 0;
}

and use:

cc -o x x.c -lbe

then the result is:

otis@jb131:~/work/libbe % cc -o m m.c -lbe
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:160:15: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  160 | _SYS_NVPAIR_H uint_t nvlist_nvflag(nvlist_t *);
      |               ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:184:11: error: unknown type name 'boolean_t'
  184 |     const boolean_t *, uint_t);
      |           ^
/usr/include/sys/nvpair.h:186:11: error: unknown type name 'uchar_t'
  186 |     const uchar_t *, uint_t);
      |           ^
/usr/include/sys/nvpair.h:218:5: error: unknown type name 'boolean_t'
  218 |     boolean_t *);
      |     ^
/usr/include/sys/nvpair.h:219:70: error: unknown type name 'uchar_t'
  219 | _SYS_NVPAIR_H int nvlist_lookup_byte(const nvlist_t *, const char *, uchar_t *);
      |                                                                      ^
/usr/include/sys/nvpair.h:239:5: error: unknown type name 'boolean_t'
  239 |     boolean_t **, uint_t *);
      |     ^
/usr/include/sys/nvpair.h:239:19: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  239 |     boolean_t **, uint_t *);
      |                   ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:240:70: error: unknown type name 'uchar_t'
  240 | _SYS_NVPAIR_H int nvlist_lookup_byte_array(nvlist_t *, const char *, uchar_t **,
      |                                                                      ^
/usr/include/sys/nvpair.h:241:5: error: unknown type name 'uint_t'; did you mean 'uint8_t'?                                                                         [55/1849]
  241 |     uint_t *);
      |     ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:243:5: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  243 |     uint_t *);
      |     ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:245:17: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  245 |     uint8_t **, uint_t *);
      |                 ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:247:17: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  247 |     int16_t **, uint_t *);
      |                 ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:249:18: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  249 |     uint16_t **, uint_t *);
      |                  ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:
/usr/include/sys/nvpair.h:251:17: error: unknown type name 'uint_t'; did you mean 'uint8_t'?
  251 |     int32_t **, uint_t *);
      |                 ^
/usr/include/sys/_stdint.h:54:20: note: 'uint8_t' declared here
   54 | typedef __uint8_t               uint8_t;
      |                                 ^
In file included from m.c:1:
In file included from /usr/include/be.h:31:
In file included from /usr/include/libnvpair.h:29:

and so forth.

The other thing may be that something is not well documented.

In D40161#1065823, @ehem_freebsd_m5p.com wrote:

I suspect this needs to now be rebased onto main. I've got concerns, so I would like to take a look at what comes when the interaction with the FIQ stuff comes in. Simply asking looking for a temporary pause.

Can you explain more on why you think you want this? The entire point was to hide the interface and avoid it becoming part of the ABI we need to care about.

In D46670#1064616, @markj wrote:

In D46670#1064123, @aly_aaronly.me wrote:

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

I think your approach is nicer. (In general I kind of dislike using bool flags to modulate functions this way: it's hard to see what's going on without jumping to the function definition, and the control flow becomes more complicated. Here it's relatively straightforward, but refactoring a bit is nicer IMHO.)

Superceded by https://github.com/freebsd/freebsd-src/pull/1363

I still think we want the login.conf addition, even if it's pipe=unlimited -- we seem to do this for all of them (perhaps as an example of the possible limits that can be configured and their login.conf names?)

In D46619#1062675, @olce wrote:

In D46619#1062645, @kevans wrote:

I suspect we also want an addition to lib/libutil/login_class.c

Isn't the current addition of a line in the resources[] array in login_class.c enough?

I suspect we also want an addition to lib/libutil/login_class.c, and maybe a default in usr.bin/login/login.conf

Issue warnings for any permission change failures- they're not fatal, but the admin should know about them in case there's a legitimate issue.

In D46443#1058815, @jlduran_gmail.com wrote:

In D46443#1058814, @kevans wrote:

In D46443#1058744, @jlduran_gmail.com wrote:

draft_test.patch1 KBDownload

First draft of test.

JFYI- I can't see what this is referencing. In the "Referenced Files" section on the right, I can see:

Restricted File
Mon, Aug 26, 20:38

which lines up with the timestamp on your comment, so I guess permissions aren't set on it somehow.

Forgive my ignorance on Phabricator, I believe it is OK now.

Test looks reasonable to me, incorporating it as:

In D46443#1058744, @jlduran_gmail.com wrote:

draft_test.patch1 KBDownload

First draft of test.

In D46086#1057112, @andrew wrote:

You can get it from https://github.com/zxombie/freebsd/commit/9eebbc0464314b7515f59a7af8fbe3dda20fb94f, and https://github.com/zxombie/freebsd/commit/7ff0b43dbdcbbd31b3ff471adac1454bd15a516b to enable VHE when possible.

It's fine with me, but I note there's a test in contrib/netbsd-tests/usr.bin/grep/t_grep.sh that assumes the default here that will start failing

In D46086#1056982, @andrew wrote:

@kevans This should be a partial revert of rGd2ae03bae2add82124973876dec95eb126ff34c8. Can you test to make sure I didn't break anything before I commit?

I'm a little torn on the idea itself, but here's some review

In D41509#1055376, @david_crossfamilyweb.com wrote:

Updated the diff per requested copyright changes.

My intent was that I had implemented all of kevans' reivew changes already, lua-lint passes cleanly-ish. the only issues are about the 'hash' accesses, but those mirror 'gfx' in other lua files.

In D46095#1051560, @olce wrote:

These calls are arguably not necessary.

But IMO the real bug is, and always has been in situations like this, that a setlogin() call should never succeed except for session leaders. Would you recall any reason for the current setlogin()'s behavior?

In D46216#1054082, @0mp wrote:

Just an idea: perhaps this could be a sysctl. This way, you could set the setting once and interested programs could check the sysctl value before deciding on being verbose or quiet.

Fix the date (whoops :-)), -W -> -q and fix bracketing in usage

jhb has a similar patch here https://reviews.freebsd.org/D45995 and I had a similar fix upstream https://github.com/openssl/openssl/pull/24903/files but I've failed to get @bcran (or anyone in upstream OpenSSL) to weigh in on an appropriate solution for their part, so I guess just do it and I'll reconcile the differences later.

Consensus moves for the first attempt

And an accompanying review: D32615

In trying to figure out why I hadn't found this in my smoke-tests, I discovered I had fixed this in another branch from when I initially did the work. Attaching the alternate version, I'm not sure which I prefer; that one just avoids copying by making domain a pointer into domain-renamed-to-domain_storage.