• -F
• Drop not useful TARGET writability test

• Discard header from template (both from new install and compat import of existing motd)
• Explicitly set template file permission when created from legacy motd
• Simplify replacing preexisting non-symlink /etc/motd
• Correct writability test for generated TARGET
• Simplify construction of /var/run/motd from template with header discarded

Sorry about the delay! I've been traveling and not very available. Thanks for the feedback, I will try to address it now.

LGTM, thanks.

In D20892#454600, @rajesh1.kumar_amd.com wrote:

@cem, sorry about the delayed response.
I was trying to use the "arc" tool for updating the patch. But it asks to create a new commit as below. I am unsure whether to accept it.

I'd just drop the extra * on the other of the definition/declaration, but I don't know if we have a style(9) preference. Functionally / semantically it looks good.

The general idea of the change looks good to me, thanks!

No objection.

In D18774#452750, @rajfbsd_gmail.com wrote:

Thanks @mav for reviewing and pushing the patch upstream. I verified it to work properly. I have some more minor changes to this Driver (basically to add support for another PCI Device ID). Shall I submit the additional patch here itself? or should I open another review for the same?

FWIW, I also work on an embedded appliance.

I would change the commit summary/title, though. Really we're just disabling a feature when it is unsupported by some particular configuration; it's not bhyve-specific.

Looks good to me.

Thanks!

LGTM. For future reference, please upload patches with more context. That could mean using the arc command line tool, which simplifies the process somewhat once it is set up, or diff -U99999 if you prefer not to use the arc ("Arcanist") tool.

In D20324#450028, @trasz wrote:

But I've added the justification (explanation of what's the point of ARB, from the consumer point of view) to the man page already. What else is needed?

In D20116#448990, @trasz wrote:

In D20116#448878, @cem wrote:

I think there are open questions around rounding behavior and making the extensive macros into functions.

The problem with functions is that in many places it uses Q_TC() (which boils down to __typeof()), or macros that expand to Q_TC(). I guess I could try to rework the ones which don't, but that would look inconsistent.

In D20324#449008, @trasz wrote:

So... is there anything else left to do here?

The design seems kind of poor (random last 32k corruption?). Alternatively / relatedly, we (ISLN) have a syslogd patch we can share that parses newsyslogd.conf and manually invokes newsyslogd directly when logs grow beyond configured rotation size.

In D20116#448783, @trasz wrote:

So... is there anything else left for me to do here before this can get committed?

Thanks, Benedict!

• Fix a typo in rc.d/motd
• Update login(1) and motd.5

Thanks!

In D20710#447771, @asomers wrote:

Wrap in vfs_vnops.c at line 502. Currently, the largest possible overflow is about 2x. So using an int16_t makes it safe.

In D20710#447768, @asomers wrote:

FYI, I discovered that f_seqcount needs to be int16_t rather than int8_t in order to detect an overflow. Otherwise the slightest overflow of IO_SEQMAX will wrap.

Thanks!

Need to change fifofs to actually use f_pipegen instead of f_seqcount :)

Rebase on recent HEAD

In D20710#447668, @asomers wrote:

Actually, I don't think the usage in fifo_vnops.c is a potential overflow. AFAICT fifos use that field for a completely different purpose. It really should be a different field (and in fact, it used to be). See r238936.

Also: nice find! :)

Looks like you could get a similar overflow by having 64k readers on a pipe too (fs/fifofs/fifo_vnops.c). You could also overflow in sequential_heuristic() if uio_resid is big enough (especially with a narrower f_seqcount).

Adding Kirk for UFS change.

Ah, I was thinking of FIEMAP I think. So the better question might be, why not add FIEMAP instead? https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt

Hi @delphij , @markm . Is this still something that seems reasonable or should I drop it?

The name, etc, beg the question: what's the motivation to differ from FIOBMAP in Linux (or NetBSD)?

As mjg@ points out, the mktemp file may be on a different filesystem than /etc
(e.g., tmpfs, but any other filesystem invalidates the initial fsync). Copy to
a temporary /etc file, sync that, then rename over /etc/motd.

In D20458#447283, @avg wrote:

I encountered a technical problem trying to replace the printf with KASSERT.
bus.h does not include systm.h and there are multiple places where bus.h is included before systm.h (or systm.h not included at all).

Rename uint128_add to the more appropriate uint128_add64

As a first-cut workaround, LGTM.

Thanks!

(<sys/param.h> includes <sys/types.h>; do not include both.)

In D20116#445849, @trasz wrote:

Regarding the LUT idea - it does make sense, but I believe it would require somewhat complicated code surgery, and I'd rather not do it at this time: it's a vendor code which I'm trying to upstream, not to rewrite. It also sounds like something that can be done later, if neccessary.

(bump .Dd if needed)

Thanks for splitting up the pages.

In D20312#444271, @delphij wrote:

For the microoptimization: Note that it's possible to avoid the additional branching by doing unlock in both cases, e.g.:

Rebase on D20312 changes

@delphij , this one technically touches sys/dev/random (fix a declaration name typo but mostly add #ifndef _KERNEL compatibility shims), so I need secteam blessing to commit. I'd appreciate a look when you get a chance.

• Drop DELAY() entirely
• Minor optimization: drop global lock over 1-15 bytes of memcpy and 16 bytes explicit_bzero.