Test scenario for sysctl kern.maxfiles

[Peter Holm]

On Thu, Mar 06, 2014 at 09:52:58AM -0500, Julio Merino wrote:
> 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.
> 

Done.

> * 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);
> }
> 

Rewritten.

> * 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").
> 

I mistakenly relied on numerous NetBSD examples that uses /etc/passwd.

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

It doesn't.

> * What's the rationale behind the TEST macro?
> 

Removed test code.

> 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.
> 

Skipped using atexit(3).

> >> 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.

Updated dif: http://people.freebsd.org/~pho/kern_descrip_test-v4.diff

-- 
Peter