Page MenuHomeFreeBSD

Refactor crashinfo(8) script before extending its functionality.
AcceptedPublic

Authored by def on Aug 25 2018, 3:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 1, 5:46 AM
Unknown Object (File)
Dec 18 2022, 7:35 AM
Subscribers
None

Details

Reviewers
oshogbo
markj
jhb
Summary

This refactoring includes the following changes:

  • Introduce separate functions to analyze a core dump using sh(1) commands and kgdb(1) commands which can be later used for user-defined commands;
  • Introduce main() function to declare variables used only for parsing arguments and preparing a core summary as local;
  • Declare global variables at the top of the script;
  • Reduce a scope of variables within functions to the local scope.

As a result of the refactoring, disallow to pass empty flag arguments or use the -d flag with an explicit vmcore.

Test Plan

This change was tested with the following cases:

  • Generate a summary for a core using an automatically detected or a given kernel;
  • Generate a summary for a core with a given dumpnr within the default or a given core dump directory using an automatically detected or a given kernel;
  • Generate a summary for a core compressed with gzip(1);
  • Generate a summary for a core compressed with zstd(1);
  • Generate a summary for a core and write it to a core.txt.# file;
  • Verify if the -d flag and explicit vmcore are mutually exclusive;
  • Verify if the -n flag and explicit vmcore are mutually exclusive.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19194
Build 18810: arc lint + arc unit

Event Timeline

def edited the test plan for this revision. (Show Details)
usr.sbin/crashinfo/crashinfo.sh
46

Perhaps this should go to stderr?

54

The variable name VMORE is misspelled.

150

You could write this entire routine as

dirname "${1}"

Ditto for core_dumpnr(). If you prefer to keep the variable names, I'd suggest something other than "name". Maybe "path"?

215

What's the purpose of the bare echo?

def marked 4 inline comments as done.

This update fixes typos, style and changes die() to write messages to stderr.

usr.sbin/crashinfo/crashinfo.sh
46

I didn't want to change the current behaviour. However since you also proposed this I've decided to apply this change.

54

Nice catch.

150

I prefer to keep the variables. I agree that path sounds better.

215

kgdb output doesn't end with a new line. I moved this line before rm so that it's easier to understand.

Looks ok to me modulo a minor nit.

usr.sbin/crashinfo/crashinfo.sh
46

It could be done in a separate change; I didn't realize when making that comment that you were avoiding functional changes. Up to you.

79

This comment no longer holds.

215

I'd argue it's even worth a comment, but I'll leave that up to you.

This revision is now accepted and ready to land.Aug 27 2018, 4:15 PM

Renaming all the functions seems a bit gratuitous but is fine I guess. Previously they read more as sentences "find gdb", etc. and if you matched that you would probably use 'generate_info' (core_generate is odd as we aren't generating a core dump, but generating the info about a core). It is true that most of our APIs in C do use prefixes, but shell scripts don't tend to follow that pattern (/etc/network.subr has a mix, and many of them are more of the imperative style "do_this" like the existing names). Using a main() in a shell script is not the normal style in FreeBSD. rc.d scripts don't use that, neither do etcupdate, /etc/rc, etc.

usr.sbin/crashinfo/crashinfo.sh
87

The quotes here (and around GDB elsewhere) don't seem really useful since the expansion of GDB_PATHS above ensures that any paths in GDB_PATHS that would contain a space wouldn't work, so it seems to give the user a false sense of something that would work that won't work in practice.