Re: svn commit: r353305 - head/tests/sys/kern
On Tue, Oct 08, 2019 at 11:22:48PM -0700, Enji Cooper (yaneurabeya) wrote:
>
> > On Oct 8, 2019, at 08:04, Konstantin Belousov wrote:
> >
> > On Tue, Oct 08, 2019 at 01:43:05PM +, Eric van Gyzen wrote:
> >> Author: vangyzen
> >> Date: Tue Oct 8 13:43:05 2019
> >> New Revision: 353305
> >> URL: https://svnweb.freebsd.org/changeset/base/353305
> >>
> >> Log:
> >> Fix problems in the kern_maxfiles__increase test
> >>
> >> ATF functions such as ATF_REQUIRE do not work correctly in child
> >> processes.
> >> Use plain C functions to report errors instead.
> > There are much more tests that fork and use ATF_ in children.
> > Look e.g. at most ptrace(2) tests.
>
> I beg to disagree:
>
> 86 /*
> 87 * A variant of ATF_REQUIRE that is suitable for use in child
> 88 * processes. This only works if the parent process is tripped up by
> 89 * the early exit and fails some requirement itself.
> 90 */
> 91 #define CHILD_REQUIRE(exp) do { \
> 92 if (!(exp)) \
> 93 child_fail_require(__FILE__, __LINE__, \
> 94 #exp " not met"); \
> 95 } while (0)
Disagree to what ? How this citation is relevant to my statement that
there are tests that use fork and ATF_ in children, I know about ptrace(2)
tests at least.
>
> The issue, in this particular case, and the item that evangyzen was fixing,
> was the fact that failures in children can result in very confusing failures
> from a parent level. In particular, ATF_CHECK does not trickle up to parents
> and ATF_REQUIRE* gets thrown up to parents as abort()’ed processes.
>
> The best way to handle this within the atf-c/atf-c++ framework (with less
> boilerplate) is to use these APIs: atf_utils_fork(..)/atf_utils_wait(..). You
> will still need to use `_exit` (instead of
> exit(..)/assert(..)/ATF_CHECK(..)/ATF_REQUIRE(..), but it’s a lot less
> boilerplate than writing it yourself.
This is not a way at all. Fork/wait is needed to create the child in
controlled matter, sometimes we must even know precisely which syscalls
are executed both by parent and child on their path. We need exactly
the bare-bone syscalls to create the testing situation.
>
> Again, this is why I was driving towards GoogleTest. Despite the fact that
> it’s a C++ framework, there’s a lot less confusing boilerplate involved and
> context, since things are executed in relatively the same context, i.e.,
> setup -> test -> teardown, and they’re easier to follow.
>
> The best way to move forward usability-wise with this (I think) is to
> probably alias the ATF_* macros to something more sensible when forking
> processes. However, given the amount of complaints I’ve heard about ATF, I
> think it’s best not to build upon an unstable foundation, but instead
> encourage the use of something more widely-accepted across the open source
> community/straightforward use wise. In this case, googletest.
Can we have some testing framework where tests can be conveniently debugged,
for the start ?
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r353305 - head/tests/sys/kern
> On Oct 8, 2019, at 08:04, Konstantin Belousov wrote:
>
> On Tue, Oct 08, 2019 at 01:43:05PM +, Eric van Gyzen wrote:
>> Author: vangyzen
>> Date: Tue Oct 8 13:43:05 2019
>> New Revision: 353305
>> URL: https://svnweb.freebsd.org/changeset/base/353305
>>
>> Log:
>> Fix problems in the kern_maxfiles__increase test
>>
>> ATF functions such as ATF_REQUIRE do not work correctly in child processes.
>> Use plain C functions to report errors instead.
> There are much more tests that fork and use ATF_ in children.
> Look e.g. at most ptrace(2) tests.
I beg to disagree:
86 /*
87 * A variant of ATF_REQUIRE that is suitable for use in child
88 * processes. This only works if the parent process is tripped up by
89 * the early exit and fails some requirement itself.
90 */
91 #define CHILD_REQUIRE(exp) do { \
92 if (!(exp)) \
93 child_fail_require(__FILE__, __LINE__, \
94 #exp " not met"); \
95 } while (0)
The issue, in this particular case, and the item that evangyzen was fixing, was
the fact that failures in children can result in very confusing failures from a
parent level. In particular, ATF_CHECK does not trickle up to parents and
ATF_REQUIRE* gets thrown up to parents as abort()’ed processes.
The best way to handle this within the atf-c/atf-c++ framework (with less
boilerplate) is to use these APIs: atf_utils_fork(..)/atf_utils_wait(..). You
will still need to use `_exit` (instead of
exit(..)/assert(..)/ATF_CHECK(..)/ATF_REQUIRE(..), but it’s a lot less
boilerplate than writing it yourself.
Again, this is why I was driving towards GoogleTest. Despite the fact that it’s
a C++ framework, there’s a lot less confusing boilerplate involved and context,
since things are executed in relatively the same context, i.e., setup -> test
-> teardown, and they’re easier to follow.
The best way to move forward usability-wise with this (I think) is to probably
alias the ATF_* macros to something more sensible when forking processes.
However, given the amount of complaints I’ve heard about ATF, I think it’s best
not to build upon an unstable foundation, but instead encourage the use of
something more widely-accepted across the open source community/straightforward
use wise. In this case, googletest.
Thanks,
-Enji
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r353305 - head/tests/sys/kern
On Tue, Oct 08, 2019 at 01:43:05PM +, Eric van Gyzen wrote:
> Author: vangyzen
> Date: Tue Oct 8 13:43:05 2019
> New Revision: 353305
> URL: https://svnweb.freebsd.org/changeset/base/353305
>
> Log:
> Fix problems in the kern_maxfiles__increase test
>
> ATF functions such as ATF_REQUIRE do not work correctly in child processes.
> Use plain C functions to report errors instead.
There are much more tests that fork and use ATF_ in children.
Look e.g. at most ptrace(2) tests.
>
> In the parent, check for the untimely demise of children. Without this,
> the test hung until the framework's timeout.
>
> Raise the resource limit on the number of open files. If this was too low,
> the test hit the two problems above.
>
> Restore the kern.maxfiles sysctl OID in the cleanup function.
> The body prematurely removed the symlink in which the old value was saved.
>
> Make the test more robust by opening more files. In fact, due to the
> integer division by 4, this was necessary to make the test valid with
> some initial values of maxfiles. Thanks, asomers@.
>
> wait() for children instead of sleeping.
>
> Clean up a temporary file created by the test ("afile").
>
> Reviewed by:asomers
> MFC after: 1 week
> Sponsored by: Dell EMC Isilon
> Differential Revision: https://reviews.freebsd.org/D21900
>
> Modified:
> head/tests/sys/kern/kern_descrip_test.c
>
> Modified: head/tests/sys/kern/kern_descrip_test.c
> ==
> --- head/tests/sys/kern/kern_descrip_test.c Tue Oct 8 11:27:48 2019
> (r353304)
> +++ head/tests/sys/kern/kern_descrip_test.c Tue Oct 8 13:43:05 2019
> (r353305)
> @@ -28,16 +28,22 @@
> __FBSDID("$FreeBSD$");
>
> #include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> #include
> #include
> #include
> #include
> #include
> +#include
> #include
> -#include
> -#include
> -#include
> #include
> +
> #include
>
> static volatile sig_atomic_t done;
> @@ -92,8 +98,13 @@ openfiles2(size_t n)
> int r;
>
> errno = 0;
> - for (i = 0; i < n; i++)
> - ATF_REQUIRE((r = open(AFILE, O_RDONLY)) != -1);
> + for (i = 0; i < n; i++) {
> + r = open(AFILE, O_RDONLY);
> + if (r < 0) {
> + fprintf(stderr, "open: %s\n", strerror(errno));
> + _exit(1);
> + }
> + }
> kill(getppid(), SIGUSR1);
>
> for (;;) {
> @@ -118,10 +129,14 @@ openfiles(size_t n)
> for (i = 0; i < PARALLEL; i++)
> if (fork() == 0)
> openfiles2(n / PARALLEL);
> - while (done != PARALLEL)
> + while (done != PARALLEL) {
> usleep(1000);
> + ATF_REQUIRE_EQ_MSG(0, waitpid(-1, NULL, WNOHANG),
> + "a child exited unexpectedly");
> + }
> unlink(RENDEZVOUS);
> - usleep(4);
> + for (i = 0; i < PARALLEL; i++)
> + ATF_CHECK_MSG(wait(NULL) > 0, "wait: %s", strerror(errno));
> }
>
> ATF_TC_WITH_CLEANUP(kern_maxfiles__increase);
> @@ -138,6 +153,7 @@ ATF_TC_BODY(kern_maxfiles__increase, tc)
> size_t oldlen;
> int maxfiles, oldmaxfiles, current;
> char buf[80];
> + struct rlimit rl;
>
> oldlen = sizeof(maxfiles);
> if (sysctlbyname("kern.maxfiles", , , NULL, 0) == -1)
> @@ -160,8 +176,11 @@ ATF_TC_BODY(kern_maxfiles__increase, tc)
> atf_tc_fail("getsysctlbyname(%s): %s", "kern.maxfiles",
> strerror(errno));
>
> - openfiles(oldmaxfiles - current + 1);
> - (void)unlink(VALUE);
> + rl.rlim_cur = rl.rlim_max = maxfiles;
> + ATF_REQUIRE_EQ_MSG(0, setrlimit(RLIMIT_NOFILE, ),
> + "setrlimit(RLIMIT_NOFILE, %d): %s", maxfiles, strerror(errno));
> +
> + openfiles(oldmaxfiles - current + EXPANDBY / 2);
> }
>
> ATF_TC_CLEANUP(kern_maxfiles__increase, tc)
> @@ -178,6 +197,8 @@ ATF_TC_CLEANUP(kern_maxfiles__increase, tc)
> , oldlen);
> }
> }
> + (void)unlink(VALUE);
> + (void)unlink(AFILE);
> }
>
> ATF_TP_ADD_TCS(tp)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r353305 - head/tests/sys/kern
Author: vangyzen
Date: Tue Oct 8 13:43:05 2019
New Revision: 353305
URL: https://svnweb.freebsd.org/changeset/base/353305
Log:
Fix problems in the kern_maxfiles__increase test
ATF functions such as ATF_REQUIRE do not work correctly in child processes.
Use plain C functions to report errors instead.
In the parent, check for the untimely demise of children. Without this,
the test hung until the framework's timeout.
Raise the resource limit on the number of open files. If this was too low,
the test hit the two problems above.
Restore the kern.maxfiles sysctl OID in the cleanup function.
The body prematurely removed the symlink in which the old value was saved.
Make the test more robust by opening more files. In fact, due to the
integer division by 4, this was necessary to make the test valid with
some initial values of maxfiles. Thanks, asomers@.
wait() for children instead of sleeping.
Clean up a temporary file created by the test ("afile").
Reviewed by: asomers
MFC after:1 week
Sponsored by: Dell EMC Isilon
Differential Revision:https://reviews.freebsd.org/D21900
Modified:
head/tests/sys/kern/kern_descrip_test.c
Modified: head/tests/sys/kern/kern_descrip_test.c
==
--- head/tests/sys/kern/kern_descrip_test.c Tue Oct 8 11:27:48 2019
(r353304)
+++ head/tests/sys/kern/kern_descrip_test.c Tue Oct 8 13:43:05 2019
(r353305)
@@ -28,16 +28,22 @@
__FBSDID("$FreeBSD$");
#include
+#include
+#include
+#include
+#include
+#include
+#include
+
#include
#include
#include
#include
#include
+#include
#include
-#include
-#include
-#include
#include
+
#include
static volatile sig_atomic_t done;
@@ -92,8 +98,13 @@ openfiles2(size_t n)
int r;
errno = 0;
- for (i = 0; i < n; i++)
- ATF_REQUIRE((r = open(AFILE, O_RDONLY)) != -1);
+ for (i = 0; i < n; i++) {
+ r = open(AFILE, O_RDONLY);
+ if (r < 0) {
+ fprintf(stderr, "open: %s\n", strerror(errno));
+ _exit(1);
+ }
+ }
kill(getppid(), SIGUSR1);
for (;;) {
@@ -118,10 +129,14 @@ openfiles(size_t n)
for (i = 0; i < PARALLEL; i++)
if (fork() == 0)
openfiles2(n / PARALLEL);
- while (done != PARALLEL)
+ while (done != PARALLEL) {
usleep(1000);
+ ATF_REQUIRE_EQ_MSG(0, waitpid(-1, NULL, WNOHANG),
+ "a child exited unexpectedly");
+ }
unlink(RENDEZVOUS);
- usleep(4);
+ for (i = 0; i < PARALLEL; i++)
+ ATF_CHECK_MSG(wait(NULL) > 0, "wait: %s", strerror(errno));
}
ATF_TC_WITH_CLEANUP(kern_maxfiles__increase);
@@ -138,6 +153,7 @@ ATF_TC_BODY(kern_maxfiles__increase, tc)
size_t oldlen;
int maxfiles, oldmaxfiles, current;
char buf[80];
+ struct rlimit rl;
oldlen = sizeof(maxfiles);
if (sysctlbyname("kern.maxfiles", , , NULL, 0) == -1)
@@ -160,8 +176,11 @@ ATF_TC_BODY(kern_maxfiles__increase, tc)
atf_tc_fail("getsysctlbyname(%s): %s", "kern.maxfiles",
strerror(errno));
- openfiles(oldmaxfiles - current + 1);
- (void)unlink(VALUE);
+ rl.rlim_cur = rl.rlim_max = maxfiles;
+ ATF_REQUIRE_EQ_MSG(0, setrlimit(RLIMIT_NOFILE, ),
+ "setrlimit(RLIMIT_NOFILE, %d): %s", maxfiles, strerror(errno));
+
+ openfiles(oldmaxfiles - current + EXPANDBY / 2);
}
ATF_TC_CLEANUP(kern_maxfiles__increase, tc)
@@ -178,6 +197,8 @@ ATF_TC_CLEANUP(kern_maxfiles__increase, tc)
, oldlen);
}
}
+ (void)unlink(VALUE);
+ (void)unlink(AFILE);
}
ATF_TP_ADD_TCS(tp)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
