Looks better, thanks!

Sorry, I was thinking that the original text was gone; I see that it is here. It is still misleading, or at least poorly worded, to say that these commands "appeared" in FreeBSD 1.0. Essentially every command in 4.4BSD-Lite was also in FreeBSD 1.0, but we don't bother saying so; it adds no useful information. The reference to 1.0 should be deleted.

Looks good to me, I'm ready to approve. I'd also like to see a reviewer who knows the brgphy code.

Thanks, Conrad. I'm checking it out.

In fact, tcp_connect was able to use a port that had a wildcard bind. I can't think of a real-world problem this would cause, but it was easy to fix.

Do not allow a port with a wildcard bind to be used for connect.

In D24781#547768, @tuexen wrote:

Hi Mike,
let me ask you two questions:

Add sysctl to control new feature

In D24781#546975, @rscheff wrote:

What was the special handling of a inp_port == 0 doing, or why did you remove this in this patch?

The enterprise-grade Sidewinder firewall has had this feature for years. I don't know what other implementations work this way, but it seems like the obvious way to do this "right". A NAT box cannot serve multiple clients without handling port overlap, nor connections to multiple services (ports) on a server.

In D24436#539201, @manu wrote:

In D24436#538986, @karels wrote:

Address more review comments; fix bug with bpf and txcsum.

Notes:

I started a version with full detach; it is not working correctly yet.

What is the problem exactly ?

Address more review comments; fix bug with bpf and txcsum.

About doing a full detach: then code is needed to undo the init routine, and there are far more failure modes. I'll look at it though.

Is there an existing example of FDT / ACPI split to use as a model?

Updates based on review comments, including teardown after failed attach.

@greg: I'm not quite sure how the FDT/ACPI split should be done; I think that will have to wait for another time.

Arm seems most likely to want pre-loading, but I don't think it hurts anything. I think it could be enabled for everything. How about mips, powerpc?

Looks good, I didn't realize that the kernel already had validation code. Might as well use it!

Conrad: this fixes a current problem with live systems, debugging a multicast problem. I am not aware of any requests to get multicast stats from a kernel core.

Fix parens

Remove incomplete ifdefs.

Upload correct version with nested loops.

Change to use nested loops for procs and threads; deal with zombproc

I took yet another tack last night, and it is better than previous attempts. It seems to be working correctly, but needs more testing. The changes are much bigger, mostly due to code motions. (svn diff is now 286 lines, about twice as big.) I'll test more, then decide whether to proceed with it.

Thanks, John.

Conrad: it has now been more than a week since Marcel asked a question. If you can't or won't explain why you oppose this change, I think you need to drop your block on the review.

change macro to take parameter

Sorry that this was unclear. There is a limitation in phabricator to the kinds of negative feedback metadata one can provide. It's "request changes" or nothing.

@cem: phabricator says "cem requested changes to this revision.", but I see no changes requested.

minor corrections

@cem: Not if it is an improvement on another change, and makes better use of the existing space.

Although I have a couple of inline comments, I wouldn't object to committing this as-is. Touching up would be fine too.

This seems good to me. If this is not to be MFC'd, compatibility wouldn't matter (in which case I wouldn't have reduced the number of spares). It's nice to have things in order, but maybe this isn't the time to break compatibility given the small scope.

I agree with John that we should do everything we can to eliminate class A/B/C rather than just shuffle it around. Removing those macros will cause some turmoil; e.g. libc's inet_netof and inet_lnaof assume classes, and don't know about local netmasks. They should probably go away. I don't know how painful that will be. But we are way overdue to get rid of this stuff, and it appears that Linux (Ubuntu) doesn't have them. But the old A/B/C are a somewhat different problem than making Class E usable.

Hi, John! Very, very long time no (anything). I am very happy to see tests in this area. I certainly expect this to work with minimal changes (i.e. this one). Re: Classs A/B/C: I'd be quite happy to see those definitions go away. Re: "loopback net": I'm aware of a small number of things using that other than 127.0.0.1 (including one of my company's legacy products), but they are minor. I still remember driving a Sun workstation crazy by pinging 127.0.0.2 (there was a network route, but obviously only one address recognized.).

Sorry, that reference should have been RFC 1112, not RFC 112.

It looks like I am the guilty party in calling Class E IN_EXPERIMENTAL, which went in when "Class D" became multicast. I suspect it was actually called that in some document, although it doesn't really matter. https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml doesn't call it Class E, but refers to RFC112, which does. In any case, assuming that https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml is still current, the treatment is the same.

I don't understand the purpose of this review. As it says, these changes should not be committed. I'm not actually sure of the official status of Class E, although I suspect it is still experimental/not for production use.

Generally looks good, one small comment inline.

I agree that arprequest_int is rather opaque. At first I thought that is was just the version that returns int, which is not useful. arprequest_internal would be better. Otherwise, I have no objection; returning any errors seems like a good thing. And yes, I did not see any of the earlier (presumed) emails for this review.

LGTM also

It would be good to see if the crashes stop before committing this, but it looks right to me.

I believe I understand the issue with route caching. in6_selectroute_fib checks for loopback,and substitutes the interface with the local address. ip6_output doesn't need to know that interface, it should just skip the scope check in that case. I'll email a possible kernel patch to Andrey and Harti.

You are right, inp_route6 would be better. I'm sure I copied and pasted, then didn't make as many changes as I should have. I will happily approve a review to make that change if you test that it compiles :-).

Looks good to me. I could go either way on the blank line.