Test scenario for sysctl kern.maxfiles

[Julio Merino]

On Thu, Mar 6, 2014 at 6:23 AM, Peter Holm <peter at holm.cc> wrote:
> On Wed, Mar 05, 2014 at 10:08:49AM -0700, Alan Somers wrote:
>> On Wed, Mar 5, 2014 at 1:58 AM, Peter Holm <peter at holm.cc> wrote:
>> > Here's an attempt to verify that increasing kern.maxfiles works as
>> > expected.
>> >
>> > http://people.freebsd.org/~pho/kern_descrip_test-v3.diff
>> > --
>> > Peter

A couple of general comments:

* In openfiles2(), it seems to me that 'i' should be size_t. That cast
in the for loop looks like a hack.

* Style detail: I'd change the code to say something like the
following, which clearly separates the action from the handling of the
result.

r = open("/etc/passwd", O_RDONLY);
if (ignore) {
  if (r == -1)
    break;
} else {
  ATF_REQUIRE(r != -1);
}

* Why does this test rely on /etc/passwd?  Just create a file in the
current directory and open it.  See the atf_utils_* in the
atf-c-api(3) manpage for various helper functions to deal with files.
If you want to depend on system files, the test should be declaring
that explicitly with atf_set_md_var("require.files", "/etc/passwd").

* Does the period in the test case name kern.maxfiles__increase
actually work?  I'm surprised.

* What's the rationale behind the TEST macro?

More below.

>> 1) done should be of type "static volatile sig_atomic_t", not int,
>> because it's set by signal handlers.
>>
>
> Yes, that is nicer (I learned something new today :-). But the use
> here works because there is a call to usleep(3) after each test,
> forcing the compiler to reload the "done" variable.
>
>> 2) using atexit() to register a cleanup routing is a hack.  No doubt
>
> Why do you say that using atexit(3) is a hack?

Because that's not how cleanup should be implemented in atf tests: you
should be using the cleanup routines.  The reason is that atexit(3) is
not guaranteed to work: if your test crashes or is killed by Kyua
because it overruns its deadline, your cleanup code won't work but a
cleanup routine will.

>> you already noticed that it's difficult to use Kyua's builtin cleanup
>> capabilities because of the need to pass the value of oldmaxfiles.  I
>> too have experienced that frustration.  Is there any way to pass
>> values from the body of a testcase to its cleanup?  Using
>> atf_tc_set_md_var() would be one way, but the man page suggests that
>> that function cannot be called from the body.  Julio, is there a
>> better way to do this?

The "problem" is that the body and the cleanup run in different
processes really, so there is no way you can pass state other than
writing to a local file.

This change:

http://cvsweb.netbsd.org/bsdweb.cgi/src/tests/kernel/t_sysv.c.diff?r1=1.3&r2=1.4&only_with_tag=MAIN

shows a way to do it.

Because these tests are dealing with scary global system state (which
is a bad thing), I think that the hassle of having to do this cleanup
dance is "kinda justified": not that many tests should need it and,
for those that do, it becomes clear that maybe the test should be done
in a different way.

We could do better, yes; I don't disagree with that. Suggestions?

> Now this raises an interesting question for me: Which environment do
> you guys expect ATF to run in? If it is hosts like freefall, any
> resource hogging tests are out of the question I would think.

That's the interesting question.  This test will probably be flaky
and/or cause side-effects in the system while it runs because it
relies on state that is out of control of the test.

For the setup at kyua1.nyi.freebsd.org, this is not a problem because
the tests run in a VM.  But we want to encourage users to run the
tests themselves and this kind of tests may be problematic.

Here goes a possibility: add a configuration variable
"allow_kernel_side_effects" or similar to kyua.conf
(test_suites.FreeBSD.allow_kernel_side_effects = true). Then make the
test case require the variable with atf_set_md_var("require.config",
"allow_kernel_side_effects") so that this test only runs when the user
has told Kyua to allow these tests.  We can later change the
kyua1.nyi.freebsd.org setup to define this variable but we leave it
off by default for end users.