In D38490#914771, @bcr wrote:

Any updates here? I'd be happy to approve the man page change.

In D39760#905888, @vmaffione wrote:

I can commit this if needed

It is worth noting that the man pages in this directory are not installed by make install.

GitHub branch/internal pull request: https://github.com/jlduran/freebsd-src/pull/26

GitHub branch/internal pull request: https://github.com/jlduran/freebsd-src/pull/25

In D39445#898034, @kp wrote:

In D39445#897963, @kp wrote:

In D39445#897960, @markj wrote:

This is neat. It took me some time to understand what's going on, but certainly this framework is better than atf-sh for writing complex or heavily parameterized tests.

I'm currently trying to write a carp test based on this example, and ... well, I can sort of see it making sense when I need scapy to parse packets (which is what I want to do in the test I'm writing), but there's a lot of magic here, and it's taken me quite a bit of time already just to get the setup going (a vnet jail with one epair interface with carp configured on it), and it still doesn't do things that are trivial in atf-sh (How do I skip the test if carp.ko is not loaded?).

Having more or less finished that test (D39454) I think my view remains that python is great for tests that do things like use scapy to parse packets, but I don't think they'd be suitable for tests such as pfsync:bulk (and indeed the majority of pf tests) where 99% of the test is setup.
There's a fair criticism of the atf-sh tests that there's a lot of boilerplate, but I find it makes the test more obvious about what's going on. That is, I can read the test as if it were a very simple shell script. Nothing is hidden, and it's clear what setup is being created.

I believe that in some cases, complex templates can be created, for example:

  outside
     |
 +---+---+
fw2 --- fw2
 +---+---+
     |
   inside

can be a class named SimplePFSync that you can inherit in your tests. You would need to understand what SimplePFSync does and returns first (as a newcomer), also it will likely reside in some common file somewhere else (as I understand you prefer to see the setup explicitly in the test), but it should allow you to focus on what's being tested. It is a matter of personal preference, of course.

If this is is just a PoC for the discussion in D39420, please ignore. Otherwise, I believe this will need an ObsoleteFiles.inc entry?
The only drawback I can find to python tests is that they run a bit slower (for now [1]), allegedly compensated by reduced development time :-))

• Fix indentation

In D39126#896811, @markj wrote:

This is looking ok to me with the nits resolved.

Tangential question: Should we add some atf-sh tests with the test plan, even if simplistic, for this and some of your previous ifconfig(8) improvements? If you think it's worth the trouble, I'll try to submit them.

Cross-builds on GitHub are failing for macOS, as secure_getenv() does not exist. Should I add it to tools/build/cross-build/include/mac/stdlib.h?

Taken from a truss output:

open("/lib/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/lib/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/lib/compat/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/lib/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/lib/compat/pkg/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/lib/perl5/5.30/mach/CORE/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/lib/perl5/5.32/mach/CORE/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/llvm11/lib/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/usr/local/llvm15/lib/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) ERR#2 'No such file or directory'
open("/lib/casper/libcap_dns.so.2",O_RDONLY|O_CLOEXEC|O_VERIFY,066000) = 3 (0x3)

After this change, it'll match on the first attempt.

• Move the tests to a separate commit
• Drop style(9) changes

In D39233#893290, @cy wrote:

In D39233#893286, @jlduran_gmail.com wrote:

In D39233#893278, @cy wrote:

Jumbo commits are discouraged. Can you break this revision into three, please.

One to fix the error.

Another to fix the regression.

A third to fix style(9) + whitespace changes.

Will do. In that case, I have a commit just to fix style for the entire file (will be submitted after 13.2-RELEASE, as requested).

Typically what I do is create a git branch and do all my commits there. Sometimes I will submit the reviews separately or if it loses context I submit one review, stating that the one review will be commmitted using N commits.

Yes, it’s on https://github.com/jlduran/freebsd-src/commits/ping-fix-20b4130. However, I’d like to clean it up a little. Tomorrow will do. Thank you!

In D39233#893278, @cy wrote:

Jumbo commits are discouraged. Can you break this revision into three, please.

One to fix the error.

Another to fix the regression.

A third to fix style(9) + whitespace changes.

This can now be abandoned. Thank you!

Address issues:

• Move the interrupt handler function onsignal() to main.c. When a SIGINT is received, check to avoid PR 4696, although I was not able to reproduce it. I am currently working on adding tests for interrupts and this will become a test. I suspect cap_gethostbyname2 is not bound to the same threading issues as gethostbyname2 (back when the bug was filed)
• Unify F_NUMERIC and F_HOSTNAME constants, given numeric output is the default, use F_HOSTNAME (-n and -H should be mutually exclusive?)
• Reduce diffs between ping.c and ping6.c whenever possible

• Reset seenint and seeninfo counters for SIGINT and SIGINFO respectively, just like ping6.c

• Do not use D38483 as a parent revision

This revision now gets in the middle of a more important one. Let's abandon it... if there is interest, revisit.

Address suggestions:

• Remove SIGINFO guards
• Remove onint()

In D38485#891006, @markj wrote:

In D38485#890556, @jlduran_gmail.com wrote:

In D38485#888290, @jlduran_gmail.com wrote:

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

The code has been rebased on GitHub, and the proposal of this diff unified has been submitted as D39126.
Thank you!

Do I understand correctly that this review is now obsolete in favour of D39126?

• Address comments
• Set D39125 as the parent revision

Sigh, use the right base for the diff.

In D38485#888290, @jlduran_gmail.com wrote:

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

I was planning on proposing —specifically for GitHub pull requests— to use their own Super-Linter, maybe submitting a mandoc linter as well.
It uses clang-format, however checkstyle9.pl is less aggressive than clang-format (and from a brief test, more accurate!). It also has the advantage that it can be used in almost any development setup. Nice!

In D38469#888919, @markj wrote:

In D38469#888306, @jlduran_gmail.com wrote:

Try a different approach.
Given this is the base revision for a number of others, some merge conflicts may arise. At any rate,w2 I have a branch with all the other accepted revisions already rebased just in case.
The code inside pr_retip() is only done here for completeness, as it will be completely removed later on.

Could you please point me to the branch? I can commit them now.

Try a different approach.
Given this is the base revision for a number of others, some merge conflicts may arise. At any rate,w2 I have a branch with all the other accepted revisions already rebased just in case.
The code inside pr_retip() is only done here for completeness, as it will be completely removed later on.

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

Just as a reference, here is OpenBSD's commit:
https://github.com/openbsd/src/commit/0c54afce7b19bf1867acebc6219af5943ce387c3

Amazing!

I like the overall idea, thank you for adding more conf.ds to FreeBSD!

I have also confirmed it is present on 13.2-BETA3.
It is essentially another IPv4 lookup scheme developed by zec@ and luigi@ more info in the 2012 published paper, and the Phabricator review D-29821, designed as an alternative to using ASICs, targeting high-end routers.
I have not been able to test it, maybe olivier@ ?, but I think the idea here is to just mention its existence.
(Intentionally not tagging users/reviewers)

First of all, thank you for taking care of documenting this type of changes!
I believe we almost forgot about DXR, something along the lines that it can be loaded with fib_dxr.ko.
Unfortunately, the best documentation I could find in base is in the code itself (and in the diff before it landed).

In D38771#882639, @imp wrote:

In D38771#882624, @jlduran_gmail.com wrote:

Should this file be created under .github, given it is specifically for GitHub contributions, so it does not "pollute" the top-level directory?

For now, I think I'm going to keep it at the top level to give it the maximum visibility. I'll keep that in mind, though.

Should this file be created under .github, given it is specifically for GitHub contributions, so it does not "pollute" the top-level directory?

Thank you for clarifying!

Nice!
Here are a few basic suggestions.
Thank you!

Now that the fix is in place, just commit the tests.

I have also tested this revision, and it is passing all my tests.
I also agree that this is the simpler approach, since it essentially reverts hlen to being a signed integer again.
If this revision gets accepted, I'll update mine to just submit the tests.

In D38431#881732, @cy wrote:

See https://reviews.freebsd.org/D38744 for a simpler alternative.

Note: another alternate solution for D38470 and D38593.

In D38469#880472, @melifaro wrote:

In D38469#878061, @jlduran_gmail.com wrote:

Address suggestions (not sure if this fits the bill).
EDIT: Let me know if adding const to a bunch of required places is preferred.

Apologies for the delay, 13.2 release process is ongoing, so the team and myself are mostly working on addressing the arising issues. Hope we'll be able to get to this (and other) ping diffs closed to the end of the week.
Thank you for being so patient!

No problem at all. Take all the time needed. I can imagine how things are at the moment. I have also stalled the submission of other, minor fixes planned until the slush curfew is lifted.

In D37722#879900, @tcberner wrote:

In D37722#879899, @jlduran_gmail.com wrote:

In D37722#863538, @dan.mcgregor_usask.ca wrote:

This is good, but it doesn't match what Linux does. Both systemd and dbus generate 32 character UUID strings, while this creates 36 character ones.

I'd suggest we strip the '-' characters from the machine-id file, and possibly think about ensuring that dbus's idea of the machine-id is the same as the one generated here, or since dbus will use /etc/machine-id if /var/lib/dbus/machine-id doesn't exist, maybe not generating the latter at all.

@tcberner regarding this comment, should we change it to /bin/uuidgen -r | tr -d - > $t, or something similar? or is not really necessary to strip the -s?

Moin moin

I probably misread the documentation that said that modern systemd creates valid UUIDs. But that probably did not mean also it uses the 'dashed' format.

mfg Tobias

In D37722#863538, @dan.mcgregor_usask.ca wrote:

This is good, but it doesn't match what Linux does. Both systemd and dbus generate 32 character UUID strings, while this creates 36 character ones.

I'd suggest we strip the '-' characters from the machine-id file, and possibly think about ensuring that dbus's idea of the machine-id is the same as the one generated here, or since dbus will use /etc/machine-id if /var/lib/dbus/machine-id doesn't exist, maybe not generating the latter at all.

Elegant test implementation! I wonder if the Freudians will find it too scatological.

@markj I've created D38593 as an exercise.

Address suggestions (not sure if this fits the bill).
EDIT: Let me know if adding const to a bunch of required places is preferred.

In D38470#877133, @markj wrote:

So isn't there now a mismatch between hlen and cp when handling IPOPT_LSRR and other options? The loop header increments cp once each iteration but doesn't decrement hlen to match.

I'll test locally with a packet containing LSRR options with just a couple of routers + NOP and see the remaining hlen value.

Address suggestions:

• Change the warning message

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

• Avoid a potential square root of a negative number calculation (based on an alternative to D38480, this change should not part of this revision)

I have a branch where it unifies the code into a single summary() function, however it changes a lot more of the code that I would like/have tests for at the moment. Besides, the code ends up being almost identical to ping6.c's. Let's leave the unification for later in the ping/ping6 series.
A few cosmetic things discovered from this exercise are:

• ping's SIGINFO prints out to stderr (ping6 to stdout)
• ping/ping6 should just become ping when displayed to the user
• ping uses the expression "somebody's printing up packets!", I prefer ping6's "somebody's duplicating packets!" when a duplicate packet is received
• ping calls the population standard deviation "stddev", while ping6 calls it "std-dev". I'm OK either way, but it should be the same for both

• Remove pr_retip()

Address comments:

• Add single NOP case
• Remove unneeded int cast
• There is no need to decrease hlen, as cp is advancing to the next octet, (single NOP case added as proof). However, let's be pedantic (and true to the original implementation), and decrease it.