witness: support a configurable output channel
ClosedPublic
Actions

Authored by markj on Nov 16 2015, 6:47 PM.

Details

Reviewers

cem
jhb

Group Reviewers

manpages

Commits

rS291059: Add support for a configurable output channel to witness(4).
rS291058: Add vlog(9).

Summary

At the moment, witness always reports LORs and other lock rule
violations by printing to the console. This is problematic for
environments that use pexpect-ish tools to configure FreeBSD systems via
the console: they can get confused by unexpected witness output.

This change adds a witness "output channel" sysctl that lets one tell
witness to use log(9) to log warnings rather than printing to the
console. It wraps all the printf() calls with witness_output(), which
selects the output channel. This change also adds vlog(9), since we
didn't already have it; it'll be committed separately.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj updated this revision to Diff 10242.Nov 16 2015, 6:47 PM

markj retitled this revision from to witness: support a configurable output channel.

markj edited the test plan for this revision. (Show Details)

markj updated this object.

Herald added a reviewer: manpages. · View Herald TranscriptNov 16 2015, 6:47 PM

Herald added a subscriber: imp. · View Herald Transcript

markj added reviewers: jhb, cem.Nov 16 2015, 6:49 PM

cem accepted this revision.Nov 16 2015, 7:06 PM

cem edited edge metadata.

cem added inline comments.

sys/kern/subr_witness.c
432–433 ↗	(On Diff #10242)	The man page documents this as tunable.
2692 ↗	(On Diff #10242)	`static const`?
2701 ↗	(On Diff #10242)	I'd use an unsigned type for `i`.
2718 ↗	(On Diff #10242)	`break;`?
2989 ↗	(On Diff #10242)	`"%.*s", len, data`? I guess we don't need it if sbuf always nul-terminates strings.

This revision is now accepted and ready to land.Nov 16 2015, 7:06 PM

Address review comments from cem.

This revision now requires review to proceed.Nov 16 2015, 7:17 PM

Herald edited edge metadata. · View Herald TranscriptNov 16 2015, 7:17 PM

cem accepted this revision.Nov 16 2015, 7:19 PM

cem edited edge metadata.

This revision is now accepted and ready to land.Nov 16 2015, 7:19 PM

markj marked 4 inline comments as done.Nov 16 2015, 7:20 PM

markj added inline comments.

sys/kern/subr_witness.c
432–433 ↗	(On Diff #10243)	Oops, thanks. I wasn't sure if RWTUN+SYSCTL_PROC would work, and forgot to actually check. Turns out that it works fine.
2991 ↗	(On Diff #10243)	Yeah, I don't think that's needed. There's an SBUF_INCLUDENUL flag, but that only determines whether the null byte is included in len - the buffer itself should always be null-terminated.

jhb added inline comments.Nov 16 2015, 9:29 PM

sys/kern/subr_witness.c
319 ↗	(On Diff #10243)	Better to drop the leading _ from the function name and drop the macro altogether.
2601 ↗	(On Diff #10243)	This looks unrelated (though a worthy cleanup)
3009 ↗	(On Diff #10243)	I think it would be clearer if we instead made stack_*_ddb() present in the kernel when WITNESS is enabled. and always use it instead of depending on '#ifdef DDB' here.

Address review comments from jhb.

This revision now requires review to proceed.Nov 16 2015, 10:00 PM

Herald edited edge metadata. · View Herald TranscriptNov 16 2015, 10:00 PM

markj marked 2 inline comments as done.Nov 16 2015, 10:02 PM

markj added inline comments.

sys/kern/subr_witness.c
2601 ↗	(On Diff #10248)	Oops, indeed. I'll commit that separately.
3002 ↗	(On Diff #10248)	That's what I've done in the latest upload, but I wonder if it wouldn't be best to just define stack__ddb unconditionally. We already define linker_ddb_ unconditionally, and these functions are misnomers anyway. They're for any caller that wants to avoid acquiring locks during symbol resolution.

jhb accepted this revision.Nov 16 2015, 10:56 PM

jhb edited edge metadata.

jhb added inline comments.

sys/kern/subr_witness.c
3002 ↗	(On Diff #10248)	Yes, I almost suggested doing s/ddb/unlocked/ or some such on the names while you are at it. Having them still be conditional is nice to the embedded folks perhaps, but agree that for that to truly be worthwhile the linker ones would need the same treatment.