Page MenuHomeFreeBSD

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

Authored by def on Aug 25 2018, 3:00 PM.

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 19194
Build 18810: arc lint + arc unit

Event Timeline

def created this revision.Aug 25 2018, 3:00 PM
def edited the summary of this revision. (Show Details)Aug 25 2018, 3:03 PM
def edited the test plan for this revision. (Show Details)
markj added inline comments.Aug 25 2018, 4:01 PM
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 updated this revision to Diff 47321.Aug 26 2018, 11:07 PM
def marked 4 inline comments as done.

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

def added inline comments.Aug 26 2018, 11:07 PM
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.

markj accepted this revision.Aug 27 2018, 4:15 PM

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
jhb added a comment.Aug 27 2018, 4:42 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.