In D27310#610450, @kib wrote:

I do not see this EINTR and EINPROGRESS change in the most recent version of POSIX I have, IEEE Std 1003.1™-2017.

The referenced austin group ticket even went as far as propose posix_close() with some retry flag, which again was not added. More, there were strong objections from Linux side arguing that EINTR behavior should be as it is de-facto implemented by Linux and FreeBSD: file is always closed.

Restarting a close() would indeed be very bad, but returning [EINTR] might cause userland to do the same.

Having this kind of possibly mysterious errors documented is very useful.

My suggestion is to change all the exit statuses that were changed to sysexits codes here to 2 instead.

In D27216#607710, @gbe wrote:

In D27216#607705, @jilles wrote:

A pthread implementation "libc_r" was already added in 2.2-release, which contained most of these functions.

A pthread implementation "libc_r" was already added in 2.2-release, which contained most of these functions.

Exit statuses should implement a protocol between the calling and called process. Since only 8 bits (or 32 if the calling process uses waitid()) are available, there is not much flexibility. I think distinctions between different exit statuses should have a purpose, while most of the sysexits codes categorize errors without a clear purpose. If more flexibility is needed, a channel with more capacity should be used.

This complies to https://www.austingroupbugs.net/bug_view_page.php?bug_id=508 which is planned for POSIX issue8 (in a few years).

Perhaps it is better to match sockstat (D26413) and make the option -C instead of -c.

I found a reason to touch this code, but otherwise I'm a bit surprised about a change here.

This change may cause breakage in setups where the chroot does not have /etc/pwd.db and related files. Given that chroot(8) has worked like this since 2003, why change it now?

I think the best way forward is to start with -@ first and leave -l output as it is, at first.

Since struct timeval is mostly obsoleted by struct timespec, I suggest adding this only when there is existing code using it.

In D25033#551601, @kevans wrote:

In D25033#551509, @jilles wrote:

The dup2 part looks good.

The dup2 part looks good.

Although there is nothing wrong with this per se, applications can deal with this problem more easily using getline(3) (which is also standard).

read(2) on a directory seems only useful for educational or debugging purposes. These purposes are better served by a different API.

Feel free to commit this revision as is. I will handle -h in mkbuiltins.

A follow-up is to delete -h from mkbuiltins and builtins.def.

This solution looks inconsistent. Either fc and bind are always available (writing an error message if history is not compiled in), in which case mkbuiltins should not be omitting them, or they are not available if history is not compiled in, in which case the functions should not be defined at all if history is not compiled in.

Disabling utx boot and utx shutdown does not prevent other parts of the system writing to the utx files. So by itself this disable creates utx files that are less useful. Disabling utx completely needs patching, at least the way things are now.

Some people wrongly add whitespace via copy and paste, but let's be consistent with login, passwd, etc.

Fine with me, assuming it still builds with recent GCC as well.

MAINTAINERS says openssh is maintained by des, who accepts reviews via email only (no phabricator).

In D23802#523379, @kib wrote:

The buffer was written by the function, this is my point of the aliasing breakage.

The bin/sh part looks OK.

What is the point of fputc_unlocked() given that putc_unlocked() already exists?

Hmm, https://reviews.freebsd.org/D17083 proposes to change qsort_r() argument order to glibc's since that is likely to become POSIX standard and is slightly better than our order.

In D12773#508215, @cem wrote:

In fact, a very similar scenario is possible _today_ with sigprocmask:

The mq_notify() mechanism indeed needs to be considered here, but I think it already handles that:

Adding INTOFF/INTON around malloc/free prevents unsafety due to interrupting the allocator, but does not prevent memory leaks or double frees due to the administration getting out of sync with the allocator (for example, if the INTON in ckmalloc() notices an interrupt, the allocated memory will leak). It would make more sense if ckmalloc/ckrealloc/ckfree merely asserted that INTON is in effect. I experimented with that earlier but did not follow through.

I think the part "to all processes with the same uid as the user" is rather vague now, and would probably be better written as "to all processes which the caller has permission to", given that permission checks are quite complicated nowadays.

Historically, another reason was performance. For example, https://svnweb.freebsd.org/base?view=revision&revision=76801 which changed make(1) to be statically linked states performance as the reason.

Looks good with one grammar issue. Don't forget to bump .Dd.

This seems useful.

In D21903#478488, @kevans wrote:

In D21903#478452, @kib wrote:

I find it somewhat strange that you require global control to enable local setting. Less cryptic, I would expect to see per-process setting of 3 cases: 1. default/use sysctl, 2. enabled, 3. disabled.

Very useful, and good generalization of shm_open(SHM_ANON) and memfd_create().