From owner-freebsd-testing@FreeBSD.ORG Thu Mar 6 17:45:54 2014 Return-Path: Delivered-To: freebsd-testing@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AAD2A8BA; Thu, 6 Mar 2014 17:45:54 +0000 (UTC) Received: from mail-we0-x229.google.com (mail-we0-x229.google.com [IPv6:2a00:1450:400c:c03::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id EF35E28C; Thu, 6 Mar 2014 17:45:53 +0000 (UTC) Received: by mail-we0-f169.google.com with SMTP id w62so3578903wes.28 for ; Thu, 06 Mar 2014 09:45:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=A1+WRqxirxzaE+XY81JKyDaBIzauEDukOrY0zpwj6Ek=; b=QdGoTXH4ipZ0QGjp2KxccE4jKbBNOLfR9VpHCNiE7CivEYu6lVmte5r2H1b/ucGxiL BIBsvZAIctzggKFPSdzwgG8GMwJPuKsZmd94tK0yRQ+IoCcRo6iPQho7JRrgPdbBVocK mbEwHkqUBRtE+c0ex6WWK8RmiNLbWgrJ9mDOJWs45YYlF04ieyFfLR7cdyJn7hbEowxA 2sNJh0PNJYcc62PIG1hoCpibinb0WPdik3hvivl0VyI5W5qnVecZZEg/k58yYyesZ6iW pmXPADy5qj+zt31PM8jLdwcDdYjlssbrSxDzGwKzlErcoVPRWte88dhANa7NDEt3pdRU b8rg== MIME-Version: 1.0 X-Received: by 10.194.157.41 with SMTP id wj9mr12483931wjb.34.1394127952458; Thu, 06 Mar 2014 09:45:52 -0800 (PST) Sender: asomers@gmail.com Received: by 10.194.168.197 with HTTP; Thu, 6 Mar 2014 09:45:52 -0800 (PST) In-Reply-To: References: <20140305085806.GA70478@x2.osted.lan> <20140306112322.GA10664@x2.osted.lan> Date: Thu, 6 Mar 2014 10:45:52 -0700 X-Google-Sender-Auth: 7oNcGUHnOEl8uhxd2xZkxu1djTM Message-ID: Subject: Re: Test scenario for sysctl kern.maxfiles From: Alan Somers To: Julio Merino Content-Type: text/plain; charset=ISO-8859-1 Cc: "freebsd-testing@freebsd.org" , Peter Holm X-BeenThere: freebsd-testing@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Testing on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Mar 2014 17:45:54 -0000 On Thu, Mar 6, 2014 at 7:52 AM, Julio Merino wrote: > On Thu, Mar 6, 2014 at 6:23 AM, Peter Holm 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 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. I suggest the more specific name "allow_sysctl_side_effects". As I continue to add tests, config variables like this will become more important. In particular, I have some tests that create and destroy zpools, some that can cause panics, some that load modules, etc. Before I can upstream all of them, we'll need to agree on a consistent set of config variables that control test case execution. I haven't put much thought into it yet. How does NetBSD handle tests that have potentially harmful side effects? -Alan