Added the following tests
- sbuf_clear_flags_test
- sbuf_set_get_flags_test
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 33587 Build 30838: arc lint + arc unit
Event Timeline
Comment Actions
I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?
Comment Actions
@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.
From sys/kern/subr_sbuf.c
KASSERT(length >= 0,
("attempt to create an sbuf of negative length (%d)", length));
KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
("%s called with invalid flags", __func__));or when SBMALLOC fails..
Comment Actions
So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.
Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.
As for how to make sbuf fail due to malloc, I don't know.
Comment Actions
@jmg @imp I have seen other places where KASSERT was re-defined as
#define KASSERT(exp,msg) assert((exp))
Another way
http://fxr.watson.org/fxr/source/netpfil/ipfw/test/dn_test.h#L143
#define KASSERT(x, y) do { if (!(x)) printf y ; exit(0); } while (0)for the tests. Please let me know if we can go that route.
Comment Actions
I think using assert should be used. It doesn't pollute stdout, generates a core dump, and is more obvious that there was a failure.
Thanks.
Comment Actions
I think using the other one is better since we'll get the message. assert doesn't give the message:
#define KASSERT(x, y) do { if (!(x)) printf y ; exit(1); } while (0)though maybe 'abort' would be better. exit(0) is bad because the test won't be considered failed, perhaps...
Comment Actions
atf_tc_expect_death("....") will print the reason. For e.g.
sbuf_core_test:sbuf_new_negative_test -> expected_failure: Buffer length cannot be negative [0.008s] sbuf_core_test:sbuf_new_negative_test_min_buf_size -> expected_failure: Minimum buffer length should be 2 [0.008s] sbuf_core_test:sbuf_new_negative_test_non_user_flags -> expected_failure: Non user flags cannot be specified [0.008s] sbuf_core_test:sbuf_setpos_test_negative_pos -> expected_failure: position cannot be negative [0.008s]
Comment Actions
@imp if the test binary is run directly like this. We can see the assertion output.
./sbuf_core_test sbuf_new_negative_test_non_user_flags sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4) expected_death: Non user flags cannot be specified Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233. Abort (core dumped)
Comment Actions
@imp tried the do while.. & assert. In both cases kyua does not print to log file. I see the msg only when the binary is run standalone or a single test case is run. For e.g.
kyua debug sbuf_core_test:sbuf_new_negative_test Assertion failed: ((length >= 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 231. Process with PID 29505 exited with signal 6 and dumped core; attempting to gather stack trace Cannot find GDB binary; builtin was 'gdb' sbuf_core_test:sbuf_new_negative_test -> expected_failure: Buffer length cannot be negative
@imp I removed the output from the kyua log from this comment.
Comment Actions
This change does more than advertised in the CR description (which doesn't describe as much as the commit message should).
- How was it determined that these areas are missing coverage? Code inspection? gcov?
- This changes the behavior of libsbuf by asserting on exceptional cases that can be triggered by humans. Also, compiling libsbuf with MK_ASSERT_DEBUG==no will completely invalidate the assertions breaking the test and triggering unwanted behavior where KASSERT was used. Libraries with publicly moving parts shouldn't assert (especially on invalid input); they should return sensible errors which are documented in the manpages which describe the APIs.
| lib/libsbuf/tests/sbuf_core_test.c | ||
|---|---|---|
| 109–118 | Please use __unused. | |
| 150–164 | Why were these invariants removed? | |
| 185 | Use _exit instead of exit. This avoids the atexit handlers which may have been installed earlier on in the application or library: EXIT(2) FreeBSD System Calls Manual EXIT(2)
NAME
_exit – terminate the calling process
LIBRARY
Standard C Library (libc, -lc)
SYNOPSIS
#include <unistd.h>
void
_exit(int status);
DESCRIPTION
The _exit() system call terminates a process with the following
consequences:
• All of the descriptors open in the calling process are closed. This
may entail delays, for example, waiting for output to drain; a
process in this state may not be killed, as it is already dying.
• If the parent process of the calling process has an outstanding
wait(2) call or catches the SIGCHLD signal, it is notified of the
calling process's termination and the status is set as defined by
wait(2).
• The parent process-ID of all of the calling process's existing child
processes are set to the process-ID of the calling process's reaper;
the reaper (normally the initialization process) inherits each of
these processes (see procctl(2), init(8) and the DEFINITIONS section
of intro(2)).
• If the termination of the process causes any process group to become
orphaned (usually because the parents of all members of the group
have now exited; see “orphaned process group” in intro(2)), and if
any member of the orphaned group is stopped, the SIGHUP signal and
the SIGCONT signal are sent to all members of the newly-orphaned
process group.
• If the process is a controlling process (see intro(2)), the SIGHUP
signal is sent to the foreground process group of the controlling
terminal, and all current access to the controlling terminal is
revoked.
Most C programs call the library routine exit(3), which flushes buffers,
closes streams, unlinks temporary files, etc., before calling _exit(). | |
| sys/kern/subr_sbuf.c | ||
| 46–63 | This seems like it should be a separate change, maybe. Also, have all of the code paths been analyzed to ensure there aren't dead code paths after this change with libsbuf? | |
Comment Actions
@ngie @lwhsu Hello, I found this on https://wiki.freebsd.org/JuniorJobs, Please close this. This is more involved than what i could comprehend from that page.