pf: close nc file descriptors in killstates test

[franco_opnsense.org]

Summary

On an atf-sh script capture the nc process lingers and blocks
the capturing process.

[kp]

What do you mean by "On an atf-sh script capture"?

[franco_opnsense.org]

Consider running tests from the src tree using atf-sh (I'm using devel/atf but the base one also works with the full path I think):

sh -c 'echo $(atf-sh /usr/src/tests/sys/netpfil/pf/killstate.sh match)'

This hangs until you kill the stray nc processes. It works fine for the build of tests where nothing is forked into the background which seems to be the general issue. Another way would be to kill the nc processes when the test body ends (not in the cleanup as that would still cause it to hang), but a single line approach is likely easier although if you run the test a number of times these processes just keep building up in the system. I'm happy to improve this in any other way.

BTW, the shell-based tests are great. I want to add tests (mainly for pflog at the moment) without the test build and kyua complications.

[kp]

In D47543#1084854, @franco_opnsense.org wrote:

Consider running tests from the src tree using atf-sh (I'm using devel/atf but the base one also works with the full path I think):

sh -c 'echo $(atf-sh /usr/src/tests/sys/netpfil/pf/killstate.sh match)'

This hangs until you kill the stray nc processes. It works fine for the build of tests where nothing is forked into the background which seems to be the general issue. Another way would be to kill the nc processes when the test body ends (not in the cleanup as that would still cause it to hang), but a single line approach is likely easier although if you run the test a number of times these processes just keep building up in the system. I'm happy to improve this in any other way.

That's kind of expected to not work. We don't run tests that way. We always run them via kyua because it does a lot of setup work for us. This starts the process midway, and stops before it's entirely done.

That is, this skips both the head and cleanup functions. It doesn't verify prerequisites (e.g. are we running as the right user e.g. atf_set require.user root, do we have required programs e.g. atf_set require.progs scapy). Failing to run the cleanup function can leave all sorts of things behind (like jails and interfaces for most tests, processes for some others).

Skipping the installation step may work to some extent for some of the shell script tests, but not for anything that builds a binary. I'm not sure what it'd do for the python-based tests, but either way, this is well outside of how the tests are expected to run.

The command above resulted in two jails and on the host an epair interface with 192.0.2.1 assigned to it being left behind. The in-tree atf-sh even explicitly warns that this isn't how it's expecting to get run:

killstate.sh: WARNING: Running test cases outside of kyua(1) is unsupported
killstate.sh: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)

Kyua takes care of this. In the case of the pf tests it even nests the tests in a vnet jail so that we don't accidentally affect the host, or other running tests.

There's more information on test writing here: https://freebsdfoundation.org/wp-content/uploads/2019/05/The-Automated-Testing-Framework.pdf , if that's useful.

[franco_opnsense.org]

Thanks, just to be clear you imply the change is wrong even though the test still works?

[kp]

In D47543#1084990, @franco_opnsense.org wrote:

Thanks, just to be clear you imply the change is wrong even though the test still works?

More pointless than wrong. You’re trying to fix incorrect use of the tests by changing the test, rather than by using it correctly.