Test scenario for sysctl kern.maxfiles

[Alan Somers]

On Sun, Mar 9, 2014 at 5:40 AM, Peter Holm <peter at holm.cc> wrote:
> 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

Be sure to add a 'atf_tc_set_md_var(tc, "require.config",
"allow_sysctl_side_effects");' line to kern_maxfiles__increase's
header.

Using symlinks as a key-value store?  Tricksy!  I like it.  And as
tricky as this is, it probably deserves some explanatory comments.

Restoring kern.maxfiles in the body of the test as well as the cleanup
is redundant.  I think it's ok to only do it in the cleanup.

The "r != -1" check in openfiles2 at line 97 is tautological.  There
is no way for that test to fail, thanks to the preceeding ATF_REQUIRE.

The RENDEZVOUS thing is a little hacky.  You could replace it with
named semaphores and that would eliminate 2 of the 3 the sleeps.  I
suppose you could also eliminate the 3rd sleep by replacing the
SIGUSR1 handler and done variable with a different named semaphore.
Alternatively, you could have openfiles2 sleep forever, and have
openfiles send SIGTERM to all of its children when it's done.  But
RENDEZVOUS works ok, I guess.

Check your whitespace.  You've got leading spaces in
kern_maxfiles__increase's head and in the ATF_TPADD_TCS() body.  And
trailing whitespace in "#define PARALLEL 4 ".

Other style(9) violations include a magic number at line 162, a
too-long line at line 180, and a return statement that doesn't enclose
its argument in parens at line 193.

Looks good otherwise,

-Alan