This looks appropriate to me.

You could add the hier(7) man page update to this review since this change is what effects that man page too.

Thank you Mike

With the constraints that are here I am in favor of allowing these.

Adding some more eyes to this, IIRC this is ofed code and actually maintained and imported from some place else. Though I agree with Glebius that this is the right way to fix it, it may need or be desired to do this upstream. Hselesky is the infiniband expert.

In D35741#811764, @melifaro wrote:

Q: that’s the current thoughts on making the behavior default one?

Agree that this should be committed at the same time as the functional code changes.

I like the numberical ip_allow_netX version as well. I also agree that changing rc.firewall at this time is premature.

Adding a reference to https://reviews.freebsd.org/D19316 for some historical discussion.

Pi, thank you for spotting this, and bz thanks for the quick fix. This explains some issues I had when I tried to migrate to 13 on a router, but had no time to investigate what was going wrong.

I strongly disagree with adding "site and user specific" tweaks to the dot.cshrc files. Further I note that a huge down side of systems that do this xterm tittle setting create very miss leading situations when the terminal disconnects for any reason (ie your xterm is now local, not remote). Some systems do infact try to clean that up in the logout scripts, but that doesnt work if your disconnected for any other reason. There is already a very clear default prompt setting that shows you the hostname of the system you are logged into.

In D28200#722356, @kp wrote:

So, I've been looking at committing this, and I'm still convinced that the previous behaviour is indeed incorrect, but I'm no longer sure this is the correct fix.
We should probably call in_broadcast() to run this check, rather than duplicate the logic here.

In D31861#720111, @karels wrote:

Add some reviewers, and pull in most of the subscribers fro D19316

Looks ok to me, just a comment on keeping code speed the same in almost all cases.

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.