Though I am not a big fan of these, as the src tree would need a massive quantity of these added to make much of a dent at all in documenting what part of what rfc is being implemented, I wont stand in the way of one more near useless comment.

The adding of blank lines between all the forward declarations seems wrong to me.

In D30050#674660, @grehan wrote:

The uuid is being correctly validated in smbiostbl.c:smbios_type1_initializer() so there's no need to do it again. An fprintf to stderr there indicatiing it is invalid is fine, and the assert on a return from smbios_build() can be changed into an exit(1).

I am accepting this without the IPSTAT_INC(ips_forward) issue being fixed, as it looks to me as if that is an existing and separate problem in the code. Probably a walk through should be done to see that ips_forward and ips_cantforward are all done correctly.

I would still like to see the "169.254.0.0/16" changed to IN_LINKLOCAL, purpose of macro is to locate this value one place and one place only, scattering this string in the code undoes that.

Thanks for chasing these down

This is already a good improvement and acceptable as is with Greg's corrections.

Though this is not exactly a normal service, to me listening on any by default is the correct thing to do here. One thing I would like to note, this code appears to only listens on one of IPv4 or IPv6, which is probably a short coming.

Hum, not a core system functionality? May I remind you that ftp://ftp.freebsd.org exists? Though it probably does not run the base system ftpd, it DOES run that functionality. Also so much for the project being a democracy, seems that core@ and security@ now simply dictate to developers@ how things are to be done... nothing personal Gordon, but that is exactly how this is coming across.

Rather than spend cycles on deprecating USED features, please spend cycles on getting pkg base done so that those that feel they have no need for things like ftp/ftpd/telnet/telnetd/foo/bar/etc can choose to not have these and those of us who live and die by such tools can choose to have them.

I am ok with doing this, though it still leaves all the other issues open.

Given the issues I would rather NOT add dscp support at all, until these issues can be addressed from down/upstream? The current situation, both in pfsense and freebsd is poor at best and may actually be one of the sources of issues we are seeing in our ECN data collection work. We know for a fact people are being told to use these ancient and obsolete TOS values by looking for them in blog posts and how-to's. Using the pry bar that is "reduce diffs between" as a reason to add a bad situation is an ever worse situation!

I'd be happy to extend the man page to make that clearer. Do you have any suggested phrasing?

I have a concern here, some of which goes beyond just this change, about aliasing DSCP to TOS. We (a small congestion control research group) have found that due to ambigious manual pages, headers and other factors, some present in PF it appears are causing mismarked traffic on the internet. DSCP is shifted left by the 2 ECN bits, the manual pages and any other documentation MUST be made explicity clear about this fact as to if your supplying the values as specified in the RFC"s on DSCP (unshifted) or are you applying TOS bits that may or may not have the ECN bits masked out.

This is a move in the right directions, at least this is state created by the installer.

Maybe a seperate pass over this code to try and clean up the fact that we have used the string "/efi" many places such that it should probably become some type of #define or derived from the system make file values so that any changes to this value can be done in one place and one place only and all things using it are fixed.

I personally prefer /efi, as I consider /boot to belong to FreeBSD and contain the FreeBSD specific boot code. EFI is a platform specific set of boot code and belongs to the system board, and though it may contain freeBSD specific code it may also contain OTHER code, especially in a multiboot environment.

In D28645#641152, @donner wrote:

In D28645#641097, @rgrimes wrote:

Infact it is probalby safer to assume a /24 than a /32.

No, please. It would assume, that the user is using the system in a home lan only.

/24 is not an assumption of home lab, it is the longest of the 3 prefixes that would of been picked before, and infact is probably most often the correct mask to be used.

I would like to see a survey of what some other OS's due in this situation. I have participated in discussions elsewhere on this issue and just blindly making these host only, IMHO, is probably going to silently cause a lot of failures on finger memory. Infact it is probalby safer to assume a /24 than a /32. BUTT I think the preferable solution here may be to return an error if the netmask is not specified, thus doing nothing rather than guessing and doing something wrong.

No statement about do_prr being on by default?

In D28545#639178, @donner wrote:

In D28545#639176, @rgrimes wrote:

In D28545#638947, @donner wrote:

• Get rid of "hent" instead of "host".

This types of change just cause lots of noise in this review that makes it harder to see the real change.

That's why this change is a secondary modification.
Please choose the relevant part in the revision tab "History"

In D28545#638947, @donner wrote:

• Get rid of "hent" instead of "host".

In D28410#636597, @rew wrote:

ping rgrimes@, does that sound reasonable to you?

Yes please

How is the default 2 minute "linger" timer being implemented now if this is dead code? iirc this value use to be used when a socket was closed on the server side to control how long one waited in close_wait for the client to finish up the connection (server initiated close.)

This fixes at least one dependance on /usr, I see another later, mktemp which is /usr/bin/mktemp. I would of marked it above, but a full file context was not uploaded to this review.

Though this is good intent, it is not a good approach. I'll assert again at one time we had erradicated all /usr/local from the base system, these above have crept in over time and again should be erradicated. There should never ever be a /usr/local #define in paths.h, having to recompile code to change this, again, is the wrong approach. This should _possibly_/_probably_ be moved to a kernel variable that can be changed at boot time.

The summary of this review is lacking detail

Is there any concern or additional work needed for ipV6?

To all those harping about it not being foo or bar or zed, this *is* an underlying frame work that is very flexible and allows one to implement what ever foo, bar or zed configuration you want on top of it. This leaves json, xml, or any other implementation out of the base layer code and those can be implemented in a layer above this. This has been discussed for a some time extensively by those that attend the semi-monthly bhyve call.

Approving as mentor only.

In D25876#574103, @chuck wrote:

...
Your observation that the data units values restart at zero is very valid. My tentative plan for this is to optionally save the current values when the VM is powered down using the new configuration file format. This saved device state could then be used on a subsequent restart to provide the continuity you describe.

Reflowing sentences and removal of .Tn seems like busy work to me. Much easier to just make mandoc ignore .Tn and igor silent on its usage. These macros DO make the groff man pages output look different, and IMHO, nicer.

I still see request changes markers from emaste and jhb/bhyve, what are these changes?

In D25833#572269, @emaste wrote:

In D25833#572248, @rgrimes wrote:

Could we please get stances in /etc/sshd/ssh{,d}_config commented out that show how to enable these if they are needed?

For example,

Ciphers +aes128-cbc,aes192-cbc,aes256-cbc

I'd rather not deviate from upstream, but if we must I'd rather do so in sshd_config, not in the source.

Could we please get stances in /etc/sshd/ssh{,d}_config commented out that show how to enable these if they are needed?

In D25793#572090, @rajesh1.kumar_amd.com wrote:

Can we add an appropriate SPDX tag to the source files as well - e.g. /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ or similar?

Need a clarification here please. The license text in the source files has additional text (part of it give below) apart from the normal BSD-3-Clause text. If this is not an issue, we can use /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */. Please clarify.

I have no objection to it, but it is still not totally factual, more than likely it was "intergrated" into FreeBSD Current/Head when it was at 12 and merged back to stable/11 and first appeared in a FreeBSD release at 11.1. I still do not see any added value to the history of mandoc by this change, and it just makes FreeBSD have a local diff to the mandoc from witch it was derived. Speaking of which, shouldn't this be on a vendor branch?

I see a fair amount of white space/formatting only changes in this, it MAY make since to break those out as 0 effort review.
I also have concerns over any performance claims, though I see the old code is probably at least sub optimal in that it often checks for UDP, then for TCP when the volume of traffic should almost always be mostly TCP. Also modern compilers should just "handle this" without rewritting code unless something odd is being done. If you really want to micro optimize I suspect rewriting this as "branchless" code that results in cmovl or other instructions might be interesting. I have heard of 40x code speed improvements using such tenchiques, but that is mostly in tight loops.

I was asked to look at this and see if I could help get the Makefile issues moved forward. I have commented inline.

In D25601#566423, @imp wrote:

In D25601#566410, @markj wrote:

In D25601#566409, @imp wrote:

I don't think this is a good idea....

Setting sysctl values multiple times may be unwise.o

I concur on this point. Not all sysctl are idempotent.

This is a good fix, it is wrong for the kernel to try and guess that something is wrong here. "Highly Probably != For Certain" and having this kind of stuff get in the way when your trying to make something work is very annoying for the end user. Now if we could just get the hard coded interface loopback route creating/removal out of the kernel.

I do not believe we normally document this type of information in the HISTORY section of manual pages. The history sections of these man pages are already rich with information, especially when you add the history included in the Authors section.

Is it possible to do cgn without rfc1918? I think the changes should allow for that configuration.

Though much better than the prior iteration I believe it leaves some detail out.