Details
- Reviewers
mhorne pauamma_gundo.com - Group Reviewers
manpages NetApp - Commits
- rG1ae2c59bcf21: Add a manual page for boottrace(4)
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 43158 Build 40046: arc lint + arc unit
Event Timeline
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 52 | "holistic" may confuse EFL readers (it sent me to MW to check its exact meaning). | |
| 63 | Taste (minor): I'd use "fills up" or "gets full". | |
| 64 | Nitpick: I think panics happen more often than they're (user-)initiated. If you mean both, perhaps "Upon initiating a shutdown or reboot, or if a kernel panic occurs or is requested by the user, ..." | |
| 66 | As the amateur sysadmin for my own computer, my vote is for "yes". | |
| 80 | (Or whichever value(s) will enable tracing.) | |
| 85 | Do you mean "Return" (to userland) here? Or send to syslog? | |
| 102 | ||
| 104 | Not an actual edit suggestion, but I'm curious: why does that only exist for shutdown? | |
| 128 | Do you mean "userland" here, at any stage including shutdown? Or the "init+rc" stage only? | |
| 173 | This looks out of place here to me. Am I missing something? | |
| 201 | Or "for them". | |
Thank you very much @0mp for picking this up.
It would be an idea, a good one in fact :)
Note that boot(9) is now kern_reboot(9).
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 34 | This is the current intended semantic of 'run-time' events; everything logged after the completion of rc scripts (or to be precise, when init(8) transitions to multi-user) but before shutdown has begun. For example, loading a kmod on the command line will create an event in the run-time event buffer. We consider execution of rc scripts to be part of 'boot-time', but the choice is somewhat arbitrary. | |
| 50 | It was mentioned a few lines above that rc(8) is covered by boottrace. | |
| 75 | The format of which part was different? I can clarify any specific bits which there is confusion about. | |
| 189 | I don't think this section adds much, at least I would want to re-write the text comparing boottrace to tslog and I wouldn't classify it as a 'bug'. | |
With https://reviews.freebsd.org/D33275#inline-206447 in mind, why is it called "boottrace" specifically and not something that conveys more accurately which stages it covers?
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 202 | Maybe "completely" or "mostly" instead depending on its amount of self-containedness? | |
I am not the original author, so I cannot say exactly how the name was decided upon. It likely comes from the motivation of being able to trace the boot process, but the feature was extended to shutdown and some run-time events, possibly after the fact.
Since shutdown is the converse of system boot, I don't think it's counterintuitive that it would be covered by a feature named 'boottrace', but I agree that it's not obvious from the name alone. I understand the desire for a name that properly encapsulates all of the functionality, but I cannot think of one that does this without becoming more vague. Unless someone can propose a name that is clearly superior, I will not go through the pain of such a large rename.
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 113 | This variable is of no interest to end users, I suggest it be dropped from the man page. The only reason to use it is if one was working on the boottrace code, at which point they would find the tunable anyway. | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 34 | I'm marking this discussion as resolved for now. The semantic seems to be good enough. | |
| 50 | I think that it is alright to add some more details here. We didn't mention the event annotations earlier. I thought about making it a bit concise for a moment but I didn't find a better way to express it. | |
| 64 | I'd not go into details of panics here. This should be covered in a separate document explaining what a kernel panic is and how it can be occur. I agree that "upon initialization" may sound like a user-initiated action. I cannot think of a better way to write this sentence, though. Please let me know if you have anything on your mind. | |
| 66 | Makes sense. Also, many other many other manpages do that, eg. vte(4). | |
| 173 | It was a part of an old template. I removed it. | |
I made some comments here - but it looks to me like both my comments have or will be addressed shortly?
- Remove the BUGS section. We just need to create a TSLOG page at some point. Differences between TSLOG and boottrace should be mentioned in a commit message of boottrace.
- Improve formatting
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 75 | I cannot find that anymore. I think I was confusing the ${procname}: name format used by boottrace somewhere internally I suppose and the format of kern.boottrace.log. | |
| 104 | That's a good question. I'll ask @mhorne about it. | |
| 113 | Done! | |
| 128 | I'll ask @mhorne about it. | |
| 189 | I removed it. We should have a dedicated manual page for comparison of tracing facilities. Also, we need a tslog manual page. | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 34–35 | This seems like an unusual thing to state in the synopsis. Other pages simply list the relevant include file, sys/boottrace.h in this case. See watchdog(4), ng_split(4), tty(4) for other non-optional components that have this section. | |
| 70 | It actually takes over when the system has completed booting (including rc execution) and init(8) transitions to multi-user mode. | |
| 107 | Something like this to describe the expected format. | |
| 128 | It logs everything that has been recorded in the boot-time and run-time event tables (whether the kernel or userspace generated the event). It doesn't include shutdown events, because this sysctl is not expected to be invoked at any point during shutdown. There is the kern.boottrace.shutdown_trace option to have those events printed before the system performs its final shutdown action(s). | |
| 157 | ||
| 188 | I think this should reference sysctl(8) instead. Also add an xref for boottrace(1). | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 157 | ||
I'll post an updated patch in a moment.
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 34–35 | Good idea. I'm moving this part to the description section and adding that include statement. | |
| 70 | Thanks. I'll include that in a patch shortly. | |
| 107 | Oh I get it now. I'll add this bit of information to the manual. | |
| 157 | I assume that this is the exact output format of that sysctl. I'd prefer to stay as close to the real output as possible. However, if @mhorne managed to make the output of this sysctl narrower, then we should definitely update this example to reflect those changes. | |
| 188 | good idea | |
- add sys/boottrace.h to the synopsis
- move the information that boottrace is unconditionally compiled into the kernel out from the synopsis
- improve descriptions of event tables
- explain the expected format of the string passed to the kern.boottrace.boottrace sysctl
- add an example of how using "sysctl kern.boottrace.boottrace" to create events
- update the See Also section
Other than that question, LGTM.
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 122 | Clarification request: is that the executable name, or the title set by setproctitle() or similar means? | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 122 | Good question, I'll look into that. | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 122 | It is the executable name. See getprogname(3) for details. | |
| share/man/man4/boottrace.4 | ||
|---|---|---|
| 122 | Ach, I didn't know about that. Thanks! | |