Page MenuHomeFreeBSD

Print fatal errors/warnings to STDERR and add -q (quiet) option
Needs ReviewPublic

Authored by aryeeteygerald_rogers.com on Jan 16 2019, 8:34 PM.

Details

Reviewers
emaste
delphij
Summary

Pipes all warnings and errors to /dev/stderr.
Allows scripts to ignore messages/decipher error text

If quiet option is used (for fetch/cron/IDS) then
stdout is not output to the user

Also adds a new verbosity "none" which could be configured
using the freebsd.conf

PR: 230243,199776

Test Plan

None

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22065
Build 21293: arc lint + arc unit

Event Timeline

I like the idea in general, but I think there were some implementation issues:

A quick glance suggests that the >&2 treatments were mostly, if not all on echo's. Instead of doing them inline, it's better to use a sub-routine as it would be more obvious that the message is for stderr.

Alternatively, note that most of freebsd-update messages are errors, it's probably better to exclude the non-error messages instead (to make the change less intrusive).

Some of formatting echo's (those without parameters and would output a new line) are not redirected at the same time but they should. For the ones near to cat - block, it's probably reasonable to just fold them into the block as a blank line instead.

Please also see my comments inline.

usr.sbin/freebsd-update/freebsd-update.sh
1152

In quiet mode, shouldn't this be omitted (also the "done." in line 1164)?

1157

I'd strongly recommend not using INFOREDIR as a flag, especially using negative logic. This would hurt readability and make the code hard to follow.

1161

Similar.

1165

See comment above.

1423

Similar.

1977

These are intentional. With the proposed change, the contents would no longer go to $PAGER and on typical terminals it means freebsd-update would only show a list of files, but not these descriptive messages.

3166

This is not needed (already redirected in line 3246).

3248

The removal doesn't appear to be intentional?

3310

This would probably not work in practice. Why not simply fetch_run > ${TMPFILE} 2>&1 below instead? (I don't think the >> is right in this context, because at this point the TMPFILE is guaranteed to be empty).

Note that this actually could have introduced a security vulnerability, by the way. Assuming one creates a symlink called '_fetch_results', mkfifo would fail (mkfifo would EEXIST and bail out) and the output would be written with root privileges...

This revision now requires changes to proceed.Jan 18 2019, 12:41 AM

It may be because it is hard to follow but warnings/errors are redirected to the stderr
And then the stdout fetch_run/IDS_run is intended to be piped to INFOREDIR. This is normally stdout but can be /dev/null in order to hide informational messages ...
is this a bad idea?

It may be because it is hard to follow but warnings/errors are redirected to the stderr

Yes, I think we are on the same page here.

And then the stdout fetch_run/IDS_run is intended to be piped to INFOREDIR. This is normally stdout but can be /dev/null in order to hide informational messages ...
is this a bad idea?

No, it's totally fine to use /dev/null to hide informational messages (assuming there is no command line option to suppress it, or the command line option was not suppressing it enough).

Note that most IDS_run output shouldn't be considered as "informational", especially checksum differences: these should not be suppressed because they suggest there are real issues.

aryeeteygerald_rogers.com marked an inline comment as done.
  • Improve code for printing to stderr

No longer uses INFOREDIR as a flag (longer error message instead).
No longer prints pager info messages to STDERR

aryeeteygerald_rogers.com added inline comments.
usr.sbin/freebsd-update/freebsd-update.sh
1157

The alternative is longer error messages as shown or adding an additional flag ... which is better?

3310

Your suggestion will cause cron to ignore -q ... is this what we want?

Note that most IDS_run output shouldn't be considered as "informational", especially checksum differences: these should not be suppressed because they suggest there are real issues.

Where are the checksum diffs? I don't think they are currently suppressed but maybe I missed it.

  • Fix style/syntax issues

Add missing closing brace
Use LINE instead of line as seen elsewhere in this script

Note that most IDS_run output shouldn't be considered as "informational", especially checksum differences: these should not be suppressed because they suggest there are real issues.

Where are the checksum diffs? I don't think they are currently suppressed but maybe I missed it.

Sorry it was my typo. It's IDS_compare's output (called by IDS_run).

Note that most IDS_run output shouldn't be considered as "informational", especially checksum differences: these should not be suppressed because they suggest there are real issues.

Where are the checksum diffs? I don't think they are currently suppressed but maybe I missed it.

Sorry it was my typo. It's IDS_compare's output (called by IDS_run).

Oh, those are already considered errors/warnings by the pipe on line 3228.