I am happy with the other fixes, not sure if there is a need to fix or error check memory size bit overflows

In D24107#532656, @grehan wrote:

In D24107#532572, @bcran wrote:

• Rework type17 generation and error out on overflow.

There is no need to error out on overflow here. The type17 isn't used by most o/s's for determining how much memory is available: there is e820, the EFI memory map, etc etc. Perhaps print an error in bhyve, buit there doesn't seem any point in erroring out.

In D24107#532391, @jhb wrote:

I think we should go with 2.7 btw. Systems shouldn't read 2.7 fields if we advertise 2.6 since those fields "shouldn't exist". OTOH, the spec does say systems are permitted to use the length of individual structures to determine which optional fields exist.

Thanks, and this should happen in the port too as you pointed out it was also doing the wrong thing.

This code should work without a need to split memory up into 32GB Dimms

Argg.. you did at the new field. Hum, I am reading 3.3.0 type 17 now.

In looking at the SMBios 2.7 change list in DSP01342.7.1.pdf (final version of 2.7) on page 134 There is a memory device (type 17) update that you also need to implement to allow DIMM's >32GB. In my work on VM_MAXCPU's I found it would actually be desirable at some point to get to 3.X version of some form, can not recall exactly what the minimum was right now, but reading a newer change log I see some interesting things like in 3.0.0 add GUID value for discovering SMBIOS tables in UEFI, which iirc would help our UEFI situation, add new entry for extended BIOS ROM size, oh here it is 3.0.0 Type 4, extned core, core eabled and thread count ranges. Looking at the change list in the 3.3.0 spec between 2.6 and 3.0.0 is not a huge task as much of it does not apply to bhyve.

I still object to range testing MTU here, but less so now that the values are closer to implementation limits. Base reason for objection is your range testing data at the producer, and it is the consumer that this should be, and I believe already is, done at.

Let me try again to make my point of why it is wrong for this code to have ANY checks on the value of MTU other than to fit in the datatype.
The in kernel vtnet device ALREADY contains code that enforces the implementation specific limits of this value, and is a single place to make adjustments to those values.
See src/sys/dev/vertio/network/if_vtnet.c at about line 1044:

if (new_mtu < ETHERNMIN || new_mtu > VTNET_MAX_MTU)
    return (EINVAL);

This should be the sole place that limits on this value are enforced, and doing so any other place is just duplicate code that raises the work to maintain the code.

The kernel device, vtnet, already does proper bounds check on mtu, returns an error if it is <ETHERMIN || >VTNET_MAX_MTU, adding range check code here just increases code maintanance, and if it is done it MUST much the kernel implementation (Which oddly has 65536 as VTNET_MAX_MTU, so even the 65535 above is a false collapse of capabilities.

In D23971#528161, @lutz_donnerhacke.de wrote:

In D23971#528126, @rgrimes wrote:

I have no idea why someone thinks a network device should have a minimum MTU of 1280, that is simply the IPv6 value, ethernet is very happy to transfer 64 byte packets. There should be some implementation detail of the in kernel vt driver that can at least go that small, and perhaps smaller as you do not have the collision detection minimum wire time that ethernet has(had).

So you suggest to violate RFC 6540 and disallow IPv6? ATM has a very small frame size, but the fragmentation FRF.8 is part of the standard. For IPv4 the minimum MTU is 512, for IPv6 it is 1280, even is the medium at layer 2 is able to transport smaller frames (which is perfectly allowed by a minimal maximum). 802.3 is even able to transport smaller packets than the minimum frame size ...
What do I miss?

I have no idea why someone thinks a network device should have a minimum MTU of 1280, that is simply the IPv6 value, ethernet is very happy to transfer 64 byte packets. There should be some implementation detail of the in kernel vt driver that can at least go that small, and perhaps smaller as you do not have the collision detection minimum wire time that ethernet has(had).

Why are we dropping these (other than cx?)

In D23690#520894, @kevans wrote:

Breaking this comment up a bit to respond to it:

Perhaps we should have this discussion on arch? I find phabricator a horrible place to try and do this level of a discussion.

Why not go back exactly how it was before? Why change additional things like ttys (note that the use of arch subdir symatics is matching to other parts of the tree. How did you arrive at this patch, what was the mechanics you went through? Why not put rc.d back? What about rather than using .PATH have the reachover to call into etc/Makefile to install the sub component? I do need to look at this much closer,. Thats all for now.

In D23687#520879, @tuexen wrote:

OK, I double checked the code. I think the way to go is taking your change to tcp_hc_get() and not do the initialisations in cc_conn_init() and tcp_mss_update().

...
I concur with Michael's conclusion, the struct initializers are unneeded and wasteful of cycles, and accept the differential based on them being removed before commiot.

In D23329#518006, @olivier wrote:

...

PR created for other routing daemon I do not maintain:

• net/quagga (PR 244027)
• net/openbgpd (PR 244028)
• net/openbgpd6 (PR 244029)

I have added myself to these 3 bugs and if no one else has committed the base change once those are closed I'll do it. Thanks for getting all the others Oliver

A document describing the language and process of depreicating would be good to have.

when dump/restore are invoked with the leading r they change behavior and speak "rmt". Yes, there is probably much documentaiton missing.

You need r{dump,restore} to talk rmt protocol to a remote tape drive, you can not just pipe over ssh to a remote tape drive.

*sigh* usr.sbin/rmt/Makefile needs updated to do the proper packaging of rmt and your done?

How are the symlinks /etc/termcap, /etc/unbound, and /etc/aliases handled by pkgbase? You certainly can't go removing those.

@emaste Yes I object especially if that is the basis for this change. Why are we messing with other software to fix a defeciency in pkg base? symlinks in /etc/ should be supported. My fear is that though you say /etc/rmt "is to support ancient tar commands" the reality is everyone using rmt probably refers to it via /etc/rmt and this is going to break if you remove it. Using the word ancient is poor taste, all things "Unix" are ancient :-) I am ancient also by your standards :-(.

Shouldnt the driver code include a gone_in?

olivier siad: Yes I can fix all routing ports to be sure they all PROVIDE the dynamicrouting keyword.

melifaro said: No one uses routed. In the current form this change is useless.
I disagree, but the use is sparse, HOWEVER, routed_program, etc all CAN and are overriden. I start what ever routing layer I am using that way, which include but are not limited to: gated, bird, openbgp. And I use routed_enable to turn them on and off.

Please create a review to first add deprecation notices to the man pages and a gone_in to the driver, with a RELNOTES: y, and a MFC 11+12. Then this can be committed with a MFC: never right behind it.

For icmp6 since that is left out of any of this I think starting a new review to add icmpv6 handling is probably in order. I do see a sysctl, net.inet5.icmp.redirectaccept=1, but do not see a log one.

The output in Phabricator GUI looks all messed up, the raw diff looks fine though.

Other than perhaps a uniformity issue, that you can ignore if you wish, this looks fine to me.

rmt, rdump, rrestore all support something other than rsh(1) as a transport mechanism, ie ssh. Also if rmt goes you need to kill rdump, rrestore and fix the man pages. MFC'able depreication notices should go in first too.

The TODO: must be done before rmt is removed. And I think further discussion on -arch about this should occur, rmt is kinda a common thing for people that run tapes, and tapes are still alive.

Further note, you should make the utility itself spit out a notice when executed.

The timing is flexible I think, but my recollection of the current depricaiton policy is since no notice went out in 12.0 the first notice of deprecation should be in the 13 branch and the code is removed in 14. But as I said history has shown us to be pretty flexible especially if we get notice MFC'ed and it goes out in 12.2. So up to you which path you choose.

Relnotes: Y and RELNOTES entry possible too, and with a slight language change MFC to 11 and 12.

I concur that one large commit is best, BUTT, we should also see how it merges before doing that, as I suspect a large amount of this well fail to merge, and that creates a merge nightmare. As one who shoots down white space fixes within your other reviews let me try to expand on reasoning. It is not that it is bad or wrong to fix white space, it is that it adds noise to a review and each white space change has to be looked at as if it was part of the functional change and that creates reviewer work load and sometimes also causes mistakes to slip by.

Looks good visually, need some others confirmed to have tested.

How did you run into this one?

Good catch, thank you Yamagi and Marcelo

I would of like to seen a more formal Copyright update plan in place before large tree sweeps got started by people doing the All Rights Reserved cleaning, but too late now.

I might of used tunif_freed inplace of tunifdtor, but thats just me wanting grep to find these :-)

I am ok to move forward with this and leave rethinking of factoring out the newreno latch to a cc function until later, how ever I would like to not use default: for the only possible state of the ECT bits being 00, OR explicity comment that the only case that can trigger the default is ECN=00.

I would encourage continued work on this and the switch code too.

This is a visual code read only review, I have not compiled or tested the code.

Nice find.

Can you first revert the wrong change, which cleans this up by not doing the deletes of \n\r that was added by that diff.

From the summary:

The main question remaining is then, if the DCTCP receiver
state machine asks for an explicit immediate ACK to be set,
in order to convey any change in the "CE" state of the incoming
packets in a timely fashion.

Thanks

Thanks for quickly addressing this. Just a code comment I noticed, and am curious if I missed something that would not allow the removal of a duplicate conditional

Offering a solution. Have the kernel track the max size of the routing table and return that value on sysctl(null) rather than the current size. This should only fail during initial table growth on a large table router, and should not require a retry except on rare occasion.

In D15488#495094, @melifaro wrote:

In D15488#493981, @rgrimes wrote:

I have come to the state that if your running into this problem you probably should not be using netstat -rn to look at route tables but rather use the proper tool that talks with your routing daemon, adding a "bang on the kernel" repeat inside of netstat is probably a poor solution.

Well, I'm not sure.
First of all, there _are_ cases where you have to fetch the data, regardless of its size - troubleshooting can be one of those.

And when trouble shooting netstat -rn general works ok, it gets the error only while the route table is growing rapidly and is very easy for the user to retry the command, SHOULD they be using it.

I have come to the state that if your running into this problem you probably should not be using netstat -rn to look at route tables but rather use the proper tool that talks with your routing daemon, adding a "bang on the kernel" repeat inside of netstat is probably a poor solution.

In D22429#490320, @rscheff_gmx.at wrote:

In D22429#490316, @rgrimes wrote:

Something really strange here with this diff

The raw diff should be ok though...

Something really strange here with this diff

I usually do the comparison separate from formula when they already exist, but either is ok with me.

This is defanitly needed. Only two comment, you elude to having a version, it would indeed be nice to have a thing like the gonein(13) functionality, how to do that I do not know, and I would commit this in 2 parts, the new functionality, and then its first use.

The additional note on style(9) on variable order declaration is informative only, you can choose to fix at your option.

My position is that "intent" in law carries a greater weight than "implementation". I agree that the intent of UCB was to embed the Copyright into the binary, and the fact that has been defeated by the choice of tools is on us to correct. Though Berne makes the binary copyright by default, it was still the intent of the original author to have these copyright strings appear. It is also very much understood in copyright law that "removing of any copyright" is an actionable item. Now, the likely hood that UCB would take action is moot. We could contact the UCB legal office and ask them for advice? Even a duplicate copyright can raise that question, I have seen no place an exception based on the fact that the same copyright appeared more than once.

Thanks for remebering to clean this up.

In D20762#449673, @markj wrote:

Reformat to 80 columns.

Finally review please

Fix 7F to F7 in 4 places

I was kinda waiting for jhb@ on this, as it is his patch, but given the urgency I am commandering this, and I'll push a new patch up for review. I would like to ask markj and pmooney to final review that and accept it before I commit to ^/head and push for an immediate MFC, and merge to releng. I'll also need to merge r347065 which is 6 weeks past MFC date.

Ping, since 11.3RC3 is back on the table I would like to see this pushed through along with r347065 which should of already been pushed through as it is 3 weeks past original MFC.

In D19495#449173, @darius.mihaim_gmail.com wrote:

I have applied the feedback @jhb gave, except for the #ifdef guards. Will work on it after rebasing with a newer master.

I helped Jason Tubnor test this patch on 11.3RC2, we had to first merge r347065 to get this patch to apply, and then had to make the 0x7F to 0xF7 Markj points out to get it to work. OpenBSD now runs as a guest in 11.3RC2

I am working with the original poster to get him a binary so he can test this fix.

In D12419#447945, @scottl wrote:

Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.

This appears to be hung up on Capsicum additions

In agreement on it probably needs cleaned up, but first finding out why it was done this way as a safe position to take.