• Add missing file
• Match ping's stdout in the injection tests.
• Python style
• More specific output checking for inject_pip
• fixup:

In D37210#853886, @markj wrote:

One problem I noticed is that the tests can spuriously fail with some error messages from scapy:

markj@nuc> sudo kyua debug ping_test:inject_opts
Executing command [ python3 /usr/tests/sbin/ping/injection.py opts ]
Fail: stderr not empty
--- /dev/null   2022-11-30 14:43:29.782039000 +0000
+++ /tmp/kyua.FOws6H/2/work/check.VgUqKM/stderr 2022-11-30 14:43:27.574257000 +0000
@@ -0,0 +1,5 @@
+
+(injection.py:37468): dbind-WARNING **: 14:43:27.258: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory
+WARNING: No IPv4 address found on bridge0 !
+WARNING: No IPv4 address found on epair0a !
+WARNING: more No IPv4 address found on epair0b !
Files left in work directory after failure: .cache, .config, tun.txt
ping_test:inject_opts  ->  failed: atf-check failed; see the output of the test for details

@thj the fix is in main now. Could you please review again?

In D37519#853759, @rmacklem wrote:

• The sysctl falls out of the krpc code. I don't know why dfr@ provided it. Maybe for old NFS code or some other protocol that uses the krpc. --> If you set it after starting the nfsd, it would override the command line setting, I think. Basically the sysctls are useless for the new nfsd code, but to get rid of them, the krpc would need to be changed to never generate them.

I don't know much about the NFS server code, but I've got a few questions:

@jlduran_gmail.com don't get too invested; @thj already has a fix prepared.

Well, it isn't ideal. But it's certainly much better than it used to be, and the ZFS tests are much too big to shoot for "ideal".

• Perform the check earlier

In D37313#847880, @markj wrote:

Seems ok, but why not handle it in g_label_taste(), or even at some higher level in GEOM (maybe g_disk_create())? I can't think of any classes that will do something sane with a provider with sectorsize 0...

In D37309#847728, @mav wrote:

Not that I have specific objections against this, but I think it would be much better to implement a generic way to print all consumers connected to specified GEOM provider. This code does not look anyhow specific to gmultipath.

BTW, these tests are for the code change in D37195

In D37210#845107, @jlduran_gmail.com wrote:

In D37210#845076, @asomers wrote:

In D37210#845043, @jlduran_gmail.com wrote:

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

Yes I did. But I can't figure out how to make the ping.c changes go away from phabricator. Can we just pretend, for the sake of review, that they aren't there?

😂 that changes everything!
But in all seriousness, we may have hit a bug? in the opts case.

In D37210#845043, @jlduran_gmail.com wrote:

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

• More specific output checking for inject_pip

• Match ping's stdout in the injection tests.
• Python style

• Add missing file

Ok, this LGTM if you fix the comment typo. And I think I like the reasoning here better than in the other PR.

• Also delete newport if old port is a dummy

• Respond to @imp's comments

• Fix an assertion spotted by Corvin Köhne

Sadly, I think there's no way to set that property for an individual test case. I think it's ok to set it for the entire program, since these tests run pretty fast anyway.

Approved. And even though I probably won't MFC the original, please be sure to add an "MFC With" to your commit message, just in case.

In D36743#834236, @dougm wrote:

In D36743#834231, @asomers wrote:

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

After your change, when the first oid_number tried is found to be in use, you usually increment the oid_number and use it, assuming that it is not also in use. Before your change, when the first oid-number tried is found to be in use, the code usually incremented the old number and iterated, to check whether that number was in use, and kept doing so until it found a number not in use. So, you have introduced a bug, which I am trying to address, by making sure that the oid_number is not in use.

If this makes external kernel modules easier, than go for it.

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

• Rename a function

@mjg are you ok with the current version of this review, using RB trees? I don't want to make the other changes because:

• Using sorted arrays with dynamic reallocation would be a lot more work than the RB trees.
• Adding a second tree to speed up name2oid would be possible, but it really doesn't depend on anything else in this change. It could be done independently.
• AFAIK name2oid's current performance is good enough for current users. It doesn't blow up quadratically like sysctl_sysctl_next_action does.

• Add comments about vn_rlimit_fsizex_res

In D36706#833361, @kib wrote:

Use of vn_rlimit_fsizex() requires finishing VOP with vn_rlimit_fsizex_res().

Thanks for fixing this. As for what the correct behavior should be when the file doesn't yet exceed the limit but a write would cross that threshold, I'm agnostic. I think the old behavior of aborting the write entirely was fine. But the new behavior is fine too. I can fix truncate with fusefs after you merge this change.

• Switch to rb trees instead of splay trees
• Bump __FreeBSD_version due to the KPI change

This is outside of my areas of expertise. I'm afraid I won't be able to help with the review.

I don't think arrays are a practical solution, because the key range is too large. oid_number can be as high as INT_MAX, so the arrays would have to be dynamically allocated, and they may have to resize on insertion.

It can't, I noted this is already clamped:

/*
 * The starting number for dynamically-assigned entries.  WARNING!
 * ALL static sysctl entries should have numbers LESS than this!
 */
#define CTL_AUTO_START  0x100

So worst case for static entries is not particularly bad. Resizing on insertion may still be sensible to do, but it wont affect lookups.

In D36500#829028, @asiciliano wrote:

Hi, thank you for the review. I like the lock update.

Could it impact <sysutils/sysctlinfo-kmod>? After this kernel update, is it still possible to compile and to use the sysctlinfo-kmod port and its "clients"? (deskutils/sysctlview, sysutils/nsysctl, audio/mixertui, sysutils/sysctlbyname-improved-kmod,...)

(I ask because unfortunately now I'm in a busy period and cannot test and give this review the attention it deserves.)