Date: Fri, 14 Mar 2014 10:30:14 -0600 From: Alan Somers <asomers@freebsd.org> To: Peter Holm <peter@holm.cc> Cc: "freebsd-testing@freebsd.org" <freebsd-testing@freebsd.org> Subject: Re: Test scenario for sysctl kern.maxfiles Message-ID: <CAOtMX2hrVgZpAUw=LdGDGAiCd7fgSTMKSnQ=M03j0=OCwFQ%2B-g@mail.gmail.com> In-Reply-To: <20140309114012.GA54149@x2.osted.lan> References: <20140305085806.GA70478@x2.osted.lan> <CAOtMX2hUJ2Hc62bG1jitbQbiHtb8b8Jm8iWaP4VaJPuADXR=Kw@mail.gmail.com> <20140306112322.GA10664@x2.osted.lan> <CAFY7cWD1Zm8MVR70e83ZVBu1JMYiZ804FZnrMkmCuj-x=HUUvw@mail.gmail.com> <20140309114012.GA54149@x2.osted.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 9, 2014 at 5:40 AM, Peter Holm <peter@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@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2hrVgZpAUw=LdGDGAiCd7fgSTMKSnQ=M03j0=OCwFQ%2B-g>